Re: 319-gary-refactor-m4sh-rules

[Ralf Wildenhues]

Hello Gary,

* Gary V. Vaughan wrote on Wed, Mar 07, 2007 at 09:31:58AM CET:
> On 6 Mar 2007, at 14:23, Ralf Wildenhues wrote:
> >* Gary V. Vaughan wrote on Tue, Mar 06, 2007 at 08:33:50PM CET:
> >>This makes code sharing with forthcoming func_version test cases  
> >>easier.
> >>
> >>Okay to commit?
> >
> >IMHO this is definitely post 2.0: it doesn't fix an important bug.
> 
> It is not touching much code at all though, and without doing it this
> way, I'll need to copy and paste the m4sh -> in -> sh rules into the  
> test case, which will mean manually keeping it all in synch, and that
> the  test itself won't actually exercise the live code.

> (Not to mention that without the patch we already have to manually
> synch several copies of the same code in the existing Makefile.am
> implementation).

Yes, certainly.  It still does not fulfill the "fix only important bugs
and regressions" criteria.  It simply doesn't.  If you insist, then it's
simply easier to not apply your func_version changes at all: they don't
either fix an _important_ bug.  I can certainly live well with that one
single long copyright line in ltmain.m4sh, which I'd not even qualify as
an unimportant bug, it's at most a negligible annoyance.  And if I were
to fix it, then I would make func_version more stupid by just letting it
output a constant string, and have a copy of that in each script.  I'll
anyway have to grep the source tree for several instances of '2007' next
January, the func_version automatization doesn't even avoid all that
much work for me.  But let's not discuss the bikeshed's color.

I would like for 2.1b to be released when the libltdl memory fault on
Cygwin is fixed and I've managed to put Charles' documentation patch
into a somewhat more general form.  And NEWS documents very clearly all
other regressions that we should be caring about.  If you want to add
one, then let's discuss that.  And if you have a test failure on some
system and see an easy fix, then I'm all ears, too.

But other than that, I'm simply going to have a blanket "No", and it's
going to get more capitalized with time.  Let's release with all the
suboptimal and ugly and duplicated code that we have.  We've had more
than three years now where we were too lazy to fix it or not to
introduce it in the first place.  So let's live with it now.  If this
keeps going on for long, I'm going to invest less effort in saying no,
too: patch reviews take time, too, time that also pushes a release
further away.  This one for one has taken way too much of my time
already.

If the Libtool developers disagree with me here on my stance, then
by all means speak up, please.  I have postponed patches including
thorough test cases that actually have been extensively exercised, but
do not fit the above criteria.  I'd love to see these applied, but
refrain from it.  I'm also open to reconsider my Libtool commitment,
should there be irresolvable differences.

> >Moreover it's wrong, too: the $(srcdir) are there for a reason.
> >Sorry.
> 
> It was my understanding that the $(srcdir) are required on the left
> of the ':' in a rule, but not on the right.

If you mention $(srcdir)/file as a prerequisite somewhere, then you also
have to mention it as the target.  We mentioned it with $(srcdir)/ on
the right in order to avoid surprise by Solaris make's VPATH rewriting
rules in the rule commands, IIRC.

> From Autoconf manual 11.13.5:
> 
>   It seems the sole solution that would please every `make'
>   implementation is to never rely on `VPATH' searches for targets.  In
>   other words, `VPATH' should be reserved to unbuilt sources.

Yes.  I think that supports my point.

> If I'm correct, then I've just removed a little unnecessary fluff
> from the right hand sides.  If I'm wrong, then there was already
> enough inconsistency that things would have already been broken by
> missing $(srcdir) on the right in other rules.  In either case, I'd
> still like to commit the factoring parts of the patch.

No.  Please no more factoring.  The last factoring changes cost months
if not years to get bug free.  Please pick one of the regressions if you
want to fix something.  If you rather want to fix test failures, please
say so, and I will post a couple of open ones.

> >If you're out to relax the requirement for GNU make, then I suggest
> >you give some information as to which make implementations you have
> >successfully tested with.
> 
> Nope.  Although now you mention it, I believe we're already requiring
> GNU make for VPATH builds, no?

Yes.  At the moment we do.  But we want to get away from it, users
obviously don't like the additional requirement.  So please don't make
it harder to do so in the future, by relying even more on GNU make.

> In which case, we can remove the
> $(srcdir) fluff and repeated associated comments from the left side
> of ':' in several rules.  The first non-GNU system I tried (on irix,
> which has a SYSV4 based make) with a fresh HEAD checkout (bootstrapped
> with automake-1.10, autoconf-2.60) blows up pretty fast from a VPATH
> build, failing to find a source file:

Yeah, I don't think we can get VPATH builds to work for IRIX make,
neither for FreeBSD make, or Solaris 2.6 make.  Most others should be
possible, though.  One important point however would be to get things to
work for a non-GNU make in the non-VPATH case.  That's what counts for a
casual user.  All I'm interested here is avoiding that each invocation
of non-GNU make rerun aclocal, autoconf, and `./config.status --recheck'
all the time, due to the stamp-vcl rules that use GNU make specific
features.

Apart from that, and to prove my point that changes introduce bugs,
here's a bit more of a review of your patch:

>   @@ -121,6 +120,27 @@
>         *) TIMESTAMP="" ;; \
>       esac
>    
>   +# Where a .in file must be distributed (since the installer may not have
>   +# autom4te installed), the rule below will generate it:
>   +SUFFIXES = .m4sh .in .sh
>   +.m4sh.in: $(sh_files) Makefile.am
>   +   rm -f "$@"
>   +   $(M4SH) -B $(srcdir)/$(auxdir) "$<" > "$@"

Double quotes around $< and $@ are useless, as make can't cope with
whitespace in those file names anyway (several instances in the patch),
so please remove them.  (Besides, even if they were of use, then not
quoting $(srcdir)/$(auxdir) here would be inconsistent.)

>   +
>   +# Otherwise, many of the M4SH generated files are distributed with the
>   +# substitutions already performed, in which case the rule below generates
>   +# that distributed file directly from the source:
>   +.m4sh.sh: $(sh_files) Makefile.am

Inference rules that specify prerequisites are not allowed by POSIX.
You have to specify the prerequisites separately.  (FWIW, according
to the Autoconf manual that works well only for double suffix rules,
which the above is.)

>   Index: Makefile.maint
>   ===================================================================
>   RCS file: /sources/libtool/libtool/Makefile.maint,v
>   retrieving revision 1.14
>   diff -u -u -r1.14 Makefile.maint
>   --- Makefile.maint 26 Feb 2007 07:44:23 -0000 1.14
>   +++ Makefile.maint 6 Mar 2007 19:31:29 -0000
[...]
>   +$(srcdir)/commit: $(auxdir)/mailnotify clcommit.tmp
>   +   mv -f clcommit.tmp $(srcdir)/commit;

superfluous ; at end.

>   +   rm -f clcommit.in
>   +
>   +$(srcdir)/$(auxdir)/mailnotify: $(auxdir)/mailnotify.tmp
>   +   mv -f mailnotify.tmp $(srcdir)/$(auxdir)/mailnotify; \

No need for `; \' at end; that way you also catch an error here.

>       rm -f mailnotify.in
>    
>    .PHONY: cvs-release
>   Index: bootstrap
>   ===================================================================
>   RCS file: /sources/libtool/libtool/bootstrap,v
>   retrieving revision 1.80
>   diff -u -u -r1.80 bootstrap
>   --- bootstrap 26 Feb 2007 07:44:23 -0000 1.80
>   +++ bootstrap 6 Mar 2007 19:31:29 -0000
>   @@ -119,7 +119,7 @@
>    # Whip up a dirty Makefile:
>    makes='Makefile.am libltdl/Makefile.inc'
>    rm -f Makefile
>   -$SED '/^if /,/^endif$/d;/^else$/,/^endif$/d;/^include /d' $makes > Makefile
>   +$SED -e '/^if /,/^endif$/d;/^else$/,/^endif$/d;/^include /d' $makes > 
> Makefile

This change is neither necessary nor listed in the ChangeLog entry.
Just drop it.

Cheers,
Ralf