Re: FYI: 333-gary-refactor-LTDL_INIT.patch

[Ralf Wildenhues]

Hello Gary,

* Gary V. Vaughan wrote on Sun, Jan 06, 2008 at 05:35:30PM CET:
> 
> I'm going to try and roll an alpha release for wider testing over the
> next day or two.

Well, I finally got done with my look over this, so you may want to
consider it first.  And when you're done and bored again, I will post a
few test failures to ponder over, ok?  ;-)


Why is the same name, _LTDL_SETUP, used for an internal macro in ltdl.m4
as well as for a helper macro in tests/*at?  Moreover, why is the
internal macro leaking into configure.ac and libltdl/configure.ac?
(Hmm, upon rereading, maybe that last item is just the way it has 
to be; sigh.)

With this patch, I see in the log of
  make distcheck

that the libltdl subdirectory is being configured.  This is wrong, as
libltdl in the Libtool package should be in nonrecursive mode rather
than in subproject mode.  This causes distcheck to fail.  Can you fix
this?

There is code that checks $prefix/lib.  I realize this has been that way
before your patch, but shouldn't we test (expanded) $libdir instead, so
that users of libdir='${prefix}/lib64' get what they want?

>   Index: ChangeLog
>   from  Gary V. Vaughan  <address@hidden>
>   
>       Move libltdl build mode options from LT_CONFIG_LTDL_DIR to
>       LTDL_INIT.  Accept (optional) new 'convenience' and 'installable'
>       options in lieu of LTDL_CONVENIENCE and LTDL_INSTALL macros. And
>       also, implement --with-included-ltdl, --with-ltdl-lib and
>       --with-ltdl-include configure-time options:
>   
>       * libltdl/m4/ltoptions.m4: Associate existing option settings with
>       LT_INIT.
>       (LT_OPTION_DEFINE): Associate options 'nonrecursive', 'recursive',
>       'subproject', 'installable' and 'convenience' with LTDL_INIT.
>       (_LT_MANGLE_OPTION, _LT_SET_OPTION, _LT_IF_OPTION)
>       (_LT_UNLESS_OPTIONS, _LT_SET_OPTIONS): Add MACRO-NAME argument to
>       support options to named macros instead of hardcoding only LT_INIT
>       options.
>       * libltdl/m4/ltdl.m4 (_LTDL_CONVENIENCE): Factor common code from
>       LTDL_CONVENIENCE and LTDL_INIT([convenience]).
>       (_LTDL_INSTALLABLE): Factor out common code from LTDL_INSTALLABLE
>       and LTDL_INIT([installable]).
>       (_LTDL_SETUP): Renamed from LTDL_INIT.  Support new configure-time
>       options: --with-included-ltdl, --with-ltdl-lib,
>       --with-ltdl-include.
>       (LTDL_CONVENIENCE, LTDL_INSTALLABLE): Adjust.
>       (LT_WITH_LTDL): Removed.
>       (LTDL_INIT): Parse caller options.
>       * libltdl/m4/libtool.m4 (LT_INIT): Declare that this macro must be
>       expanded before LTDL_INIT, and be sure to parse caller options.
>       * configure.ac: Call directly into internal _LTDL_SETUP macro.
>       * libtoolize.m4sh (func_scan_files): Ltdl mode argument moved from
>       LT_CONFIG_LTDL_DIR to LT_INIT.

s/LT_INIT/LTDL_INIT/  ?

>       * tests/nonrecursive.at, tests/recursive.at: Use new interfaces.
>       * tests/configure-iface.at: Test it.
>       * Makefile.am (TESTSUITE_AT): Add configure-iface.at.
>       * doc/libtool.texi (Distributing libltdl): Document improved.

s/\.$//  ?

>       LTDL_INIT interfaces.
>       * NEWS: Updated.
>   

>   Index: NEWS
>   ===================================================================
>   RCS file: /sources/libtool/libtool/NEWS,v
>   retrieving revision 1.209
>   diff -u -u -r1.209 NEWS
>   --- NEWS 2 Jan 2008 18:52:08 -0000 1.209
>   +++ NEWS 6 Jan 2008 16:32:05 -0000
[...]
>   @@ -31,6 +33,14 @@
>      - Fix installation of libltdl so that it does not need Autoconf and
>        Automake installed, in order to be usable in another package.  This
>        lifts the restrictions introduced in 1.9b.
>   +  - Default convenience or installable libltdl builds can optionally
>   +    be declared using new `convenience' or `installable' options to

s/$/ the/

>   +    LTDL_INIT macro (as an alternative to individual LTDL_CONVENIENCE
>   +    or LTDL_INSTALLABLE invocations).
>   +  - New configure-time options to allow libltdl parent project builder
>   +    to choose between installed and shipped libltdl, when invoking
>   +    LTDL_INIT: --with-included-ltdl, --with-ltdl-include,
>   +    --with-ltdl-lib.
>      - New LT_CONFIG_LTDL_DIR macro to specify a different directory name
>        for a convenience libltdl.
>      - libtoolize has been completely overhauled.

>   Index: libtoolize.m4sh
>   ===================================================================
>   RCS file: /sources/libtool/libtool/libtoolize.m4sh,v
>   retrieving revision 1.63
>   diff -u -u -r1.63 libtoolize.m4sh
>   --- libtoolize.m4sh 2 Jan 2008 18:52:08 -0000 1.63
>   +++ libtoolize.m4sh 6 Jan 2008 16:32:07 -0000

>   @@ -481,9 +480,26 @@
>        # Validate ltdl_mode. #
>        # ------------------- #
>    
>   -    test -n "$ac_ltdl_mode" && seen_ltdl=:
>   +    test -n "$ltdl_options" && seen_ltdl=:
>    
>   -    # If neither --ltdl nor LT_CONFIG_LTDL_DIR are specified, default to
>   +    # If $configure_ac contains LTDL_INIT, check that its
>   +    # arguments were not given in terms of a shell variable!
>   +    case "$ltdl_options" in
>   +      *\$*)
>   +        func_fatal_error "can not handle variables in LTDL_INIT"
>   +        ;;
>   +    esac
>   +
>   +    # Extract mode name from ltdl_options
>   +    # FIXME: Diagnose multiple conflicting modes in ltdl_options

Are you going to address this FIXME?

>   +    ac_ltdl_mode=
>   +    case " $ltdl_options " in
>   +      *" nonrecursive "*)  ac_ltdl_mode=nonrecursive       ;;
>   +      *" recursive "*)     ac_ltdl_mode=recursive  ;;
>   +      *" subproject "*)    ac_ltdl_mode=subproject ;;
>   +    esac
>   +
>   +    # If neither --ltdl nor an LTDL_INIT mode are specified, default to
[...]


>   Index: doc/libtool.texi
>   ===================================================================
>   RCS file: /sources/libtool/libtool/doc/libtool.texi,v
>   retrieving revision 1.233
>   diff -u -u -r1.233 libtool.texi
>   --- doc/libtool.texi 30 Nov 2007 04:18:40 -0000 1.233
>   +++ doc/libtool.texi 6 Jan 2008 16:32:10 -0000

>   +If the @option{--with-included-ltdl} is not passed at
>   +configure time, and an installed @code{libltdl} is not
>   address@hidden@c
>   address@hidden
>   +Even if libltdl is installed, @samp{LTDL_INIT} may fail
>   +to detect it if libltdl depends on symbols provided by libraries
>   +other than the C library.
>   address@hidden
>   +}, then @command{configure} will exit immediately with an error that
>   +asks the user to either specify the location of an installed
>   address@hidden using the @option{--with-ltdl-include} and
>   address@hidden options, or to build with the
>   address@hidden sources shipped with the package by passing
>   address@hidden

Sure you want to disallow users just passing in appropriate CPPFLAGS and
LDFLAGS to find an installed libltdl?  I would count that as an
incompatible API change, too.

>   +If an installed @code{libltdl} is found, then @code{LIBLTDL} is set to
>   +the link flags needed to use it, and @code{LTDLINCL} to the
>   +preprocessor flags needed to find the installed headers.  Note,
>   +however, that no version checking is performed.  You should manually
>   +check for the @code{libltdl} features you need in @file{configure.ac}:
[...]

>   +Whatever method you use, @samp{LTDL_INIT} will define both the shell
>   +variable @var{LIBLTDL} to the link flag that you should use to link
>   +with @code{libltdl}, and the shell variable @var{LTDLINCL} to the
>   +preprocessor flag that you should use to compile programs that
>   +include @file{ltdl.h}. So, when you want to link a program with
>    libltdl, be it a convenience, installed or installable library, just
[...]
>   +use @samp{$(LTDLINCL)} for preprocessing and compilation, and
>   address@hidden(LIBLTDL)} for linking.

Unrelated to your patch, because present before:

This fails to mention whether $(LIBLTDL) may be used as a dependency in,
say, libfoo_la_DEPENDENCIES.  The case where it is `-lltdl' is the
tricky one, as it leads to a make error, so we may have to introduce
another variable?  See M4 build failures on Bob's build farm for an
example of this.

>   address@hidden
>   +If your package is built using the convenience libltdl, @var{LIBLTDL}
>   +will be the pathname for the convenience version of libltdl
>   +(starting with @address@hidden@}/}) and @var{LTDLINCL} will be
>   address@hidden followed by the directory that contains @file{ltdl.h}
>   +(starting with @address@hidden@}/}).
>   +
>   address@hidden
>   +If an installable version of the included @code{libltdl} is being
>   +built, its pathname starting with @address@hidden@}/}, will be
>   +stored in @var{LIBLTDL}, and @var{LTDLINCL} will be set just like in
>   +the case of convenience library.
>   address@hidden itemize

These two paragraphs are factually wrong if top_build_prefix is
available (i.e., with new enough Autoconf).

>   Index: libltdl/m4/libtool.m4
>   ===================================================================
>   RCS file: /sources/libtool/libtool/libltdl/m4/libtool.m4,v
>   retrieving revision 1.124
>   diff -u -u -r1.124 libtool.m4
>   --- libltdl/m4/libtool.m4 2 Jan 2008 18:52:08 -0000 1.124
>   +++ libltdl/m4/libtool.m4 6 Jan 2008 16:32:11 -0000
>   @@ -68,6 +68,7 @@
>    [AC_PREREQ([2.58])dnl We use AC_INCLUDES_DEFAULT
>    AC_BEFORE([$0], [LT_LANG])dnl
>    AC_BEFORE([$0], [LT_OUTPUT])dnl
>   +AC_BEFORE([$0], [LTDL_INIT])dnl
>    AC_REQUIRE([_LT_CHECK_BUILDDIR])dnl
>    
>    dnl Autoconf doesn't catch unexpanded LT_ macros by default:
>   @@ -80,7 +81,9 @@
>    AC_REQUIRE([LTVERSION_VERSION])dnl
>    AC_REQUIRE([LTOBSOLETE_VERSION])dnl
>    m4_require([_LT_PROG_LTMAIN])dnl
>   -m4_require([_LT_SET_OPTIONS], [_LT_SET_OPTIONS([$1])])dnl
>   +
>   +dnl Parse OPTIONS
>   +_LT_SET_OPTIONS([$0], [$1])

Is this change fixing a bug?  If yes, why not have a test for it?

>   @@ -617,10 +620,10 @@
>          lt_cl_silent=: ;;
>    
>        -*) AC_MSG_ERROR([unrecognized option: $[1]
>   -Try `$[0] --help' for more information.]) ;;
>   +Try \`$[0] --help' for more information.]) ;;
>    
>        *) AC_MSG_ERROR([unrecognized argument: $[1]
>   -Try `$[0] --help for more information.]) ;;
>   +Try \`$[0] --help' for more information.]) ;;

This hunk is completely superfluous.  Backticks get escaped properly
automatically with AC_MSG_ERROR.

>   --- libltdl/m4/ltdl.m4 16 Nov 2007 07:08:34 -0000 1.34
>   +++ libltdl/m4/ltdl.m4 6 Jan 2008 16:32:11 -0000

>   @@ -33,16 +33,10 @@
>           [],
>       [m4_fatal([multiple libltdl directories: `]_LTDL_DIR[', 
> `]_ARG_DIR['])])])
>    m4_popdef([_ARG_DIR])
>   -dnl If not otherwise defined, default to the 1.5.x compatible subproject 
> mode:
>   -m4_if(_LTDL_MODE, [],
>   -   [m4_define([_LTDL_MODE], m4_default([$2], [subproject]))
>   -   m4_if([-1], [m4_bregexp(_LTDL_MODE, 
> [\(subproject\|\(non\)?recursive\)])],
>   -           [m4_fatal([unknown libltdl mode: ]_LTDL_MODE)])])
>    ])# LT_CONFIG_LTDL_DIR

s/# LT/# _LT/


[ in _LTDL_INSTALLABLE ]
>   +# If configure.ac declared an installable ltdl, and the user didn't 
> override
>   +# with --disable-ltdl-install, we will install the shipped libltdl.
>   +case $enable_ltdl_install in
>   +  no) ac_configure_args="$ac_configure_args --enable-ltdl-install=no"
>   +      LIBLTDL="-lltdl"
>   +      LTDLINCL=
>   +      ;;
>   +  *)  enable_ltdl_install=yes
>   +      ac_configure_args="$ac_configure_args --enable-ltdl-install"
>   +      LIBLTDL='${top_builddir}'"${lt_ltdl_dir+/$lt_ltdl_dir}/libltdl.la"

Why is the _LT_BUILD_PREFIX bugfix re-broken here again?

>   +      LTDLINCL='-I${top_srcdir}'"${lt_ltdl_dir+/$lt_ltdl_dir}"
>   +      ;;
>   +esac
>   +
>    AC_SUBST([LIBLTDL])
>    AC_SUBST([LTDLINCL])


>   +   AC_MSG_ERROR([\`--with-ltdl-include' and \`--with-ltdl-lib' options 
> must be used together])

Again, backslashes are unnecessary but harmless.


>   --- /dev/null       1 Jan 1970 00:00:00 -0000
>   +++ tests/configure-iface.at 6 Jan 2008 16:32:11 -0000

>   +# TODO: Check that the installed program `main' is linked against our 
> libltdl
>   +AT_CHECK([test -f $prefix/lib/libltdl.la])
>   +AT_CHECK([test -f $prefix/include/ltdl.h])

Yep, that TODO is a rather important test that is missing.  It will be
crucial on systems like OpenBSD.

>   +# Remove build files
>   +for x in .* *; do
>   +  test $x == _inst || rm -rf $x
>   +done

That doesn't work.  Apart from the quoting errors ("$x"), you try to
remove testsuite.log which on w32 will not be possible (because it is
held open).  Why not just put libltdl in a subdir and remove that?

>   +AT_CHECK([test -f $prefix/lib/libltdl.la])
>   +AT_CHECK([test -f $prefix/include/ltdl.h])

>   +# We don't use 'libtoolize --ltdl', so that we get an error if the test
>   +# tries to build and link against its own ltdl sources:
>   +LT_AT_BOOTSTRAP([], [-I _inst/aclocal], [], [--add-missing], [],
>   +   [--with-ltdl-lib=$prefix/lib --with-ltdl-include=$prefix/include],
>   +   [all])
>   +
>   +## TODO: portable ldd check for correct libltdl
>   +## Currently, this test doesn't fail if `main' ends up linking against a
>   +## previously installed system libltdl.
>   +LT_AT_NOINST_EXEC_CHECK([./main], [-dlopen libmodule.la], [], [expout], [])

Hmm.  Can't you build a fake libltdl, install that in the same position,
and require that execution fails?

>   +LT_AT_BOOTSTRAP([--ltdl], [-I libltdl/m4], [], [--add-missing], [],
>   +        [--with-included-ltdl], [all])
>   +
>   +# --with-included-ltdl should build a convenience lib by default
>   +AT_CHECK([test -f libltdl/libltdlc.la])
>   +
>   +## TODO: portable ldd check for correct libltdl
>   +## Currently, this test doesn't fail if `main' ends up linking against a
>   +## previously installed system libltdl.
>   +LT_AT_NOINST_EXEC_CHECK([./main], [-dlopen libmodule.la], [], [expout], [])

Hmm again.

[recursive.at]
>   +LT_AT_LIBTOOLIZE([--debug --copy --ltdl])

Do we need that --debug in there?  It makes reading the output very
difficult.

Cheers, and thanks for all your work on this!
Ralf