[Top][All Lists]
[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
PGP.sig
Description: This is a digitally signed message part
- FYI: 333-gary-refactor-LTDL_INIT.patch, Gary V. Vaughan, 2008/01/06
- Re: FYI: 333-gary-refactor-LTDL_INIT.patch, Ralf Wildenhues, 2008/01/07
- Re: FYI: 333-gary-refactor-LTDL_INIT.patch,
Gary V. Vaughan <=
- Re: FYI: 333-gary-refactor-LTDL_INIT.patch, Ralf Wildenhues, 2008/01/08
- Re: FYI: 333-gary-refactor-LTDL_INIT.patch, Ralf Wildenhues, 2008/01/16
- Re: FYI: 333-gary-refactor-LTDL_INIT.patch, Gary V. Vaughan, 2008/01/16
- Re: FYI: 333-gary-refactor-LTDL_INIT.patch, Ralf Wildenhues, 2008/01/17
- Re: FYI: 333-gary-refactor-LTDL_INIT.patch, Gary V. Vaughan, 2008/01/17
- Re: FYI: 333-gary-refactor-LTDL_INIT.patch, Ralf Wildenhues, 2008/01/17
- Re: FYI: 333-gary-refactor-LTDL_INIT.patch, Ralf Wildenhues, 2008/01/08
- Re: FYI: 333-gary-refactor-LTDL_INIT.patch, Ralf Wildenhues, 2008/01/14
- Re: FYI: 333-gary-refactor-LTDL_INIT.patch, Gary V. Vaughan, 2008/01/14
- Re: FYI: 333-gary-refactor-LTDL_INIT.patch, Ralf Wildenhues, 2008/01/15