Re: [PATCH 5/6] Don't leak developer GREP, SED etc into distribution file.

[Gary V. Vaughan]

On 1 Sep 2010, at 01:12, Ralf Wildenhues wrote:
> * Gary V. Vaughan wrote on Tue, Aug 31, 2010 at 08:43:19AM CEST:
>> From: Gary V. Vaughan <address@hidden>
>> 
>> * bootstrap (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 substitution that should not happen until
>> configure time.
>> * bootstrap: Move the bootstrap related section up towards the top
>> of the file.
>> (libltdl/m4/ltversion.m4): Extract `$macro_revision' from this
>> file for direct comparison with `$revision', rather than munging
>> the file's serial number.  Use `bootstrap_edit' for substitutions
>> when generating from `ltversion.in'.
>> (libltdl/config/ltmain.sh): Similarly, extract `$package_revision'
>> for comparison with rebuild's `$revision' setting, and make
>> bootstrap time substitutions with `bootstrap_edit'.
>> (libtool): Likewise.
>> (libtoolize): Use `configure_edit' for substitutions at configure
>> time.
>> (tests/package.m4, tests/defs): 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.
> 
> Please credit Joerg Sonnenberger for finding the bug.  Thanks.

Will do.

> I usually do VPATH builds exclusively, except when trying out patches
> where I think in-tree builds could be broken.  In my VPATH tree, when I
>  make -f ../libtool/Makefile.main announce-gen SHELL='/bin/sh -x'
> 
> that contains the following output:
> 
> + -e 's,@TIMESTAMP\@, 1.3257 2010-08-30,g' -e 's,@LASTRELEASE\@,2.2.11,g' -e 
> 's,@top_srcdir\@,../libtool,g' announce-gen.in
> /bin/sh: line 4: -e: command not found

As I mentioned already, it seems that I was overly optimistic about
being able to cherry pick the non-gnulib related parts of my use-gnulib
topic brach onto master and expect them to work :(

The error above comes from Makefile.maint, which doesn't exist in use-gnulib,
superceded by maint.mk/cfg.mk.  Sorry for the bad merge.

> Please, once and for all, whenever you rename some identifier,
> use some method to verify that you've renamed all instances;
>  git grep '\<edit\>'
> 
> is quite a good approximation to the perfect method, and costs maybe a
> couple of seconds on your part.  Then submit that as separate patch.

As I did when writing, testing and committing within the series I
developed in.

> When you do pure code move-arounds, that also is much easier to review
> when done as a separate patch, without any actual changes to the text
> that was moved.  I remember that we took more than dozen iterations on
> the makefile rules to get them halfway correct, I have no intentions of
> going there again.

Okay.

> I have stopped reviewing at this point.  I would gladly re-review a
> minimal patch based on minimal prerequisites that has been properly
> tested.  The Makefile.am changes should be tested with both GNU and
> non-GNU make, and both VPATH and in-tree.

As I did in the context of the use-gnulib branch.

On balance, there is an inordinate amount of time and effort involved
from each of us to fully test and review these patches out of context
(i.e. on master) when half the work has already been done once on
another branch (i.e use-gnulib).  Let's just forget this patch series
for now.  Thanks for the review, and sorry for not making the time
to retest everything all over again as I cherry-picked.  I'll take the
pertinent parts into consideration before resubmitting the use-gnulib
branch series in its entirety after the release.

Cheers,
-- 
Gary V. Vaughan (address@hidden)

PGP.sig
Description: This is a digitally signed message part