libtool-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Gary V. Vaughan
Subject: Re: FYI: 333-gary-refactor-LTDL_INIT.patch
Date: Tue, 8 Jan 2008 12:23:24 +0800

On 8 Jan 2008, at 04:40, Ralf Wildenhues wrote:
Hello Gary,

Morgen Ralf,

Thanks for the review.

* 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?  ;-)

I'll address whatever I can before I roll up the alpha and have my INBOX
flooded :-)

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?

Just bad memory. Actually, the at macro is pushed and popped so everything continues to work properly, even so I'll fix it to avoid confusion later.

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.)

I think it is okay to use libtool internal macros internally to libtool ;-)
It's not as pretty as I'd like, but if we think of a better way later we
can always change it.  For now I just want to make the release already!

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?

I noticed this yesterday when I started the alpha release process too.
Yes, I'll figure this out before I go any further.

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?

Nice catch!  I'll patch that separately in a moment.  Thanks.


 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/  ?

ACK.

        * 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/\.$//  ?

D'oh!  ACK.

 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/

ACK.

 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?

One day. ;-) I'll look at it again on the other side of the alpha, if not
before.

 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.

CPPFLAGS and LDFLAGS will continue to work as they always have done.  It
seems far too obvious to mention in the docs though. I have noticed that
many autotools users seem to prefer passing --enable/with-options though
so I'm providing those too.

+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.

Hmm.  Bummer.  Could you add a test?  Or, Bob, if you could give me
access to your build logs, I'll look into creating one myself.

 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).

Oh.  Guess I haven't been paying attention to the Autoconf developments
for a while then :-(  What should I have written instead?

 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?

I don't know. If it is, I didn't see it before I wrote this patch.
I'll try to find time to remind myself why I needed that, and come
up with a test case if necessary.

 @@ -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.

Even with old autoconf?

 --- 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/

ACK.

[ 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?

Oh, sorry. I started this patch long before that fix hit the tree. Nice catch.
I'll patch it separately.

 --- /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.

I'm not sure how to test it though... any thoughts?

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

That doesn't work.  Apart from the quoting errors ("$x"),

From the line above $x can never be empty (at worst it will be a literal
'*').

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?

ACK.

+# 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?

That might work.  I'll try to find time after I've rolled the alpha up.

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

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

D'oh!  ACK.

Cheers, and thanks for all your work on this!

Haha... if only I'd done it a year or two sooner, eh?

Ralf

Cheers,
        Gary
--
  ())_.              Email me: address@hidden
  ( '/           Read my blog: http://blog.azazil.net
  / )=         ...and my book: http://sources.redhat.com/autobook
`(_~)_      Join my AGLOCO Network: http://www.agloco.com/r/BBBS7912



Attachment: PGP.sig
Description: This is a digitally signed message part


reply via email to

[Prev in Thread] Current Thread [Next in Thread]