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

[Gary V. Vaughan]

Hallo Ralf,

Thanks for the fast fixes!

On 16 Jan 2008, at 14:22, Ralf Wildenhues wrote:
> OK to apply?

I have some nits first.  Please commit once they are addressed.

> 2008-01-16  Ralf Wildenhues  <address@hidden>
>
>         New variable LIBLTDL_DEP for use in output_DEPENDENCIES.
>         * libltdl/m4/ltdl.m4 (_LTDL_CONVENIENCE, _LTDL_INSTALLABLE)
>         (LTDL_INIT): Set and substitute LIBLTDL_DEP according to chosen
>         method.
>         * tests/configure-iface.at (installable libltdl)
>         (--with-ltdl-include/lib, --with-included-ltdl): Test it.
>         * doc/libtool.texi (Distributing libltdl): Document LIBLTDL_DEP.

Our ChangeLog standard says to write intro's like this:

        New variable LIBLTDL_DEP for use in output_DEPENDENCIES:

        * libltdl/m4/ltdl.m4 (_LTDL_CONVENIENCE, _LTDL_INSTALLABLE)
        (LTDL_INIT): Set and substitute [[...]]

>         Also clarify that `${top_build_prefix}' may be used in place of
>         `${top_builddir}/'.

This is a different patch.

>         * NEWS: Update.

Except for a couple of instances in the latest ChangeLog rotation, we've
always used past tense here.  Super picky, I know, but we might as well
strive for consistency.

> [[...]]

> Index: doc/libtool.texi
> ===================================================================
> RCS file: /cvsroot/libtool/libtool/doc/libtool.texi,v
> retrieving revision 1.234
> diff -u -r1.234 libtool.texi
> --- doc/libtool.texi    6 Jan 2008 16:33:30 -0000       1.234
> +++ doc/libtool.texi    16 Jan 2008 06:21:06 -0000
> @@ -4443,10 +4443,11 @@
> @option{--with-included-ltdl}.
>
> 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}:
> +the link flags needed to use it, @code{LIBLTDL_DEP} is empty, and
> address@hidden to the preprocessor flags needed to find the  
> installed
> +headers.

"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, and @code{LIBLTDL_DEP} will
be empty."

I'm also not sure about the name LIBLTDL_DEP, as it is visibly different
to the other two, and also to existing PREOPEN_DEPENDENCIES and  
INFO_DEPS,
and gnulibs use of the '_DEPENDENCIES' suffix. I'd prefer either  
LTDLDEPS
(vis LTDLINCL) or LIBLTDL_DEPENDENCIES for consistency with prior art.
There are several other occurrences in the patch I haven't highlighted.

> -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
> +Whatever method you use, @samp{LTDL_INIT} will define the shell  
> variable
> address@hidden to the link flag that you should use to link
> +with @code{libltdl}, the shell variable @var{LIBLTDL_DEP} to the file

s/file/files/

> +that can be used as @command{make} dependency, and the shell variable

...can be used as a dependency in @file{Makefile} rules, ...

Actually, now that I think about it, the user doesn't see these as shell
variables at all, but as make macros.  It might be better to simply  
refer
to them as make macros to avoid any confusion.

> Index: tests/configure-iface.at
> ===================================================================
> RCS file: /cvsroot/libtool/libtool/tests/configure-iface.at,v
> retrieving revision 1.5
> diff -u -r1.5 configure-iface.at
> --- tests/configure-iface.at    12 Jan 2008 14:07:02 -0000      1.5
> +++ tests/configure-iface.at    16 Jan 2008 06:21:09 -0000
> @@ -144,6 +145,10 @@
> AT_CHECK([test -f $prefix/lib/libltdl.la])
> AT_CHECK([test -f $prefix/include/ltdl.h])
>
> +# Check that main is rebuilt if libltdl.la is newer
> +rm -f libltdl/libltdl.la
> +AT_CHECK([$MAKE -q main$EXEEXT || exit 1], [1], [ignore], [ignore])
> +

-q might not work with some of the makes that we support.  Safer to use:

  AT_CHECK([$MAKE main$EXEEXT >/dev/null 2>&1 || exit 1], ...


> @@ -262,6 +269,10 @@
> ## previously installed system libltdl.
> LT_AT_NOINST_EXEC_CHECK([./main], [-dlopen libmodule.la], [],  
> [expout], [])
>
> +# Check that main is rebuilt if libltdlc.la is newer
> +rm -f libltdl/libltdlc.la
> +AT_CHECK([$MAKE -q main$EXEEXT || exit 1], [1], [ignore], [ignore])

Same here.

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