Re: [PATCH] Add new 'AM_PROG_AR' macro, triggering the 'ar-lib' script.

[Stefano Lattarini]

On Tuesday 14 September 2010, Peter Rosin wrote:
> Hi!
Hello Peter.  Hope you don't mind a quick partial review until we
hear from Ralf...

> This is a second attempt to implement AM_PROG_AR. The previous
> attempt was bundled with the addition of the 'ar-lib' script but
> was left behind. I have now fleshed it out with tests and
> portability warnings etc.
> 
> I think I have everything in the thread covered:
> http://lists.gnu.org/archive/html/automake-patches/2010-08/msg00116.html
> This is going with case (1) in that message.
> 
> However, I have only updated tests/ar.test to cope with the new
> reality. So, a lot of tests (100?) are likely to fall over due to
> the new portability warnings. The reason is that I don't know on
> what branch I should base such an intrusive patch.
You mean a patch adding AM_PROG_AR into all the tests which now requires
it, right?  I'd base that on msvc branch (in fact, I'd make it a squash-in
for your attached patch).
> I fear that such a patch, when merged elsewhere, will be incomplete
No big deal IMHO, we could resort to a fake merge in such case
(`git commit --amend' can also edit merge commits AFAIK), fixing
all the tests that need to be fixed.  And there will be few of
them anyway IMO.
> 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.

> 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
> 
> 
> From e4810ab05c41461c3b6c5638a41d8d415ebcc40f Mon Sep 17 00:00:00
> 2001 From: Peter Rosin <address@hidden>
> Date: Tue, 14 Sep 2010 14:58:17 +0200
> Subject: [PATCH] Add new 'AM_PROG_AR' macro, triggering the
> 'ar-lib' script. 
 
> diff --git a/ChangeLog b/ChangeLog
> index 02f2fcd..b911686 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,36 @@
> +2010-09-14  Peter Rosin  <address@hidden>
> +         Ralf Wildenhues  <address@hidden>
> +
> +     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.
> +     * automake.in (seen_ar): New variable.
> +     (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.  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.
> +     * tests/ar-lib3.test: New test.  Test if lib_LIBRARIES requires
> +     AM_PROG_AR.
> +     * tests/ar-lib4.test: New test.  Test if lib_LTLIBRARIES requires
> +     AM_PROG_AR.
> +     * tests/ar-lib5.test: New test.  Test if AM_PROG_AR triggers
> +     use of ar-lib when the archiver is Microsoft lib.
> +     * tests/ar-lib6.test: New test.  Test ordering of AM_PROG_AR and
> +     LT_INIT.
> +     * tests/defs.in: New required entry 'lib'.
> +     * tests/Makefile.am: Add new tests.
> +     * NEWS: Update.
> +
>  2010-09-02  Peter Rosin  <address@hidden>
> 
>       Make ar-lib support backslashed files in archives.


> --- /dev/null
> +++ b/m4/ar-lib.m4
> @@ -0,0 +1,55 @@
> +##                                                          -*-
> Autoconf -*- +# Copyright (C) 2010
> +# Free Software Foundation, Inc.
> +#
> +# 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 preserved.
> +
> +# serial 1
> +
> +# AM_PROG_AR
> +# --------------
You should add explanation of what this macro does, and, if it takes
arguments, list them in a sort of "macro prototype"; e.g.

 # AM_PROG_AR([ACT-IF-FAIL])
 # -------------------------
 # Try to determine the the archiver interface (FIXME: more details);
 # If unable to, run ACT-IF-FAIL (default: abort configure).

I guess you can come up with a better explanation ;-)

> +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)
Why not use proper m4 quoting everywhere?  E.g.:
 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'
What is the rationale for not redirecting stderr here, along with stdout?
> +      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)?
> +        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="\$(top_srcdir)/ar-lib \$(AR)"
Do you mean  am__AR="$am_aux_dir/ar-lib $AR" ?
> +  AR="$am_aux_dir/ar-lib $AR"
> +  ;;
> +esac
> +AC_SUBST([AR])dnl
> +])

> diff --git a/tests/ar-lib4.test b/tests/ar-lib4.test
> new file mode 100755
> index 0000000..c389189
> --- /dev/null
> +++ b/tests/ar-lib4.test
> @@ -0,0 +1,63 @@
> +#! /bin/sh
> +# Test if lib_LTLIBRARIES requests AM_PROG_AR.
> +
> +required=libtoolize
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cp configure.in X
> +
> +cat >> configure.in << 'END'
> +AC_PROG_CC
> +AC_PROG_RANLIB
> +AC_PROG_LIBTOOL
> +AC_OUTPUT
> +END
> +
> +cat > Makefile.am << 'END'
> +lib_LTLIBRARIES = libfoo.la
> +libfoo_la_SOURCES = foo.c
> +END
> +
> +libtoolize
> +$ACLOCAL
> +$AUTOCONF
useless autoconf call
> +AUTOMAKE_fails
> +
> +grep 'requires.*AM_PROG_AR' stderr
> +
> +cp X configure.in
> +
> +cat >> configure.in << 'END'
> +AC_PROG_CC
> +AM_PROG_AR
> +AC_PROG_RANLIB
> +AC_PROG_LIBTOOL
> +AC_OUTPUT
> +END
> +
> +$ACLOCAL
> +$AUTOCONF
another useless autoconf call
> +AUTOMAKE_fails
> +
> +grep 'ar-lib.*not found' stderr
> +
> +$AUTOMAKE --add-missing
> +
> +:
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?

> diff --git a/tests/ar-lib5.test b/tests/ar-lib5.test
> new file mode 100755
> index 0000000..f6eb5b4
> --- /dev/null
> +++ b/tests/ar-lib5.test
> @@ -0,0 +1,56 @@
> +#! /bin/sh
> +# Test if AM_PROG_AR triggers the use of the ar-lib script.
> +
> +requires=lib
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >> configure.in << 'END'
> +AC_PROG_CC
> +AM_PROG_AR
> +AC_PROG_RANLIB
> +AC_OUTPUT
> +END
> +
> +cat > Makefile.am << 'END'
> +lib_LIBRARIES = libwish.a
> +libwish_a_SOURCES = wish.c
> +END
> +
> +cat > wish.c << 'END'
> +int wish(void) { return 0; }
> +END
> +
> +$ACLOCAL
> +$AUTOCONF
> +$AUTOMAKE --add-missing --copy
> +./configure AR=lib RANLIB=:
> +
> +cat > ar-lib << 'END'
> +# /bin/sh
> +echo "It works"
> +END
> +chmod +x ar-lib
> +
> +$MAKE > stdout
> +cat stdout
> +
> +grep 'It works' stdout
> +
> +:
Hmm... grepping the output of make might be fragile...

What about rewriting the test on the following lines, to make it even
more "semantic"?
 1. Add proper call to `AC_CONFIG_AUX_DIR' in configure.in (with
    argument, say, `arlib-auxdir')
 2. Add to Makefile.am:
      check-local:
          test -f ar-lib-worked
      MOSTLYCLEANFILES = ar-lib-worked
    (the last line required for "make distcheck" below)
 3. Put the fake `ar-lib' into `arlib-auxdir'; this way, the call to
    automake should ensure that the new code respects AC_CONFIG_AUX_DIR
 4. Make the fake `ar-lib' create the file `ar-lib-worked' in the
    current builddir:
      $ cat arlib-auxdir/ar-lib
      #! /bin/sh
      :  > ar-lib-worked
 5. Run "$MAKE check" and "$MAKE distcheck"

> diff --git a/tests/ar-lib6.test b/tests/ar-lib6.test
> new file mode 100755
> index 0000000..45693dd
> --- /dev/null
> +++ b/tests/ar-lib6.test
> @@ -0,0 +1,38 @@
> +#! /bin/sh
> +# Test AM_PROG_AR ordering requirements
> +
> +required=libtoolize
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >> configure.in << 'END'
> +AC_PROG_CC
> +AC_PROG_RANLIB
> +m4_ifdef([LT_INIT], [LT_INIT], [AC_PROG_LIBTOOL])
> +AM_PROG_AR
> +END
> +
> +libtoolize
> +$ACLOCAL
> +$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 ;-)

> +
> +$EGREP '(AC_PROG_LIBTOOL|LT_INIT).*before.*AM_PROG_AR' stderr
> +
> +:

> diff --git a/tests/ar.test b/tests/ar.test
> index d68fc54..00ea69e 100755
> --- a/tests/ar.test
> +++ b/tests/ar.test
> @@ -21,6 +21,7 @@
>  set -e
> 
>  cat >> configure.in << 'END'
> +AM_PROG_AR
>  AC_SUBST([AR], ['echo it works'])
>  AC_SUBST([ARFLAGS], ['>'])
>  AC_SUBST([RANLIB], ['echo really works >>'])
> @@ -34,7 +35,7 @@ END
> 
>  $ACLOCAL
>  $AUTOCONF
> -$AUTOMAKE
> +$AUTOMAKE --add-missing
I'd rather create a dummy (empty) ar-lib above, and do not change
this line.
>  ./configure
>  $MAKE
>  grep 'it works' libfoo.a

> diff --git a/tests/defs.in b/tests/defs.in
> index af4a3cd..31003ff 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=$?
Nitpicking: why a subshell here?
> +      test $exit_status = 76 && exit 77
> +      ;;
>      makedepend)
>        echo "$me: running makedepend -f-"
>        ( makedepend -f- ) || exit 77

Thanks,
   Stefano