Re: [PATCH, take 4][cygwin|mingw] Control where win32 DLLs get installed.

[Dave Korn]

Ralf Wildenhues wrote:
> Hi Dave,
> 
> sorry for making you go through another round.

  :-/ That'll teach me to say "unless there's anything else....?"

>> +pathcar="s,^/\([^/]*\).*$,\1,"
>> +pathcdr="s,^/[^/]*,,"
>> +removedotparts="s,/\(\.\(/\|$\)\)\+,/,g"
>> +collapseslashes="s,/\+,/,g"
> 
> \+ is a GNU sed extension, \{1,\} is Posix (two instances).
> Nested \( \) are not portabled sed, neither are alternations \|.

  Aaaaaaaaaaarrrrrrrgghh *headdesk* no alternations?  That's ... just ... it's
not even boolean!  Grrrrrrnashrollfoamgibber  Damn you, AT&T!  Damn you, Sun!
 *shakes fist at crowd of greying bearded sandal-wearing old programmers*  See
you in hell, Unix-boys!  <pulls pin out of a negated character class and
laughs maniacally>

> Anchors $ inside groups are not portable.
> The nesting probably works on most systems we care about, but if we can
> avoid it then I'd prefer to do so.

  Okeydokey, simplified the SED scripts.

>> +        if test x$func_relative_path_tlibdir = x ; then
> 
> Please double-quote $func_relative_path_tlibdir.  The rest of your patch
> seems to cope with with spaces in file name components (even if the rest
> of Libtool may not).

  Woops, yes, that was the intention, missed that one.  One more quick check
over all the quoting won't hurt while I'm at it.

>> +func_restore_path ()
>> +{
>> +  PATH=$save_PATH
> 
> No 'export PATH' here?  (two instances)

  So I reread the autoconf portable shell bit, found the bit about export, now
I know why it needs re-exporting each time it gets changed; will add the
missing directive.

>> +AT_CHECK([$LIBTOOL --mode=execute ./main$EXEEXT], [], [stdout])
> 
> Please use LT_AT_NOINST_EXEC_CHECK here, to avoid error when cross
> compiling.  Similar for the other executions of uninstalled programs.
> (This macro is defined in tests/testsuite.at.)

  Thanks for pointing me at it, can do.

>> +# Ensure libraries can be found on PATH, if we are on one
>> +# of the affected platforms, before testing the shared version.
>> +
>> +func_save_and_prepend_path $curdir/.libs
>> +AT_CHECK([$LIBTOOL --mode=execute ./.libs/main$EXEEXT], [], [stdout])
> 
> eval "`$LIBTOOL --config | grep '^objdir='`"
> func_save_and_prepend_path $curdir/$objdir
> LT_AT_NOINST_EXEC_CHECK(... $objdir/...)

  Ah, thanks, that beats hard-coding ".libs/".

> I'm afraid the direct execution of programs below .libs may not work
> everywhere.  I'd have to check to be sure though.  It's fine with me
> if you run this particular test only on systems of interest to you.

  I think it's easier to just make it install the test executable.

  Also, "gah"!  "Can't run a program because it's in a subdirectory"?  What
kind of hare=brained system is that?  Would it help if I gave "-rpath $objdir"
when linking main as well as for the libs?

>> +#  In fact, prepending the PATH as above is superfluous on the windows
>> +# platforms that this feature is primarily aimed at, as the DLL search
>> +# path always includes the directory from which the app was launched.
>> +# To make sure it still works even when not side-by-side, we'll install
>> +# the main executable and execute it from there while the PATH still
>> +# points to the shared libs in the .libs subdir.  On other platforms,
>> +# the rpaths we set at link time will guarantee it runs from the bindir.
>> +
>> +mkdir $curdir/bin
>> +AT_CHECK([$LIBTOOL --mode=install $lt_INSTALL main$EXEEXT 
>> $curdir/bin/main$EXEEXT], [], [ignore], [ignore])
>> +AT_CHECK([$LIBTOOL --mode=execute $curdir/bin/main$EXEEXT], [], [stdout])
> 
> This one should work without "$LIBTOOL --mode=execute" prepended, no?

  I have no idea.  I just copied the only other things in the testsuite that I
could identify as execution tests.  What does it actually *do*?

> In that case, you could use LT_AT_EXEC_CHECK here, again, to avoid
> spurious failures when cross compile testing.

  Point taken.

> If you care about the fact that an installed program does not
> accidentally require, or search for, a library in an uninstalled
> location, then you can, after the mode=install but before the execution,
> clean up the files in the build tree, or even put a poisoned library
> into the build tree.  I don't think either is necessary to do in this
> test (at least the latter seems overkill, as we do it in other tests
> already), but please decide for yourself.

  Deleting the libs from the build tree seems easiest.

>> +AT_SETUP([bindir install tests])
> 
>> +curdir=`pwd`
>> +for libdir in \
>> +    $curdir/usr/lib/gcc/i686-pc-cygwin/4.5.0 \
>> +    $curdir/usr/lib/gcc/../gcc/.//i686-pc-cygwin/4.5.0/../../././//. \
>> +    $curdir/usr/lib/ \
>> +    $curdir/usr/lib \
>> +    $curdir/baz \
>> +    $curdir/baz/lib/ ;
> 
> Mini nit: this ";" is not necessary.  :-)

  It's still valid, isn't it?  I don't like to make my syntax in a way that
only works because of line breaks.

>> +  AT_CHECK([$LIBTOOL --mode=link --tag=CC $CC -no-undefined -shared -o 
>> libfoo.la $CPPFLAGS $CFLAGS $LDFLAGS foo.lo bar.lo baz.lo -rpath 
>> $libdir],[0],[ignore],[ignore])
> 
> Please drop -shared, it should be unnecessary.

  Ok, will do.

>> +  AT_CHECK($bindirneeded && ( test -f $libdir/../bin/???foo-0.dll || test 
>> -f $libdir/../bin/libfoo.so.0 ) || test -f $libdir/libfoo.so.0)
> 
> Please M4-quote "[...]" the argument to AT_CHECK.

  Ok.

> You can replace the inner subshell "( ... )" with "{ ... ; }" to avoid a
> subshell.

  Thanks.

> Out of curiosity, which of the systems of interest creates a
> libfoo.so.0 file?  

  Linux.

> What if that is a symlink rather than a plain file
> ("test -f" only tests for plain files)?

  It is.  The test still passes.  Hmmmm..... better take a closer look at
that.  (And use the full so.0.0.0 name in the test).

>> +    # Clear any old stuff out before we install.  Because bindir
>> +    # may be in /tmp, we have to take care to create it securely
>> +    # and not to delete and recreate it if we do.
>> +    rm -rf $libdir $curdir/bin $curdir/sbin $curdir/baz $curdir/usr 
>> 2>/dev/null
> 
> I'd drop the 2>/dev/null; let's see errors in the log.

  It makes a load of ugly noise when it tries to delete
"$curdir/usr/lib/gcc/../gcc/.//i686-pc-cygwin/4.5.0/../../././//.", but I
guess it's better that than missing something important.

>> +    if test ! -z "$tmp" ; then
>> +      rm -rf "$tmp"
>> +    fi
> 
> For extra credit you could also throw in
>       $LIBTOOL --mode=uninstall rm -f $libdir/libfoo.la 
> $curdir/sbin/main$EXEEXT
>       $LIBTOOL --mode=clean rm -f libfoo.la main$EXEEXT
> 
> here, if you like.

  I think I'll opt out.  I'm not trying to test that functionality here after 
all!

  Anyway, I've done the general.m4sh changes and am working on the test case
fixes.  Respin coming soon.

    cheers,
      DaveK