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

[Ralf Wildenhues]

Hello Dave,

* Dave Korn wrote on Tue, Aug 11, 2009 at 09:07:12AM CEST:
>
>   Well, the bindir option exists only to support PE DLLs,

Bzzt.  First error.  If libtool provides -bindir, then it should accept
it on every system, so writing portable makefiles is made easy; and your
test addition should ensure that it does so.  Of course on non-PE
systems, it should just ignore the option silently.  libtool aims to
provide a uniform interface to users.

So it's only the PE-specific bits of the test that should be skipped on
systems where they don't apply.

Thus, bindir.at is a sensible name.  Do you intend for Automake to pass
-bindir to libtool --mode=link invocations automatically (maybe for
<foo>_LTLIBRARIES with <foo> not equal to lib or libexec)?

* Dave Korn wrote on Tue, Aug 11, 2009 at 10:35:28AM CEST:
>   All rebuilt OK.  Checked docs with "make dvi info pdf" and viewing the
> generated file.  Testsuite re-run is still in progress, but I already checked
> that the new tests all still pass, so unless I post back saying otherwise in 
> the
> next couple of hours, assume the lot completed without regressions.

Tested on what system(s)?

* Dave Korn wrote on Tue, Aug 11, 2009 at 12:30:47PM CEST:
> 
> libtool/ChangeLog:
> 
>       * Makefile.am (TESTSUITE_AT): Add pe-dll-inst-bindir.at.
>       * libltdl/config/general.m4sh (func_relative_path): New function.
>       * libltdl/config/ltmain.m4sh (func_mode_link): Accept new -bindir
>       option and use it, if supplied, to place Windows DLLs.
>       * tests/pe-dll-inst-bindir.at: New file for DLL install tests.
>       * doc/libtool.texi (Link Mode): Update documentation.

BTW, even if the part going into GCC is covered by your GCC assignment,
the rest is still sufficiently nontrivial to warrant an assignment.

> diff --git a/Makefile.am b/Makefile.am
> old mode 100644
> new mode 100755

Your files suffer from mode changes.  They are of course not acceptable,
though I understand they are a w32 artifact.  Can git be made to ignore
them for you?

> index a18955e..fdeeffd
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -494,7 +494,8 @@ TESTSUITE_AT      = tests/testsuite.at \
>                 tests/configure-iface.at \
>                 tests/stresstest.at \
>                 tests/cmdline_wrap.at \
> -               tests/darwin.at
> +               tests/darwin.at \
> +               tests/pe-dll-inst-bindir.at

tests/bindir.at, as already noted above; and as this is about
'libtool' (the script) API, please sort it alongside the other API
tests, preferably before or after cwrapper.at.  Thanks.

> --- a/doc/libtool.texi
> +++ b/doc/libtool.texi
> @@ -1376,6 +1376,15 @@ Tries to avoid versioning (@pxref{Versioning}) for 
> libraries and modules,
>  i.e.@: no version information is stored and no symbolic links are created.
>  If the platform requires versioning, this option has no effect.
>  
> address@hidden -bindir
> +When linking a DLL for Windows or another PE platform, this option tells

What does this have to do with PE?  All this is about is that there is
no real, independent $shlibpath_var beside PATH.  I'm OK with mentioning
that Windows is the sole current user of this, but please let's word
this in a way that doesn't require us to change the interface if some
other system requires it, too.  Ideally, neither the text.

> +libtool where to eventually install the @samp{.dll} file. The output path
> +is used to install the @samp{.la} control file, usually into a 
> @samp{.../lib/}
> +subdirectory of the @var{prefix}; except in the case of a dlopen()-able
> +module (@pxref{Modules for libltdl}), it is usually desirable to install the
> +DLL into a @samp{.../bin/} directory alongside. This option specifies the
> +absolute path to the @var{bindir}.

> @@ -1460,7 +1469,10 @@ the @option{-version-info} flag instead 
> (@pxref{Versioning}).
>  @item -rpath @var{libdir}
>  If @var{output-file} is a library, it will eventually be installed in
>  @var{libdir}.  If @var{output-file} is a program, add @var{libdir} to
> -the run-time path of the program.
> +the run-time path of the program.  If @var{output-file} is a Windows
> +(or other PE platform) DLL, the @samp{.la} control file will be
> +installed in @var{libdir}, but see @option{-bindir} above for the
> +eventual destination of the @samp{.dll} file itself.
>  
>  @item -R @var{libdir}
>  If @var{output-file} is a program, add @var{libdir} to its run-time

> --- a/libltdl/config/general.m4sh
> +++ b/libltdl/config/general.m4sh
> @@ -100,6 +100,76 @@ func_dirname_and_basename ()
>  
>  # Generated shell functions inserted here.
>  
> +# func_relative_path libdir bindir
> +# generates a relative path from LIBDIR to BINDIR, intended
> +# for supporting installation of windows DLLs into -bindir.

gnulib-tool has a function func_relativize.  Can we just reuse that
(and make it fast)?  Failing that, did you write this function from
scratch or took it from anywhere else.

I don't see any reason this function should be written specific to
libdir and bindir.  There are other file names that may usefully be
relativized, so the function should be as general as possible, and use
generic names, too.

> +# call:
> +#   func_dirname:
> +#             func_dirname file append nondir_replacement

Why do you repeat the descriptions of these functions here again?
That's not normally done elsewhere in the code (and anyway leads to
unmaintainable text duplication).  Oh wait, it's done with
func_dirname_and_basename, but only because the sole reason for that
function is to wrap to other ones.

We also need to eventually give up the local variable name uglification
in favor of something more readable.  This is already quite mad (but of
course not your fault, nor reason to fix in this patch).

> +func_relative_path ()
> +{
> +  func_relative_path_result=
> +  func_stripname '' '/' "$1"
> +  func_relative_path_tlibdir=$func_stripname_result
> +  func_stripname '' '/' "$2"
> +  func_relative_path_tbindir=$func_stripname_result
> +
> +  # Ascend the tree starting from libdir
> +  while :; do
> +    # check if we have found a prefix of bindir
> +    case "$func_relative_path_tbindir" in
> +      $func_relative_path_tlibdir*) # found a matching prefix
> +     func_stripname "$func_relative_path_tlibdir" '' 
> "$func_relative_path_tbindir"
> +     func_relative_path_tcancelled=$func_stripname_result
> +     if test -z "$func_relative_path_result"; then
> +       func_relative_path_result=.
> +     fi
> +     break
> +     ;;
> +      *)
> +     func_dirname $func_relative_path_tlibdir
> +     func_relative_path_tlibdir=${func_dirname_result}
> +     if test x$func_relative_path_tlibdir = x ; then
> +       # Have to descend all the way to the root!
> +       func_relative_path_result=../$func_relative_path_result
> +       func_relative_path_tcancelled="$func_relative_path_tbindir"
> +       break
> +     fi
> +     func_relative_path_result=../$func_relative_path_result
> +     ;;
> +    esac
> +  done
> +
> +  # Now calculate path; take care to avoid doubling-up slashes.
> +  func_stripname '' '/' "$func_relative_path_result"
> +  func_relative_path_result=$func_stripname_result
> +  func_stripname '' '/' "$func_relative_path_tcancelled"
> +  
> func_relative_path_result=${func_relative_path_result}${func_stripname_result}
> +
> +  # Normalisation. If bindir is libdir, return empty string,
> +  # if subdir return string beginning './', else relative path
> +  # ending with a slash; either way, target file name can be
> +  # directly appended.
> +  if test -z "$func_relative_path_result"; then
> +    func_relative_path_result=./
> +  else
> +    func_stripname './' '' "$func_relative_path_result/"
> +    func_relative_path_result=$func_stripname_result


> --- a/libltdl/config/ltmain.m4sh
> +++ b/libltdl/config/ltmain.m4sh
> @@ -1129,6 +1129,8 @@ The following components of LINK-COMMAND are treated 
> specially:
>  
>    -all-static       do not do any dynamic linking at all
>    -avoid-version    do not add a version suffix if possible
> +  -bindir BINDIR    specify path to $prefix/bin (needed only when installing
> +                    a Windows DLL)

Again, please don't give the impression that users should pass this for
w32 only.

>    -dlopen FILE      \`-dlpreopen' FILE if it cannot be dlopened at runtime
>    -dlpreopen FILE   link in FILE and add its symbols to lt_preloaded_symbols
>    -export-dynamic   allow symbols from OUTPUT-FILE to be resolved with 
> dlsym(3)

> --- /dev/null
> +++ b/tests/pe-dll-inst-bindir.at

> +noskip=:
> +case "$host_os" in
> +cygwin*|mingw*|cegcc*) ;;
> +*) noskip=false ;;
> +esac
> +
> +$noskip && {

You cannot wrap AT_SETUP/AT_CLEANUP blocks in shell conditionals.  It
will mess up Autotest (the -l output, running specific tests only etc).
Just consider any code outside of these blocks going to /dev/null.

> +AT_BANNER([PE DLL install tests])

Please drop the banner, that's only really helpful for big sets of tests.

> +AT_SETUP([simple compile check])

I'd just compactify this into one test group; or name the groups
  bindir compile, bindir basic, bindir install

or so.

> +# Verify compiling works.
> +
> +AT_DATA([simple.c] ,[[
> +int main() { return 0;}
> +]])
> +
> +$noskip && {
> +$CC $CPPFLAGS $CFLAGS -o simple simple.c 2>&1 > /dev/null || noskip=false
> +rm -f simple 

Please avoid trailing white space.

> +}
> +
> +AT_CHECK([$noskip || (exit 77)])
> +
> +AT_CLEANUP
> +
> +# Now the tests themselves.
> +
> +AT_SETUP([dll basic test])
> +
> +AT_DATA([foo.c],[[
> +int x=0;
> +]])
> +
> +AT_DATA([baz.c],[[
> +int y=0;
> +]])
> +
> +AT_DATA([bar.c],[[
> +extern int x;
> +int bar(void);
> +int bar() { return x;}
> +]])
> +
> +AT_DATA([main.c],[[
> +extern int x;
> +extern int y;
> +
> +int main() {
> +return x+y;
> +}
> +]])

Your main doesn't need anything from libbar, and your libbar doesn't
need anything from libfoo.  Why have the libraries in that case?

Likewise in the test below.

> +AT_CHECK([$LIBTOOL --mode=compile --tag=CC $CC -c -o foo.lo $CPPFLAGS 
> $CFLAGS foo.c],[0],[ignore],[ignore])
> +
> +AT_CHECK([$LIBTOOL --mode=compile --tag=CC $CC -c -o baz.lo $CPPFLAGS 
> $CFLAGS baz.c],[0],[ignore],[ignore])
> +
> +AT_CHECK([$LIBTOOL --mode=link --tag=CC $CC -no-undefined -shared -o 
> libfoo.la $CPPFLAGS $CFLAGS $LDFLAGS foo.lo baz.lo -rpath 
> /nonexistent],[0],[ignore],[ignore])
> +
> +AT_CHECK([$LIBTOOL --mode=compile --tag=CC $CC -c -o bar.lo $CPPFLAGS 
> $CFLAGS bar.c],[0],[ignore],[ignore])
> +
> +AT_CHECK([$LIBTOOL --mode=link --tag=CC $CC -no-undefined -shared -o 
> libbar.la $CPPFLAGS $CFLAGS $LDFLAGS bar.lo libfoo.la -rpath 
> /nonexistent],[0],[ignore],[ignore])
> +
> +AT_CHECK([$LIBTOOL --mode=compile --tag=CC $CC -c -o main.lo $CPPFLAGS 
> $CFLAGS main.c],[0],[ignore],[ignore])
> +
> +AT_CHECK([$LIBTOOL --mode=link --tag=CC $CC -o main$EXEEXT $CPPFLAGS $CFLAGS 
> $LDFLAGS main.lo libbar.la],[0],[ignore],[ignore])
> +
> +AT_CLEANUP
> +
> +AT_SETUP([dll install to bindir])
> +
> +AT_DATA([foo.c],[[
> +int x=0;
> +]])
> +
> +AT_DATA([bar.c],[[
> +extern int x;
> +int bar(void);
> +int bar() { return x;}
> +]])
> +
> +AT_DATA([baz.c],[[
> +int y=0;
> +]])
> +
> +AT_CHECK([$LIBTOOL --mode=compile --tag=CC $CC -c -o foo.lo $CPPFLAGS 
> $CFLAGS foo.c],[0],[ignore],[ignore])
> +AT_CHECK([$LIBTOOL --mode=compile --tag=CC $CC -c -o baz.lo $CPPFLAGS 
> $CFLAGS baz.c],[0],[ignore],[ignore])
> +AT_CHECK([$LIBTOOL --mode=link --tag=CC $CC -no-undefined -shared -o 
> libfoo.la $CPPFLAGS $CFLAGS $LDFLAGS foo.lo baz.lo -rpath 
> /nonexistent],[0],[ignore],[ignore])
> +
> +AT_CHECK([$LIBTOOL --mode=compile --tag=CC $CC -c -o bar.lo $CPPFLAGS 
> $CFLAGS bar.c],[0],[ignore],[ignore])
> +
> +# Now try installing the libs.  There are the following cases:
> +# No -bindir
> +# -bindir below lib install dir
> +# -bindir is lib install dir
> +# -bindir beside lib install dir
> +# -bindir above lib dir
> +# -bindir above and beside lib dir
> +# -bindir in entirely unrelated prefix.
> +
> +curdir=`pwd`
> +libdir=${curdir}/usr/lib/gcc/i686-pc-cygwin/4.5.0
> +
> +# Do a basic install with no -bindir option for reference
> +
> +AT_CHECK([$LIBTOOL --mode=link --tag=CC $CC -no-undefined -shared -o 
> libbar.la $CPPFLAGS $CFLAGS $LDFLAGS bar.lo libfoo.la -rpath 
> ${libdir}],[0],[ignore],[ignore])
> +rm -rf ${curdir}/usr
> +mkdir -p ${libdir}
> +AT_CHECK([$LIBTOOL --mode=install $lt_INSTALL libbar.la $libdir],
> +      [], [ignore], [ignore])
> +
> +# And ensure it went where we expect.
> +AT_CHECK(test -f $libdir/../bin/???bar-0.dll)
> +
> +for x in \
> +     ${curdir}/usr/lib/gcc/i686-pc-cygwin/4.5.0/bin/ \
> +     ${curdir}/usr/lib/gcc/i686-pc-cygwin/4.5.0/bin \
> +     ${curdir}/usr/lib/gcc/i686-pc-cygwin/4.5.0/ \
> +     ${curdir}/usr/lib/gcc/i686-pc-cygwin/4.5.0 \
> +     ${curdir}/usr/lib/gcc/i686-pc-cygwin/bin/ \
> +     ${curdir}/usr/lib/gcc/i686-pc-cygwin/bin \
> +     ${curdir}/usr/lib/gcc/i686-pc-cygwin/ \
> +     ${curdir}/usr/lib/gcc/i686-pc-cygwin \
> +     ${curdir}/usr/lib/bin/ \
> +     ${curdir}/usr/lib/bin \
> +     ${curdir}/usr/bin/ \
> +     ${curdir}/usr/bin \
> +     ${curdir}/bin/ \
> +     ${curdir}/bin \
> +     /tmp/$$/foo/bar ;

Using 'rm -rf /tmp/$$' is a straight path to a security CVE, and writing
there still at least a DoS waiting to happen.  Don't ignore TMPDIR, and
please use the sample mktemp replacement code from the Autoconf manual
if you really need to create files in a temporary directory.

What about duplicate slashes BTW?  They occur frequently with
  --prefix=/some/where/
or similar (new enough Autoconf releases strip the trailing slash in
this case, so you might want to wait for my GCC autotools upgrade patch
set).  But have you tested them?

> +do
> +
> +  AT_CHECK([$LIBTOOL --mode=link --tag=CC $CC -no-undefined -bindir $x 
> -shared -o libbar.la $CPPFLAGS $CFLAGS $LDFLAGS bar.lo libfoo.la -rpath 
> ${libdir}],[0],[ignore],[ignore])
> +
> +  rm -rf ${curdir}/usr ${curdir}/bin /tmp/$$
> +  mkdir -p ${libdir} $x ${curdir}/usr ${curdir}/bin /tmp/$$
> +  AT_CHECK([$LIBTOOL --mode=install $lt_INSTALL libbar.la $libdir],
> +      [], [ignore], [ignore])
> +  # Ensure it went to bindir this time.
> +  AT_CHECK(test -f $x/???bar-0.dll)
> +done
> +
> +AT_CLEANUP
> +
> +}
> +

Thanks,
Ralf