Re: support standalone libltdl [libtool--gary--1.0--patch-23]

[Ralf Wildenhues]

Hi Gary,

* Gary V. Vaughan wrote on Fri, Aug 19, 2005 at 07:38:31PM CEST:
> Ralf Wildenhues wrote:
> >* Gary V. Vaughan wrote on Tue, Apr 26, 2005 at 03:13:17PM CEST:
> 
> Thanks for the review... I've had a busy summer, so I hope you'll 
> forgive that it has taken me 3 months to respond :-(

Hehe, let's just hope your next response will take less long.

> Attached patch is taken against HEAD.

OK, good.

*snip some unproblematic stuff*
> >`make clean' fails to remove libltdl/lt__strl.lo, but it removes
> >libltdl/.libs/lt__strl.o, causing build failure.
> 
> This is a bug in automake-1.9.x with subdir objects.  As a workaround 
> I've added $(LIBOBJS) & $(LTLIBOBJS) to CLEANFILES.

OK, good.

> >`make uninstall' fails to remove anything below $prefix/share/libtool
> >and below include/libltdl.
> 
> Oops.  Forgot to match uninstall-hook against install-hook and 
> install-data-local.  Now fixed.

Hmm.  Comments inline, see attached.

*snip*
> >Your libtoolize tests all fail (1, 2, 3) because you changed `copying'
> >to `linking' (why not say `linking' only if that is what you do, i.e.
> >without `--copy'?)
> 
> They failed because of path changes.  Now fixed. (`linking' is only for 
> "ln -s" -- also "libtoolize -v" gives source path instead of dest path).

OK.

> >Somehow the test suite was not automagically updated to include
> >`standalone'.  I believe this might have been my fault.
> 
> Can't reproduce.

OK.  Forget about it then.

> >There exists $top_builddir/.deps with files "argz.Plo lt__dirent.Plo
> >lt__strl.Plo" now.  I don't know whether this is a bug (believe not).
> 
> Side effect of subdir-objects.

OK.

> >The files
> >  libltdl/Makefile{,.am,.in}
> >          configure{,.ac}
> >          aclocal.m4
> >          README
> >          config-h.in
> >          m4/lt~obsolete.m4
> >
> >are not shipped in libtool-2.1.tar.gz, and a subsequent
> >  configure
> >  make
> >from the tarball does not create any of them (it should create Makefile,
> >and everything else should be shipped, right?).
> 
> Yep.  All good. These files are all shipped correctly for me -- maybe 
> because of the other fixes I made on the way here?

Maybe.

> >I don't really like config and m4 below libltdl.  It seems ugly.
> >But that is a minor issue.
> 
> Really?  From libltdl's point of view, it owns config and m4 -- we 
> install copies of those directories to $prefix/share/libtool, and 
> libtoolize --ltdl puts them inside the libltdl it installs.  Now that 
> I've moved them here, having them in $top_srcdir (where they are only 
> used during bootstrap) seems ugly to me.

OK, functionality is more important than looks.
But to tell you the truth, the move of libtool.m4 from top to m4/,
combined with the limitations of cvs/cvsweb, have cost me hours of
searching time already (while trying to hunt down why a particular
change was made).

When you eventually commit the reorganization, I will definitely want
this in two steps: 1) move files only, 2) change files.
I know this sucks, and is fault of CVS, but as long as that's what
savannah offers, there is no other sane way.

Ah, and also libltdl/Makefile.am needs to be removed from CVS if it's to
be generated.  (It's good to see its diff now, though, made me find some
bugs in your patch.)

> >I stopped further testing after this.  It would be nice if your
> >standalone tests could run a `make distcheck' so we can be reasonably
> >sure `libltdl/Makefile.am' has everything it needs.  Once for
> >AC_LIBLTDL_CONVENIENCE and once for AC_LIBLTDL_INSTALLABLE.
> 
> The c++ template with subdir-objects autotest is failing for me right 
> now (I think this is because I need to backport another patch to my 
> local automake-1.9.6 installation -- any clue as to which one?), so I'd 
> like to address this in a separate patch once we have this one in HEAD 
> and branch-2-0...

Erm, tests/template.at does not use Automake.
Please run with -d -v, post testsuite output, to see what's happening.

> >It would also be nice (while I'm at the point of using old macro names)
> >to test whether using the old names works.  That is exactly what all
> >upgraders will hit first when going to 2.0.
> 
> ACK.
> 
> >As I am a fan of "one change per patch", I'd have liked to have the
> >- use of auxdir, m4dir
> >- move files around
> >- update all dependents, simplify libtoolize
> >- change `copying' to `linking'
> >- build libltdl from $top_builddir/Makefile
> >
> >as separate patches,
> 
> Me too.  This started out as what I thought was going to be a relatively 
> simple 'standalone libltdl' patch, that grew big before I thought to 
> split it up.  Sorry about that.

Peter Ekberg was much easier to convince, even after posting his monster
patch.  ;-)

> >they would have been easier to verify (the first
> >two separated only because of CVS limitations -- it's almost impossible
> >to trace back using `cvs annotate' if you change a file while moving at
> >the same time).
> 
> Gah!  I have been spoiled by arch.  Sorry again.

See above.

> >By the way, have you ever tried `make installcheck' with the new
> >testsuite?
> 
> Not in libtool, though I've used it often with m4's autotest suite.

Hmm.  That does not exercise libtoolize much.

> >How do you expect a backport to be able to work when bootstrapped with
> >released autoconf/automake?  IOW: what exactly do you want to backport?
> 
> If you are happy with the attached:  it'll be easier for me to show you 
> the backport patch.

No need to start working on that yet,  this patch has several issues
that make it unsuitable for application.

Also, I have not tested this patch yet, merely read it.  My last review
took several hours, I will only invest this sort of time with a patch
that has a chance of being good.

Cheers,
Ralf

libtool--gary--1.0--patch-23--annot
Description: Text document