Hi Jacob, Thanks for your review!
* Jacob Meuser wrote on Sun, Jan 22, 2006 at 04:56:59AM CET: > On Sat, Jan 21, 2006 at 09:37:27AM +0100, Ralf Wildenhues wrote: > > > > The changes applied to branch-1-5 had two flaws (at least): > > a) They did not move all paths to uninstalled libs up front. > > b) They removed some uninstalled libs in some cases. > > c) the main flaw, not all deplibs paths are absolute, but the > notinst_path paths are. Well, yes, there's a certain point to this. libtool's path handling is a bit deficient (see e.g. the XFAIL test deplibs-ident.at in CVS libtool). OTOH, using absolute paths throughout is troublesome at times as well: on w32 systems, they don't start with a slash, often contain spaces whereas relative paths don't (there is a TODO item to deal better with spaces in paths in libtool). Paths specified with -L need not actually exist (yet), so we can't necessarily find out the absolute path. > > - notinst_path="$notinst_path $abs_ladir" > > + notinst_path="$notinst_path $abs_ladir $dir" > > fi > > fi # $installed = yes > > func_stripname 'lib' '.la' "$laname" > > is it guaranteed that this will always add the path exactly as it will > look later, when it is processed by the following? *snip* If you know about a case where it's not, then I'd like to know about it, to add it to the test for exposure. > IIRC, notinst_path can also contain paths where objects reside, so > starting with notinst_path could add paths that shouldn't be there. Hmm, my patch won't ever actually _add_ a notinst_path to the output line if it wasn't before. Maybe I misunderstood this? > below is a revision of the original patch I made for this issue. > it is against 1.5.22. > > the main differences from your patch are: > a) expand all paths to the absolute instead of relying on them to > have already been expanded > b) filter against notinst_path instead of starting with notinst_path Thank you for the patch, I will test it. Two small issues I see with it: - It will fail if the user passes a relative path to a non-existing directory. - It is a bit slower than my patch, as it will expand paths more than once. Likely that is necessary anyway for better accuracy -- I just mention it for completeness. > however, the shell parameter substitution used here is probably not > portable by libtool standards. No, but that's trivial to change (or factor in a shell function and choose appropriately for both more and less capable shells, as we do in CVS HEAD). By the way, I have been struggling with CVS HEAD libtool quite a bit on OpenBSD wrt the other build tool ports: it only worked with gm4, plus self-compiled autoconf and automake, and gmake. Even with the latter, I see spurious rebuilds. Don't take this as a bug report or anything -- most likely the issues are to be found in libtool rather than anywhere else, I'm merely noting. Haven't had time to analyze this yet. Cheers, Ralf
