Re: [patch 05/19] 288-gary-ltdl-nonrecursive-tests.diff Queue

[Gary V. Vaughan]

Ralf Wildenhues wrote:
> Hi Gary,

Hallo Ralf,

Thanks for the review!

> * Gary V. Vaughan wrote on Mon, Oct 10, 2005 at 12:26:29PM CEST:
> >         * tests/subdirectory.at: New tests for libltdl as a subdirectory,
> >         configured and compiled from the toplevel project.
> >         * tests/testsuite.at: Use it.
> >         * Makefile.am (TESTSUITE_AT): Depend on it.
>
> See nits below, and this one: all of them FAIL with 2.59/1.9.6 (without
> libobjdir fixes):
>
> [...]
> | nonrecursive.at:66: $AUTORECONF --force --verbose --install
> | stderr:
> | autoreconf: Entering directory `.'
> | autoreconf: configure.ac: not using Gettext
> | autoreconf: running: aclocal --force -I libltdl/m4
> | autoreconf: configure.ac: tracing
> | autoreconf: configure.ac: not using Libtool
> | autoreconf: running: autoconf --force
> | autoreconf: running: autoheader --force
> | autoreconf: running: automake --add-missing --copy --force-missing
> | configure.ac: installing `libltdl/config/install-sh'
> | configure.ac: installing `libltdl/config/missing'
> | configure.ac:8: installing `libltdl/config/config.guess'
> | configure.ac:8: installing `libltdl/config/config.sub'
> | Makefile.am: installing `libltdl/config/compile'
> | configure.ac:11: required file `./lt__dirent.c' not found
> | configure.ac:11: required file `./argz.c' not found
> | configure.ac:11: required file `./lt__strl.c' not found
> | Makefile.am: installing `libltdl/config/depcomp'
> | autoreconf: automake failed with exit status: 1
>
> Can you make them SKIP in this case, so we don't get oodles of bogus bug
> reports?

Nice catch.  Yes, I'll have a think about this.  I think I also need to
document that nonrecursive mode only works with full LIBOBJDIR support
in Automake and Autoconf...

> > +# _LTDL_SETUP
>
> The naming is unfortunate.  As all the tests share an m4 macro name
> space, and you actually use _LTDL_SETUP in several othere tests for a
> different purpose, please rename all of them.

No need, it's fine.  The name is quoted in the m4_define exactly so that
we can do this sort of thing.  What is the point of renaming?

> Maybe m4_undefine at the
> end of the test would be ok as well, but then I believe you wanted to
> share some macros.

Okay.  That's less intrusive.  I'll pushdef/popdef the definition then,
which is the cleanest way to stop it leaking out into other tests.

> > +# -----------
> > +m4_define([_LTDL_SETUP],
> > +[AT_DATA([configure.ac],
> > +[[AC_INIT([subdirectory-demo], ]AT_PACKAGE_VERSION[, ]AT_PACKAGE_BUGREPORT[)
> > +LT_CONFIG_LTDL_DIR([libltdl], [nonrecursive])
> > +AC_CONFIG_AUX_DIR([libltdl/config])
> > +AC_CONFIG_MACRO_DIR([libltdl/m4])
> > +AC_CONFIG_LIBOBJ_DIR([libltdl])
>
> Urgs.  In large packages, these three settings are going to be a major
> pain in the long run.  I'm not saying you need to change them here, this
> issue isn't a Libtool problem rather than an Autoconf/Automake one, but
> it'd be good if people could change all three of these and things would
> still work somehow.
>
> (Just thinking out loud; nothing we'll want to worry about before 2.0.)

Yes, the whole LIBOBJ model is, unfortunately, completely broken :-(  (I'm
sure you've seen the threads on gnulib, and the mess in CVS M4) Only toy
packages can cope well with a single directory of LIBOBJ sources.  I have
no idea how to fix this either... although when I transfer my attention
back to M4, inspiration may occur.  Luckily the requirements for the other
build modes aren't nearly so invasive.

> > +AM_PROG_CC_C_O
>
> Why do you need this?  Is it only for Automake backwards compatibility?

subdir-objects requires it.

> If so, then I recommend adding AC_PROG_CC before as well.

Seems like overkill to me... Belt and braces?

$ fgrep -l AM_PROG_CC_C_O /usr/share/aclocal*/*.m4
/usr/share/aclocal-1.5/minuso.m4
/usr/share/aclocal-1.6/minuso.m4
/usr/share/aclocal-1.7/minuso.m4
/usr/share/aclocal-1.8/minuso.m4
/usr/share/aclocal-1.9/minuso.m4
$ fgrep AC_REQUIRE /usr/share/aclocal*/minuso.m4
/usr/share/aclocal-1.5/minuso.m4:[AC_REQUIRE([AC_PROG_CC_C_O])dnl
/usr/share/aclocal-1.5/minuso.m4:AC_REQUIRE([AM_AUX_DIR_EXPAND])dnl
/usr/share/aclocal-1.6/minuso.m4:[AC_REQUIRE([AC_PROG_CC_C_O])dnl
/usr/share/aclocal-1.6/minuso.m4:AC_REQUIRE([AM_AUX_DIR_EXPAND])dnl
/usr/share/aclocal-1.7/minuso.m4:[AC_REQUIRE([AC_PROG_CC_C_O])dnl
/usr/share/aclocal-1.7/minuso.m4:AC_REQUIRE([AM_AUX_DIR_EXPAND])dnl
/usr/share/aclocal-1.8/minuso.m4:[AC_REQUIRE([AC_PROG_CC_C_O])dnl
/usr/share/aclocal-1.8/minuso.m4:AC_REQUIRE([AM_AUX_DIR_EXPAND])dnl
/usr/share/aclocal-1.9/minuso.m4:[AC_REQUIRE([AC_PROG_CC_C_O])dnl
/usr/share/aclocal-1.9/minuso.m4:AC_REQUIRE([AM_AUX_DIR_EXPAND])dnl
$ fgrep -l AC_PROG_CC_C_O /usr/share/autoconf/autoconf/*.m4
/usr/share/autoconf/autoconf/c.m4
/usr/share/autoconf/autoconf/oldnames.m4
$ sed -n '/^# AC_PROG_CC_C_O/,/])# AC_PROG_CC_C_O/p' 
/usr/share/autoconf/autoconf/c.m4
# AC_PROG_CC_C_O
# --------------
AC_DEFUN([AC_PROG_CC_C_O],
[AC_REQUIRE([AC_PROG_CC])dnl
...

> > +AM_INIT_AUTOMAKE([foreign subdir-objects])
>
> I believe you should have AM_INIT_AUTOMAKE before AM_PROG_CC_C_O.
> After all, AM_INIT_AUTOMAKE should be the first automake macro called.

Again, I'm not sure about this reasoning :-)  AM_PROG_CC_C_O is designed
to be a wrapper for AC_PROG_CC, and surely AC_PROG_CC should come before
AM_INIT_AUTOMAKE?  I don't want to change this, but if you are firm, and
it still works then I can be persuaded...

> > +LT_INIT
> > +LT_WITH_LTDL
> > +AC_CONFIG_FILES([Makefile])
> > +AC_OUTPUT
> > +]])
> > +
> > +AT_DATA([Makefile.am],
> > +[[ACLOCAL_AMFLAGS = -I libltdl/m4
> > +BUILT_SOURCES =
> > +EXTRA_DIST =
> > +CLEANFILES =
> > +MOSTLYCLEANFILES =
>
> If you agree on my suggestions wrt. Makefile.inc changes, this needs to
> be adjusted as well.

Agreed.  Thanks.

> > +touch foo.c
>
> Empty source files are not portable.
> You can use
>   echo 'static int dummy = 0;' > foo.c
> instead.  If you want, just fix the same bug in subproject.at as well,
> sorry for not seeing that earlier.

My bad.  I'd meant to change that myself... will do.

Cheers,
        Gary.
--
Gary V. Vaughan      ())_.  address@hidden,gnu.org}
Research Scientist   ( '/   http://tkd.kicks-ass.net
GNU Hacker           / )=   http://www.gnu.org/software/libtool
Technical Author   `(_~)_   http://sources.redhat.com/autobook

signature.asc
Description: OpenPGP digital signature