Re: [PATCH 3/4] maint: don't leak developer GREP, SED etc into distribution file.

[Ralf Wildenhues]

Hi Gary,

* Gary V. Vaughan wrote on Wed, Sep 22, 2010 at 07:05:50PM CEST:
> This is the second part of the patch I've split since the last
> time I posted.  I added Joerg as reporter, and he is already
> named in THANKS.
> 
> Okay to push?

With this patch applied, the generated libtool script still contains
the following lines:

: ${EGREP="@EGREP@"}
: ${FGREP="@FGREP@"}
: ${GREP="@GREP@"}
: ${LN_S="@LN_S@"}
[...]
: ${SED="@SED@"}

Now, the lines are all ineffective, because the tag variables (which
will be expanded before these lines) will set all of these variables.
Kind of ugly, but alright if you prefer to clean that up later.


Secondly, with this patch, and with the nits I wrote inline in the patch
below fixed, still freebsd-make (which is from the freebsd-buildutils
7.0-2 Debian package, and is a somewhat older FreeBSD make built for
GNU/Linux) keeps on rerunning aclocal each time I invoke make.  This
means something is not quite right in the rebuild rules, and it will
probably show up with at least one of the major BSD systems' make
implementations.  Please fix and repost, I will finish review then.

In case of doubt it might be easier to not try to change the actual
dependency graph at all, but merely the rules, which should be enough
to fix the problem you are targeting.

Thanks,
Ralf


> * Makefile.am: Having rearranged the file, now apply the actual
> changes to follow-up.
> (LT_M4SH): Call $(M4SH) with the libtool m4sh directory at the
> front of the search path.
> (rebuild): Set the shell variable `revision' rather than
> `correctver' for clarity.
> (edit): Split into two parts...
> (bootstrap_edit): ...substitutions that should happen at bootstrap
> time...
> (configure_edit): ...and substitutions that should not happen until
> configure time.
> * libtoolize.m4sh (package_revision): For consistency, and ease of
> extraction when deciding whether libtoolize needs to be
> regenerated.
> * Makefile.am (libltdl/m4/ltversion.m4, libltdl/config/ltmain.sh)
> (libtoolize.in): Much simplified - only rebuild if the
> recalculated timestamp is newer than the timestamp recorded in the
> target already; regenerate in one step without changing
> directories, using `$(bootstrap_edit)'.
> (libtoolize, libtool): Hugely simplified - since we already depend
> on `libtoolize.in' and `libltdl/config/ltmain.sh' respectively,
> no need to recheck version numbers; also they have the
> `$(bootstrap_edit)' substitutions made already, so we can just
> go ahead and process with `$(configure_edit)' whenever the
> dependencies go out of date.
> * Makefile.maint (commit, libltdl/config/mailnotify, bootstrap):
> Likewise.
> * HACKING (Release Procedure): Remove the note to workaround the
> bug fixed by this changeset.
> * NEWS (Bug fixes): Mention that this bug is now fixed.
> Reported by Joerg Sonnenberger.

> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -43,6 +43,8 @@ noinst_LTLIBRARIES  =
>  lib_LTLIBRARIES              =
>  EXTRA_LTLIBRARIES    =
>  
> +LT_M4SH                      = $(M4SH) -B $(srcdir)/$(auxdir)
> +
>  auxdir                       = libltdl/config
>  m4dir                        = libltdl/m4
>  
> @@ -57,7 +59,7 @@ timestamp = set dummy `$(MKSTAMP) $(srcdir)`; shift; \
>         *) TIMESTAMP="" ;; \
>       esac
>  
> -rebuild = rebuild=:; $(timestamp); correctver=$$1
> +rebuild = rebuild=:; $(timestamp); revision=$$1
>  
>  
>  # ---------- #
> @@ -75,26 +77,23 @@ EXTRA_DIST     += bootstrap $(srcdir)/libtoolize.in 
> $(auxdir)/ltmain.m4sh \
>  CLEANFILES     += libtool libtoolize libtoolize.tmp \
>                 $(auxdir)/ltmain.tmp $(m4dir)/ltversion.tmp
>  
> -edit = sed \
> -     -e 's,@EGREP\@,$(EGREP),g' \
> -     -e 's,@FGREP\@,$(FGREP),g' \
> -     -e 's,@GREP\@,$(GREP),g' \
> -     -e 's,@LN_S\@,$(LN_S),g' \
> -     -e 's,@MACRO_VERSION\@,$(VERSION),g' \
> -     -e 's,@PACKAGE\@,$(PACKAGE),g' \
> -     -e 's,@PACKAGE_BUGREPORT\@,$(PACKAGE_BUGREPORT),g' \
> -     -e 's,@PACKAGE_URL\@,$(PACKAGE_URL),g' \
> -     -e 's,@PACKAGE_NAME\@,$(PACKAGE_NAME),g' \
> -     -e 's,@PACKAGE_STRING\@,$(PACKAGE_NAME) $(VERSION),g' \
> -     -e 's,@PACKAGE_TARNAME\@,$(PACKAGE),g' \
> -     -e 's,@PACKAGE_VERSION\@,$(VERSION),g' \
> -     -e 's,@SED\@,$(SED),g' \
> -     -e 's,@VERSION\@,$(VERSION),g' \
> -     -e 's,@aclocaldir\@,$(aclocaldir),g' \
> -     -e 's,@datadir\@,$(datadir),g' \
> -     -e 's,@pkgdatadir\@,$(pkgdatadir),g' \
> -     -e 's,@host_triplet\@,$(host_triplet),g' \
> -     -e 's,@prefix\@,$(prefix),g'
> +## These are the replacements that need to be made at bootstrap time,
> +## because they must be static in distributed files, and not accidentally
> +## changed by configure running on the build machine.
> +bootstrap_edit  = \
> +               -e 's,@MACRO_VERSION\@,$(VERSION),g' \

How come the *_edit variables lost their 'sed' command?  Is that because
a later patch will use more than one of them inside one sed script?  In
that case, OK with me.

> +               -e "s,@MACRO_REVISION\@,$$revision,g" \
> +               -e "s,@MACRO_SERIAL\@,$$serial,g" \
> +               -e 's,@PACKAGE\@,$(PACKAGE),g' \
> +               -e 's,@PACKAGE_BUGREPORT\@,$(PACKAGE_BUGREPORT),g' \
> +               -e 's,@PACKAGE_URL\@,$(PACKAGE_URL),g' \
> +               -e 's,@PACKAGE_NAME\@,$(PACKAGE_NAME),g' \
> +               -e "s,@package_revision\@,$$revision,g" \
> +               -e 's,@PACKAGE_STRING\@,$(PACKAGE_NAME) $(VERSION),g' \
> +               -e 's,@PACKAGE_TARNAME\@,$(PACKAGE),g' \
> +               -e 's,@PACKAGE_VERSION\@,$(VERSION),g' \
> +               -e "s,@TIMESTAMP\@,$$TIMESTAMP,g" \
> +               -e 's,@VERSION\@,$(VERSION),g'
>  
>  ## We build ltversion.m4 here, instead of from config.status,
>  ## because config.status is rerun each time one of configure's
> @@ -104,31 +103,25 @@ edit = sed \
>  ## We used to do this with a 'stamp-vcl' file, but non-gmake builds
>  ## would rerun configure on every invocation, so now we manually
>  ## check the version numbers from the build rule when necessary.
> -## Use `$(srcdir)/m4' for the benefit of non-GNU makes: this is
> +## Use `$(srcdir)/$(m4dir)' for the benefit of non-GNU makes: this is
>  ## how ltversion.m4 appears in our dependencies.
>  EXTRA_DIST += $(m4dir)/ltversion.in $(srcdir)/$(m4dir)/ltversion.m4
>  $(srcdir)/$(m4dir)/ltversion.m4: $(m4dir)/ltversion.in configure.ac ChangeLog
> -     @target='$(srcdir)/$(m4dir)/ltversion.m4'; $(rebuild); \
> -     if test -f "$$target"; then \
> -       set dummy `sed -n '/^# serial /p' "$$target"`; shift; \
> -       actualver=1.$$3; \
> -       test "$$actualver" = "$$correctver" && rebuild=false; \
> +     @$(rebuild); \
> +     if test -f "$@"; then \
> +       eval `sed -n '/^macro_revision=/p' "$@"`; \
> +       test "$$macro_revision" = "$$revision" && rebuild=false; \
>       fi; \
>       for prereq in $?; do \
>         case $$prereq in *ChangeLog | *configure.ac);; *) rebuild=:;; esac; \
>       done; \
>       if $$rebuild; then \
> -       cd $(srcdir); \
> -       rm -f $(m4dir)/ltversion.tmp; \
> -       serial=`echo "$$correctver" | sed 's,^1[.],,g'`; \
> -       echo $(edit) -e "s,@MACRO_REVISION\@,$$correctver,g" \
> -         -e "s,@MACRO_SERIAL\@,$$serial,g" \
> -         $(srcdir)/$(m4dir)/ltversion.in \> $(srcdir)/$(m4dir)/ltversion.m4; 
> \
> -       $(edit) -e "s,@MACRO_REVISION\@,$$correctver,g" \
> -               -e "s,@MACRO_SERIAL\@,$$serial,g" \
> -               $(m4dir)/ltversion.in > $(m4dir)/ltversion.tmp; \
> -       chmod a-w $(m4dir)/ltversion.tmp; \
> -       mv -f $(m4dir)/ltversion.tmp $(m4dir)/ltversion.m4; \
> +       rm -f address@hidden; \
> +       serial=`echo "$$revision" | sed 's,^[1-9][0-9]*[.],,g'`; \
> +       echo "sed $(bootstrap_edit) $< > $@"; \

Using $< outside of inference rules is not portable to non-GNU make.
You can spell out the file name instead: $(srcdir)/$(m4dir)/ltversion.in

> +       sed $(bootstrap_edit) $< > address@hidden; \

Likewise.

> +       chmod a-w address@hidden; \
> +       mv -f address@hidden $@; \
>       fi
>  
>  ## And for similar reasons, ltmain.sh can't be built from config.status.
> @@ -136,48 +129,48 @@ $(srcdir)/$(m4dir)/ltversion.m4: $(m4dir)/ltversion.in 
> configure.ac ChangeLog
>  ## would rerun configure on every invocation, so now we manually
>  ## check the version numbers from the build rule when necessary.
>  ## !WARNING! If you edit this rule to change the contents of ltmain.sh,
> -##           you must `touch $(srcdir)/$(auxdir)/ltmain.in' from the
> +##           you must `touch $(srcdir)/$(auxdir)/ltmain.m4sh' from the
>  ##           shell if you need ltmain.sh to be regenerated.  Ideally, we
>  ##           should make this rule depend on Makefile but that will break
>  ##           distcheck (at least) by rebuilding ltmain.sh in the source
>  ##           tree whenever config.status regenerates the Makefile.
>  EXTRA_DIST += $(srcdir)/$(auxdir)/ltmain.sh
> -$(srcdir)/$(auxdir)/ltmain.sh: $(sh_files) $(auxdir)/ltmain.m4sh 
> configure.ac ChangeLog
> -     @target='$(srcdir)/$(auxdir)/ltmain.sh'; $(rebuild); \
> -     if test -f "$$target"; then \
> -       eval `sed -n '/^package_revision=/p' "$$target"`; \
> -       actualver=$$package_revision; \
> -       test "$$actualver" = "$$correctver" && rebuild=false; \
> +$(srcdir)/$(auxdir)/ltmain.sh: $(auxdir)/ltmain.m4sh $(sh_files) 
> configure.ac ChangeLog
> +     @$(rebuild); \
> +     if test -f "$@"; then \
> +       eval `sed -n '/^package_revision=/p' "$@"`; \
> +       test "$$package_revision" = "$$revision" && rebuild=false; \
>       fi; \
>       for prereq in $?; do \
>         case $$prereq in *ChangeLog);; *) rebuild=:;; esac; \
>       done; \
>       if $$rebuild; then \
> -       cd $(srcdir); \
> -       rm -f $(auxdir)/ltmain.in $(auxdir)/ltmain.tmp \
> -         $(auxdir)/ltmain.sh; \
> -       echo $(M4SH) -B $(auxdir) $(auxdir)/ltmain.m4sh \
> -         \> $(auxdir)/ltmain.in; \
> -       $(M4SH) -B $(auxdir) $(auxdir)/ltmain.m4sh \
> -         > $(auxdir)/ltmain.in; \
> -       echo $(edit) -e "s,@TIMESTAMP\@,$$TIMESTAMP,g" \
> -         -e "s,@package_revision\@,$$correctver," \
> -         $(srcdir)/$(auxdir)/ltmain.in "> $$target"; \
> -       $(edit) -e "s,@TIMESTAMP\@,$$TIMESTAMP,g" \
> -             -e "s,@package_revision\@,$$1,g" \
> -             $(auxdir)/ltmain.in > $(auxdir)/ltmain.tmp; \
> -       rm -f $(auxdir)/ltmain.in; \
> -       chmod a-w $(auxdir)/ltmain.tmp; \
> -       mv -f $(auxdir)/ltmain.tmp $(auxdir)/ltmain.sh; \
> +       rm -f address@hidden; \
> +       echo '$(LT_M4SH) $< | sed $(bootstrap_edit) > $@'; \

See above $<.

> +       $(LT_M4SH) $< | sed $(bootstrap_edit) > address@hidden; \

Likewise.

> +       chmod a-w address@hidden; \
> +       mv -f address@hidden $@; \
>       fi
>  
>  # Use `$(srcdir)' for the benefit of non-GNU makes: this is
>  # how libtoolize.in appears in our dependencies.
>  EXTRA_DIST += libtoolize.m4sh
> -$(srcdir)/libtoolize.in: $(sh_files) libtoolize.m4sh Makefile.am
> -     cd $(srcdir); \
> -     rm -f libtoolize.in; \
> -     $(M4SH) -B $(auxdir) libtoolize.m4sh > libtoolize.in
> +$(srcdir)/libtoolize.in: $(srcdir)/libtoolize.m4sh $(sh_files) configure.ac 
> ChangeLog
> +     @$(rebuild); \
> +     if test -f "$@"; then \
> +       eval `sed -n '/^package_revision=/p' "$@"`; \
> +       test "$$package_revision" = "$$revision" && rebuild=false; \
> +     fi; \
> +     for prereq in $?; do \
> +       case $$prereq in *ChangeLog);; *) rebuild=:;; esac; \
> +     done; \
> +     if $$rebuild; then \
> +       rm -f address@hidden; \
> +       echo '$(LT_M4SH) $< | sed $(bootstrap_edit) > $@'; \
> +       $(LT_M4SH) $< | sed $(bootstrap_edit) > address@hidden; \

See above (two instances).

> +       chmod a-w address@hidden; \
> +       mv -f address@hidden $@; \
> +     fi
>  
>  $(srcdir)/libltdl/Makefile.am: $(srcdir)/libltdl/Makefile.inc
>       cd $(srcdir); \
> @@ -220,37 +213,32 @@ all-local: $(LTDL_BOOTSTRAP_DEPS)
>  ## Libtool scripts. ##
>  ## ---------------- ##
>  
> +configure_edit = \
> +     -e 's,@aclocal_DATA\@,$(aclocalfiles),g' \
> +     -e 's,@aclocaldir\@,$(aclocaldir),g' \
> +     -e 's,@datadir\@,$(datadir),g' \
> +     -e 's,@EGREP\@,$(EGREP),g' \
> +     -e 's,@FGREP\@,$(FGREP),g' \
> +     -e 's,@GREP\@,$(GREP),g' \
> +     -e 's,@host_triplet\@,$(host_triplet),g' \
> +     -e 's,@LN_S\@,$(LN_S),g' \
> +     -e "s,@pkgconfig_files\@,$(auxfiles),g" \
> +     -e 's,@pkgdatadir\@,$(pkgdatadir),g' \
> +     -e "s,@pkgltdl_files\@,$(ltdldatafiles),g" \
> +     -e 's,@prefix\@,$(prefix),g' \
> +     -e 's,@SED\@,$(SED),g'
> +
>  # The libtool distributor and the standalone libtool script.
>  bin_SCRIPTS = libtoolize libtool
>  
>  libtoolize: $(srcdir)/libtoolize.in $(top_builddir)/config.status
> -     rm -f libtoolize.tmp libtoolize
> -     $(timestamp); \
> -     $(edit) -e "s,@TIMESTAMP\@,$$TIMESTAMP,g" \
> -             -e 's,@aclocal_DATA\@,$(aclocalfiles),g' \
> -             -e "s,@pkgltdl_files\@,$(ltdldatafiles),g" \
> -             -e "s,@pkgconfig_files\@,$(auxfiles),g" \
> -             $(srcdir)/libtoolize.in > libtoolize.tmp
> -     chmod a+x libtoolize.tmp
> -     chmod a-w libtoolize.tmp
> -     mv -f libtoolize.tmp libtoolize
> -
> -# We used to do this with a 'stamp-vcl' file, but non-gmake builds
> -# would rerun configure on every invocation, so now we manually
> -# check the version numbers from the build rule when necessary.
> -libtool: $(top_builddir)/config.status $(srcdir)/$(auxdir)/ltmain.sh 
> ChangeLog
> -     @target=libtool; $(rebuild); \
> -     if test -f "$$target"; then \
> -       set dummy `./$$target --version | sed 1q`; actualver="$$5"; \
> -       test "$$actualver" = "$$correctver" && rebuild=false; \
> -     fi; \
> -     for prereq in $?; do \
> -       case $$prereq in *ChangeLog);; *) rebuild=:;; esac; \
> -     done; \
> -     if $$rebuild; then \
> -       echo $(SHELL) ./config.status $$target; \
> -       cd $(top_builddir) && $(SHELL) ./config.status $$target; \
> -     fi
> +     rm -f address@hidden
> +     sed $(configure_edit) $< > address@hidden

See above.

> +     chmod 555 address@hidden
> +     mv -f address@hidden $@
> +
> +libtool: $(top_builddir)/config.status $(srcdir)/$(auxdir)/ltmain.sh
> +       cd $(top_builddir) && $(SHELL) ./config.status $@
>  
>  .PHONY: configure-subdirs
>  configure-subdirs distdir: $(DIST_MAKEFILE_LIST)
> @@ -552,7 +540,7 @@ $(srcdir)/tests/package.m4: $(srcdir)/configure.ac 
> Makefile.am
>         echo 'm4_define([AT_PACKAGE_STRING],    address@hidden@])'; \
>         echo 'm4_define([AT_PACKAGE_BUGREPORT], address@hidden@])'; \
>         echo 'm4_define([AT_PACKAGE_URL],       address@hidden@])'; \
> -     } | $(edit) > $(srcdir)/tests/package.m4
> +     } | sed $(configure_edit) > $(srcdir)/tests/package.m4
>  
>  tests/atconfig: $(top_builddir)/config.status
>       $(SHELL) ./config.status tests/atconfig
> @@ -896,9 +884,7 @@ DIST_SUBDIRS   += $(CONF_SUBDIRS)
>  # regenerated since the source tree can be read-only.
>  check-recursive: tests/defs
>  tests/defs: $(srcdir)/tests/defs.in
> -     rm -f tests/defs.tmp tests/defs; \
> -     $(edit) $(srcdir)/tests/defs.in > tests/defs.tmp; \
> -     mv -f tests/defs.tmp tests/defs
> +     sed $(configure_edit) $< > $@

See above.


> --- a/libtoolize.m4sh
> +++ b/libtoolize.m4sh
> @@ -70,6 +70,7 @@ AS_INIT[]m4_divert_push([HEADER-COPYRIGHT])
>  : ${TAR=tar}
>  
>  PROGRAM=libtoolize
> address@hidden@
>  
>  m4_divert_pop
>  m4_include([getopt.m4sh])