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

[Ralf Wildenhues]

Hello Gary,

* Gary V. Vaughan wrote on Thu, Sep 23, 2010 at 05:21:19PM CEST:
> I've pared this down to the absolute minimum necessary to fix the
> bug it is targetting.  I did run this through `make distcheck' (and
> it passed), but most of the others following only got a cursory
> `git clean -x -d -f && ./bootstrap && ./configure && ./make all
> check TESTS=tests/sh.test TESTSUITEFLAGS=1' unless noted otherwise.

Did you look at the output of the cursory check?  Like, after this
patch, I get this in the output:

  ## ---------------------------- ##
  ## @PACKAGE_STRING@ test suite. ##
  ## ---------------------------- ##

See below for an explanation.

> > 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.
> 
> Oh!  I'd forgotten that AC_INIT_COMMANDS don't get run through the
> AC_SUBST machinery :(

Unfortunately, I cannot parse this statement.  What does
AC_INIT_COMMANDS have to do with these lines which come from
general.m4sh originally?

> I'd like to tackle that separately.  Unfortunately those settings
> come from general.m4sh boilerplate and are required by libtoolize, so
> it's not entirely straight forward.

Agreed.

> I'm torn between running our
> own minimal substitution to fill the values in from libtool.m4, or
> having the make rule for libtool just edit them out.  Thoughts?

Editing them out is fine.  Again, fixing this later is fine with me,
too.

> Okay to push this one now?

It is still not quite correct.  See below.

> * Makefile.am: Having rearranged the file, now apply the actual
> changes to follow-up.
> (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.
> * Makefile.am (libltdl/m4/ltversion.m4, libltdl/config/ltmain.sh)
> (libtoolize.in): Use bootstrap_edit.

This log line is not reflected in the patch contents.  And in fact, with
the patch applied, the generated libtoolize script still has
untransformed strings:

$ head libtoolize | sed 's/^/| '
| #! /bin/sh
| # Generated from libtoolize.m4sh by GNU Autoconf 2.68.
| 
| # libtoolize (GNU @PACKAGE@@TIMESTAMP@) @VERSION@
[...]

> (libtoolize, tests/package.m4): Use configure_edit.

For tests/package.m4, this is the wrong thing to do; see below.

> * 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
> @@ -75,26 +75,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  = sed \
> +               -e 's,@MACRO_VERSION\@,$(VERSION),g' \
> +               -e "s,@MACRO_REVISION\@,$$correctver,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\@,$$correctver,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
> @@ -121,11 +118,9 @@ $(srcdir)/$(m4dir)/ltversion.m4: $(m4dir)/ltversion.in 
> configure.ac ChangeLog
>         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" \
> +       echo $(bootstrap_edit) \
>           $(srcdir)/$(m4dir)/ltversion.in \> $(srcdir)/$(m4dir)/ltversion.m4; 
> \
> -       $(edit) -e "s,@MACRO_REVISION\@,$$correctver,g" \
> -               -e "s,@MACRO_SERIAL\@,$$serial,g" \
> +       $(bootstrap_edit) \
>                 $(m4dir)/ltversion.in > $(m4dir)/ltversion.tmp; \
>         chmod a-w $(m4dir)/ltversion.tmp; \
>         mv -f $(m4dir)/ltversion.tmp $(m4dir)/ltversion.m4; \
> @@ -160,11 +155,9 @@ $(srcdir)/$(auxdir)/ltmain.sh: $(sh_files) 
> $(auxdir)/ltmain.m4sh configure.ac Ch
>           \> $(auxdir)/ltmain.in; \
>         $(M4SH) -B $(auxdir) $(auxdir)/ltmain.m4sh \
>           > $(auxdir)/ltmain.in; \
> -       echo $(edit) -e "s,@TIMESTAMP\@,$$TIMESTAMP,g" \
> -         -e "s,@package_revision\@,$$correctver," \
> +       echo $(bootstrap_edit) \
>           $(srcdir)/$(auxdir)/ltmain.in "> $$target"; \
> -       $(edit) -e "s,@TIMESTAMP\@,$$TIMESTAMP,g" \
> -             -e "s,@package_revision\@,$$1,g" \
> +       $(bootstrap_edit) \
>               $(auxdir)/ltmain.in > $(auxdir)/ltmain.tmp; \
>         rm -f $(auxdir)/ltmain.in; \
>         chmod a-w $(auxdir)/ltmain.tmp; \
> @@ -220,16 +213,27 @@ all-local: $(LTDL_BOOTSTRAP_DEPS)
>  ## Libtool scripts. ##
>  ## ---------------- ##
>  
> +configure_edit = sed \
> +     -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" \
> +     $(configure_edit) \
>               $(srcdir)/libtoolize.in > libtoolize.tmp

>       chmod a+x libtoolize.tmp
>       chmod a-w libtoolize.tmp
> @@ -552,7 +556,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
> +     } | $(configure_edit) > $(srcdir)/tests/package.m4

This isn't right.  package.m4 will still contain un-substituted strings,
which causes the output I posted at the very beginning of this message.
Did you mean bootstrap_edit here?  Now, maybe you after all meant to
move this section elsewhere in 1/11 then too?

>  tests/atconfig: $(top_builddir)/config.status
>       $(SHELL) ./config.status tests/atconfig
> @@ -897,7 +901,7 @@ DIST_SUBDIRS   += $(CONF_SUBDIRS)
>  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; \
> +     $(configure_edit) $(srcdir)/tests/defs.in > tests/defs.tmp; \
>       mv -f tests/defs.tmp tests/defs
>  
>  # Use `$(srcdir)/tests' for the benefit of non-GNU makes: this is

Please post a fixed version of 1/11 and 2/11 (see above for why you may
want to update 1/11 once more) and I will re-review them.

I will also review the rest of the patch series at that point, but I
cannot do this now, because bugs in this patch are hard to distinguish
from followup bugs.  So please consider the 72 hour clock reset at the
time when you have posted fixed versions of 1/11 and 2/11.

Thanks,
Ralf