On Tuesday 14 September 2010, Peter Rosin wrote:
> Hi Stefano,
>
> Den 2010-09-14 20:14 skrev Stefano Lattarini:
> > On Tuesday 14 September 2010, Peter Rosin wrote:
> [CUT]
> >> or otherwise difficult to merge due to other changes. I'm also
> >> not sure if this is the desired route. Please advise.
> >
> > Done :-). But let's hear what Ralf has to say, he might know
> > better.
>
> Yes. I don't want to do that bit of work in vain.
Good idea, there's no haste after all.
> >> This patch is on top of the msvc branch.
> >>
> >> AC_PROG_RANLIB is rendered obsolete by LT_INIT. Is it also
> >> rendered obsolete by AC_PROG_LIBTOOL? Should I not care about
> >> libtool 1.5?
> >>
> >> Cheers,
> >> Peter
> >>
> >>
> >> diff --git a/ChangeLog b/ChangeLog
> >> + * tests/ar-lib2.test: New test. Test if AM_PROG_AR triggers
> >> + install of ar-lib.
> >
> > I feel that something like "New test, checking that AM_PROG_AR
> > triggers install of ar-lib" would sound more natural. More
> > instances below. This is admittedly a bike-shedding issue, so
> > feel free to ignore it.
>
> ok
Thanks.
> >> +: ${AR=ar}
> >> +
> >> +AC_CACHE_CHECK([the archiver ($AR) interface],
> >> [am_cv_ar_interface], + [am_cv_ar_interface=ar
> >> + AC_COMPILE_IFELSE([[int some_variable = 0;]],
> >> + [am_ar_try='$AR cru libconftest.a conftest.$ac_objext
> >> >&AS_MESSAGE_LOG_FD'
> >
> > What is the rationale for not redirecting stderr here, along with
> > stdout?
>
> But stderr is redirected?
Where? Am I mising something?
> >> + AC_TRY_EVAL([am_ar_try])
> >> + if test "$ac_status" -eq 0; then
> >> + am_cv_ar_interface=ar
> >> + else
> >> + am_ar_try='$AR -NOLOGO -OUT:conftest.lib conftest.$ac_objext'
> >
> > What is the rationale for not redirecting stdout/stderr here,
> > considered how AC_TRY_EVAL is used above (with redirection)?
>
> No rationale at all, I just didn't see any trash. But it's better
> to redirect now than to wait for the bug report. Fixed.
As above: where is stderr redirected?
> >> + # FIXME: It is wrong to rewrite AR.
> >> + # But if we don't then we get into trouble of one sort or another.
> >> + # A longer-term fix would be to have automake use am__AR in this case,
> >> + # and then we could set am__AR="\$(top_srcdir)/ar-lib \$(AR)"
> >
> > Do you mean am__AR="$am_aux_dir/ar-lib $AR" ?
>
> I'm not sure what I mean actually. I stole shamelessly from
> m4/minuso.m4.
Yep, there are some bogus comments there too. They seem to be a
leftover from a time when automake still placed all auxiliary scripts
in $(top_srcdir) unconditionally.
> I replaced top_srcdir with am_aux_dir for this round, but kept
> the Makefile notation, or is that totally bogus too?
I'd go with the shell syntax. See more details below.
> >> diff --git a/tests/ar-lib4.test b/tests/ar-lib4.test
> >> +$AUTOCONF
> > useless autoconf call
>
> Zapped. Also for ar-lib3.test
Nicely spotted. Thanks.
> > This testcase checks for two distinct automake failures; I'd
> > prefer that to be done in two different tests, one checking for
> > missing `AM_PROG_AR' call, the other one for missing `ar-lib'
> > auxfile. WDYT?
>
> I added ar-lib7.test and removed the second AUTOMAKE_fails from
> both ar-lib3.test
Thanks for spotting this too, I missed it.
> and ar-lib4.test.
> >> diff --git a/tests/ar-lib5.test b/tests/ar-lib5.test
> >
> > Hmm... grepping the output of make might be fragile...
> >
> > What about rewriting the test on the following lines, to make it
> > even more "semantic"?
> > [CUT]
> > 5. Run "$MAKE check" and "$MAKE distcheck"
>
> I had to also add a line
> DISTCHECK_CONFIGURE_FLAGS = AR=lib RANLIB=:
> to Makefile.am
Yes, sorry I didn't mention it explicitly. BTW, you could have also
set DISTCHECK_CONFIGURE_FLAGS on the make command line:
$MAKE distcheck DISTCHECK_CONFIGURE_FLAGS='AR=lib RANLIB=:'
That said, your way too is perfectly fine for the purposes of the test.
> and adjust the creation on configure.in to get
> the correct ordering.
Yes, that's an unfortunate necessity with the current `tests/defs.in'.
There's a pending patch of mine about this too, somewhere in the list
archives (reviews are welcome ;-)
> >> diff --git a/tests/ar-lib6.test b/tests/ar-lib6.test
> >> +$AUTOCONF 2>stderr
> >> +cat stderr
> >
> > Please ansure the stderr is displayed even if autoconf fails:
> > $AUTOCONF 2>stderr || { cat stderr >&2; Exit 1; }
> > cat stderr >&2
> >
> > Yes, I admit this idiom is annoying and should be factored out
> > somehow... patch in preparation ;-)
>
> Ok, can you please hold that until after this is through the pipe
> though, so that I don't have to fixup when merging?
Don't worry, the patch is for the tests-init branch, so no merge problems
for you :-) Besides, there is a good share of pending patches holding
that one back, so your patch will almost surely be applied before mine
anyway.
> >> diff --git a/tests/defs.in b/tests/defs.in
> >> + ( $AR -? ) || exit_status=$?
> >
> > Nitpicking: why a subshell here?
>
> I started out by copying the cl) branch and it stuck after I
> realized that lib exited non-zero no matter what when given
> non-work options (for options that produce output that is,
> "lib -nologo" does return zero but without output so feels
> a bit uninformative).
>
> Thanks for the review! Below is the next iteration.
And below is next round comments and annoyances ;-)
> Cheers,
> Peter
> diff --git a/ChangeLog b/ChangeLog
> index 02f2fcd..36adf60 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,38 @@
> +2010-09-14 Peter Rosin <p...@lysator.liu.se>
> + Ralf Wildenhues <ralf.wildenh...@gmx.de>
> +
> + Add new 'AM_PROG_AR' macro, triggering the 'ar-lib' script.
> + * m4/ar-lib.m4: New macro AM_PROG_AR, which locates an
> + archiver and triggers the auxiliary 'ar-lib' script if needed.
> + * m4/Makefile.am: Add above.
Better to do this:
* m4/Makefile.am (dist_m4data_DATA): Add above.
or even:
* m4/Makefile.am (dist_m4data_DATA): Updated.
(and ditto in the git commit message). Sorry for not noticing it in
the previous review.
> + * automake.in (seen_ar): New variable.
s/seen_ar/$seen_ar/ (again, sorry for not noticing it before).
> + (scan_autoconf_traces): Set it.
> + (handle_libraries): Don't set default values for AR and ARFLAGS
> + if AM_PROG_AR has been seen.
> + (handle_libraries, handle_ltlibraries): Require AM_PROG_AR for
> + portability.
> + * doc/automake.texi (Public Macros): Mention the new
> + 'AM_PROG_AR' macro.
> + (Subpackages): Add AM_PROG_AR to the example.
> + (A Library): Adjust recommendations for AR given the new
> + AM_PROG_AR macro.
> + * tests/ar.test: Adjust to avoid portability warnings.
> + * tests/ar-lib2.test: New test, checking that AM_PROG_AR triggers
> + install of ar-lib.
> + * tests/ar-lib3.test: New test, checking that lib_LIBRARIES
> + requires AM_PROG_AR.
> + * tests/ar-lib4.test: New test, checking that lib_LTLIBRARIES
> + requires AM_PROG_AR.
> + * tests/ar-lib5.test: New test, checking that AM_PROG_AR triggers
> + use of ar-lib when the archiver is Microsoft lib.
> + * tests/ar-lib6.test: New test, checking the ordering of
> + AM_PROG_AR and LT_INIT.
> + * tests/ar-lib7.test: New test, checking that automake warns
> + if ar-lib is missing.
> + * tests/defs.in: New required entry 'lib'.
> + * tests/Makefile.am: Add new tests.
Or better:
* tests/Makefile.am (TESTS): Update.
(and ditto in the git commit message)
> + * NEWS: Update.
> +
> diff --git a/m4/ar-lib.m4 b/m4/ar-lib.m4
> new file mode 100644
> index 0000000..f670f6e
> --- /dev/null
> +++ b/m4/ar-lib.m4
> @@ -0,0 +1,60 @@
> +## -*- Autoconf -*-
> +# Copyright (C) 2010
> +# Free Software Foundation, Inc.
I think this should be written as:
# Copyright (C) 2010 Free Software Foundation, Inc.
on a single line.
> +#
> +# This file is free software; the Free Software Foundation
> +# gives unlimited permission to copy and/or distribute it,
> +# with or without modifications, as long as this notice is reserved.
> +
> +# serial 1
> +
> +# AM_PROG_AR
> +# --------------
This should be removed, since ...
> +# AM_PROG_AR([ACT-IF-FAIL])
> +# -------------------------
... you added this (which is correct AFAICS).
> +# Try to determine the the archiver interface and trigger the
> +# ar-lib wrapper if it is needed;
> +# If unable to, run ACT-IF-FAIL (default: abort configure).
What about this instead?
# Try to determine the archiver interface, and trigger the ar-lib wrapper
# if it is needed. If the detection of archiver interface fails, run
# ACT-IF-FAIL (default is to abort configure with a proper error message).
> +AC_DEFUN([AM_PROG_AR],
> +[AC_BEFORE([$0], [LT_INIT])dnl
> +AC_BEFORE([$0], [AC_PROG_LIBTOOL])dnl
> +AC_REQUIRE([AM_AUX_DIR_EXPAND])dnl
> +AC_REQUIRE_AUX_FILE([ar-lib])dnl
> +AC_CHECK_TOOLS([AR], [ar lib "link -lib"], [false])
> +: ${AR=ar}
> +
> +AC_CACHE_CHECK([the archiver ($AR) interface],
> [am_cv_ar_interface], + [am_cv_ar_interface=ar
> + AC_COMPILE_IFELSE([[int some_variable = 0;]],
> + [am_ar_try='$AR cru libconftest.a conftest.$ac_objext
> >&AS_MESSAGE_LOG_FD'
Maybe s|>&AS_MESSAGE_LOG_FD|>&AS_MESSAGE_LOG_FD 2>&1| ? (not sure about this)
> + AC_TRY_EVAL([am_ar_try])
> + if test "$ac_status" -eq 0; then
> + am_cv_ar_interface=ar
> + else
> + am_ar_try='$AR -NOLOGO -OUT:conftest.lib onftest.$ac_objext
> >&AS_MESSAGE_LOG_FD'
Maybe s|>&AS_MESSAGE_LOG_FD|>&AS_MESSAGE_LOG_FD 2>&1| here too?
> + AC_TRY_EVAL([am_ar_try])
> + if test "$ac_status" -eq 0; then
> + am_cv_ar_interface=lib
> + else
> + m4_default([$1],
> + [AC_MSG_ERROR([could not determine $AR interface])])
> + fi
> + fi
> + rm -f conftest.lib libconftest.a
> + ])
> + ])
> +
> +case $am_cv_ar_interface in
> +ar)
> + ;;
> +lib)
> + # Microsoft lib, so override with the ar-lib wrapper script.
> + # FIXME: It is wrong to rewrite AR.
> + # But if we don't then we get into trouble of one sort or another.
> + # A longer-term fix would be to have automake use am__AR in this case,
> + # and then we could set am__AR="\$(am_aux_dir)/ar-lib \$(AR)"
What about:
``... and then we could set am__AR="$am_aux_dir/ar-lib \$(AR)" or
something similar''.
instead? This seems somewhat more correct, without forcing us to be
overly precise and explicit.
> + AR="$am_aux_dir/ar-lib $AR"
> + ;;
> +esac
> +AC_SUBST([AR])dnl
> +])
> diff --git a/tests/ar-lib5.test b/tests/ar-lib5.test
> new file mode 100755
> index 0000000..63ae2a3
> --- /dev/null
> +++ b/tests/ar-lib5.test
> @@ -0,0 +1,64 @@
> +#! /bin/sh
> +
> +requires=lib
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat > configure.in << 'END'
> +AC_INIT([ar-lib5.test], [1.0])
We prefer not to hard-code the test name if possible; what about:
cat > configure.in << END
AC_INIT([$me], [1.0])
instead? This shouldn't be a problem, since the rest of the
configure.in content doesn't require to be quoted.
> +AC_CONFIG_AUX_DIR([auxdir])
> +AM_INIT_AUTOMAKE
> +AC_CONFIG_FILES([Makefile])
> +AC_PROG_CC
> +AM_PROG_AR
> +AC_PROG_RANLIB
> +AC_OUTPUT
> +END
> diff --git a/tests/ar-lib7.test b/tests/ar-lib7.test
> new file mode 100755
> index 0000000..5182894
> --- /dev/null
> +++ b/tests/ar-lib7.test
> @@ -0,0 +1,41 @@
> +#! /bin/sh
> +# Copyright (C) 2010 Free Software Foundation, Inc.
>
> +# Test if automake warns if ar-lib is missing.
Nitpicking again: s/\.$/when AM_PROG_AR is used./ ?
> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >> configure.in << 'END'
> +AC_PROG_CC
> +AC_PROG_RANLIB
> +AM_PROG_AR
> +END
> +
> +cat > Makefile.am << 'END'
> +lib_LIBRARIES = libfoo.a
> +libfoo_a_SOURCES = foo.c
> +END
>
Are these truly required? I mean, if AM_PROG_AR does not require
AC_PROG_CC and AC_PROG_RANLIB to be called explicitly, the above
could be simplified to just:
cat >> configure.in << 'END'
AM_PROG_AR
END
: > Makefile.am
right?
> +
> +$ACLOCAL
> +AUTOMAKE_fails
> +
> +grep 'ar-lib.*not found' stderr
> +
> +$AUTOMAKE --add-missing
> +
> +:
> diff --git a/tests/defs.in b/tests/defs.in
> index af4a3cd..bd2e813 100644
> --- a/tests/defs.in
> +++ b/tests/defs.in
> @@ -156,6 +156,16 @@ do
> echo "$me: running $CC -V -help"
> ( $CC -V -help ) || exit 77
> ;;
> + lib)
> + AR=lib
> + export AR
> + # There is no way to get any identifying output with
> + # a zero exit status. So, remap exit status 76 to 0.
> + echo "$me: running $AR -?"
> + exit_status=0
> + $AR -? || exit_status=$?
Very paranoid nitpicking, but... what about using "$AR -\?", to
be sure not to trigger unexpected file globbing?
> + test $exit_status = 76 && exit 77
I'm not sure I understand the semantic here; an exit status of 76
means you have the expected `lib' program... and in this case you
skip the test because it requires `lib' itself? (BTW, sorry for
not having noticed this in my prevous review).
Thanks,
Stefano
