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

[Stefano Lattarini]

Hi Peter, thanks for not giving up on this.

On Wednesday 19 October 2011, Peter Rosin wrote:
> From 68bdc56f54aeb497200598ccab1f5e9f9616c556 Mon Sep 17 00:00:00 2001
> From: Peter Rosin <address@hidden>
> Date: Wed, 19 Oct 2011 19:33:58 +0200
> Subject: [PATCH 1/2] Add new 'AM_PROG_AR' macro, triggering the 'ar-lib'
>  script.
>
Micro-nit: we've started to write all the git commit summaries according
to the format "<topic>: <message>" (no trailing full stop); so what about
rewriting the summary as:

  ar: new 'AM_PROG_AR' macro, triggering the 'ar-lib' script

or:

  lib: 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 (dist_m4data_DATA): Update.
> * automake.in ($seen_ar): New variable.
> (scan_autoconf_traces): Set it.
> (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/aclibobj.test: Adjust to new portability requirements due
> to the new AM_PROG_AR macro.
> * tests/aclocal4.test: Likewise.
> * tests/ansi10.test: Likewise.
> [SNIP]
> * tests/vala1.test: Likewise.
>
Small nit: wouldn't it be better to substitute this looong list with
simply the following?

  * All relevant tests: Adjust to new portability requirements due
  to the new AM_PROG_AR macro.

This should be clearer, and IMHO it would still obey the GNU coding
standards -- see:
  <http://www.gnu.org/prep/standards/html_node/Simple-Changes.html>

Ditto for the ChangeLog entry, of course.

> * 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-lib5a.test: New test, checking that AM_PROG_AR triggers
> use of ar-lib when the archiver is Microsoft lib.
> * tests/ar-lib5b.test: New test, checking that AM_PROG_AR triggers
> use of ar-lib when the archiver is a faked 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/ar3.test: New test, checking that AR and ARFLAGS may
> be overridden by the user even if AM_PROG_AR is used.
> * tests/defs.in: New required entry 'lib'.
> * tests/Makefile.am (TESTS): Update.
> * NEWS: Update.
> 
> Signed-off-by: Peter Rosin <address@hidden>
>
> [SNIP]
>
> diff --git a/doc/automake.texi b/doc/automake.texi
> index ce1bdbb..5d0942d 100644
> --- a/doc/automake.texi
> +++ b/doc/automake.texi
> @@ -3945,6 +3945,15 @@ environment, or use the @option{--with-lispdir} option 
> to
>  @command{configure} to explicitly set the correct path (if you're sure
>  you have an @command{emacs} that supports Emacs Lisp).
>  
> address@hidden AM_PROG_AR(@ovar{act-if-fail})
> address@hidden AM_PROG_AR
> address@hidden AR
> +You must use this macro when you use the archiver in your project, if
> +you want support for weird archivers such as Microsoft lib.
>
Micro-nit: wouldn't "unusual archivers" or "non-standard archivers" or
even "archivers that are not compatbile with ar" be better than "weird
archivers"?  One more instance below.

> +The content of the optional argument is executed if the archiver
> +interface is not recognized; the default action is to abort configure
> +with an error message.
> +
Do we have tests checking these behaviours?

>  @item AM_PROG_AS
>  @acindex AM_PROG_AS
>  @vindex CCAS
> @@ -4568,6 +4577,7 @@ AC_INIT([hand], [1.2])
>  AC_CONFIG_AUX_DIR([.])
>  AM_INIT_AUTOMAKE
>  AC_PROG_CC
> +AM_PROG_AR
>  AC_PROG_RANLIB
>  AC_CONFIG_FILES([Makefile])
>  AC_OUTPUT
> @@ -5008,11 +5018,12 @@ by invoking @samp{$(AR) $(ARFLAGS)} followed by the 
> name of the
>  library and the list of objects, and finally by calling
>  @samp{$(RANLIB)} on that library.  You should call
>  @code{AC_PROG_RANLIB} from your @file{configure.ac} to define
> address@hidden (Automake will complain otherwise).  @code{AR} and
> address@hidden default to @code{ar} and @code{cru} respectively; you
> -can override these two variables my setting them in your
> address@hidden, by @code{AC_SUBST}ing them from your
> address@hidden, or by defining a per-library @code{maude_AR}
> address@hidden (Automake will complain otherwise).  You should also
> +call @code{AM_PROG_AR} to define @code{AR}, in order to support weird
> +archivers.
>
I think explicitly referencing "Microsoft lib" here would be worthwhile.

> address@hidden will default to @code{cru}; you can override
> +this variable by setting it in your @file{Makefile.am} or by
> address@hidden it from your @file{configure.ac}.  You can override
> +the @code{AR} variable by defining a per-library @code{maude_AR}
>  variable (@pxref{Program and Library Variables}).
>  
>  @cindex Empty libraries
>
> [SNIP]
>
> diff --git a/m4/ar-lib.m4 b/m4/ar-lib.m4
> new file mode 100644
> index 0000000..822ca60
> --- /dev/null
> +++ b/m4/ar-lib.m4
> @@ -0,0 +1,58 @@
> +##                                                          -*- Autoconf -*-
> +# Copyright (C) 2011 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([ACT-IF-FAIL])
> +# -------------------------
> +# 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
>
Why are these two `AC_BEFORE' is required?  An explanatory comment would be
nice.  Also, you might want to add `AM_PROG_LIBTOOL' to the list.

> [SNIP]
>
> diff --git a/tests/ar-lib2.test b/tests/ar-lib2.test
> new file mode 100755
> index 0000000..e6372e0
> --- /dev/null
> +++ b/tests/ar-lib2.test
> @@ -0,0 +1,40 @@
> +#! /bin/sh
> +# Copyright (C) 2011 Free Software Foundation, Inc.
> +# ...
> +# Test if AM_PROG_AR installs ar-lib.
> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >> configure.in << 'END'
> +AC_PROG_CC
> +AM_PROG_AR
> +END
> +
> +cat > Makefile.am << 'END'
> +bin_PROGRAMS = wish
> +wish_SOURCES = a.c
> +END
> +
> +$ACLOCAL
> +$AUTOMAKE --add-missing 2>stderr || { cat stderr >&2; Exit 1; }
> +cat stderr >&2
> +# Make sure ar-lib is installed, and that Automake says so.
> +grep 'install.*ar-lib' stderr
>
Would grepping also `FILE:LINENO' in the message be worthwhile?

> +test -f ar-lib
>
Note to self: add checks on `AM_PROG_AR' and `ar-lib' in
`add-missing.test' once we merge to testsuite-work.

> diff --git a/tests/ar-lib3.test b/tests/ar-lib3.test
> new file mode 100755
> index 0000000..6bcf6c2
> --- /dev/null
> +++ b/tests/ar-lib3.test
> @@ -0,0 +1,45 @@
> +#! /bin/sh
> +# Copyright (C) 2011 Free Software Foundation, Inc.
> +# ...
> +# Test if lib_LIBRARIES requests AM_PROG_AR.
> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >> configure.in << 'END'
> +AC_PROG_CC
> +AC_PROG_RANLIB
> +END
> +
> +cat > Makefile.am << 'END'
> +lib_LIBRARIES = libfoo.a
> +libfoo_a_SOURCES = foo.c
> +END
> +
> +$ACLOCAL
> +AUTOMAKE_fails
> +
> +grep 'requires.*AM_PROG_AR' stderr
> +
Small nit: could you also grep for `FILENAME:LINENO' in the error
message?  As in:

  grep '^Makefile\.am:1:.*require.*AM_PROG_AR' stderr

Similarly for test `ar-lib4.test'.

> diff --git a/tests/ar-lib5b.test b/tests/ar-lib5b.test
> new file mode 100755
> index 0000000..bf9073e
> --- /dev/null
> +++ b/tests/ar-lib5b.test
> @@ -0,0 +1,83 @@
> +#! /bin/sh
> +# Copyright (C) 2011 Free Software Foundation, Inc.
> +# ...
> +# Test if AM_PROG_AR triggers the use of the ar-lib script.
> +# This test does not require Micrsoft lib.
>
Typo here: s/Micrsoft/Microsoft/

> +# Keep this test in sync with sister test `ar-lib5b.test'.
> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat > configure.in << END
> +AC_INIT([$me], [1.0])
> +AC_CONFIG_AUX_DIR([auxdir])
> +AM_INIT_AUTOMAKE
> +AC_CONFIG_FILES([Makefile])
> +AC_PROG_CC
> +AM_PROG_AR
> +AC_PROG_RANLIB
> +# We want to test the content of am_cv_ar_interface in the Makefile.
> +AC_SUBST([am_cv_ar_interface])
> +AC_OUTPUT
> +END
> +
> +cat > Makefile.am << 'END'
> +lib_LIBRARIES = libwish.a
> +libwish_a_SOURCES = wish.c
> +
> +check-local:
> +     test x'$(am_cv_ar_interface)' = x'lib'
> +     test -f ar-lib-worked
> +MOSTLYCLEANFILES = ar-lib-worked
> +END
> +
> +cat > wish.c << 'END'
> +int wish(void) { return 0; }
> +END
> +

> +mkdir auxdir
> +cat > auxdir/ar-lib << 'END'
> +# /bin/sh
> +:> ar-lib-worked
> +END
> +chmod +x auxdir/ar-lib
> +
I think this should be removed ...

> +# Let's fake microsoft lib.
> +mkdir bin
> +cat > bin/lib << 'END'
> +# /bin/sh
> +case " $* " in
> +  *' -OUT:'*) exit 0;;
> +  *' cru '*) exit 1;;
> +  *) : > ar-lib-worked;;
> +esac
>
... and this should be substituted by:

  # Let's fake Microsoft lib.
  mkdir bin
  cat > bin/lib << 'END'
  # /bin/sh
  case " $* " in
        *\ c*) exit 1;;            # ar-lib failed to munge the command line.
    *\ -OUT:*) : > ar-lib-worked;; # ar-lib munged the command line.
            *) exit 1;;            # ar-lib failed to munge the command line.
  esac

> +END
> +chmod +x bin/lib
> +
> +$ACLOCAL
> +$AUTOCONF
> +$AUTOMAKE --add-missing
> +
> +# Sanity check: test that it is ok to use `am_cv_ar_interface' as we do.
> +$FGREP 'am_cv_ar_interface=' configure
> +
> +./configure AR=bin/lib RANLIB=:
> +
> +$MAKE check
> +$MAKE distcheck DISTCHECK_CONFIGURE_FLAGS="AR=`pwd`/bin/lib RANLIB=:"
> +
> +:

> diff --git a/tests/ar-lib5a.test b/tests/ar-lib5a.test
> new file mode 100755
> index 0000000..1c63bbb
> --- /dev/null
> +++ b/tests/ar-lib5a.test
>
> [SNIP]
>
Isn't this test overkill in light of `ar-lib5b.test'?  I'd say to just
remove it (and then rename "ar-lib5a.test" -> "ar-lib5.test", for
simplicity).

> [SNIP]
>
> diff --git a/tests/ar-lib7.test b/tests/ar-lib7.test
> new file mode 100755
> index 0000000..c4b0b02
> --- /dev/null
> +++ b/tests/ar-lib7.test
> @@ -0,0 +1,36 @@
> +#! /bin/sh
> +# Copyright (C) 2011 Free Software Foundation, Inc.
> +# ...
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test if automake warns if ar-lib is missing when AM_PROG_AR is used.
> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >> configure.in << 'END'
> +AM_PROG_AR
> +END
> +
> +:> Makefile.am
> +
> +$ACLOCAL
> +AUTOMAKE_fails
> +
> +grep 'ar-lib.*not found' stderr
> +
Same small nit: also grep `FILENAME:LINENO' in the error message?

> +$AUTOMAKE --add-missing
> +
> +:

> [SNIP]

> diff --git a/tests/defs.in b/tests/defs.in
> index 2959f8b..9a6fb10 100644
> --- a/tests/defs.in
> +++ b/tests/defs.in
> @@ -278,6 +278,14 @@ do
>        echo "$me: running javac -version -help"
>        javac -version -help || exit 77
>        ;;
> +    lib)
> +      AR=lib
> +      export AR
> +      # Attempting to create an empty archive will actually not
> +      # create the archive, but lib will output its version.
> +      echo "$me: running $AR -out:defstest.lib"
> +      ( $AR -out:defstest.lib ) || exit 77
>
I suggest to substitute this line with the following:

 $AR -out:defstest.lib || skip_ "Microsoft \`lib' utility no available"

> +      ;;
>      makedepend)
>        echo "$me: running makedepend -f-"
>        ( makedepend -f- ) || exit 77

>
> [MEGA-SNIP]
>

Thanks,
  Stefano