[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: support standalone libltdl [libtool--gary--1.0--patch-23]
From: |
Ralf Wildenhues |
Subject: |
Re: support standalone libltdl [libtool--gary--1.0--patch-23] |
Date: |
Sun, 21 Aug 2005 09:21:23 +0200 |
User-agent: |
Mutt/1.5.9i |
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