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

[Peter Rosin]

Hi Stefano,

Thanks for the review!


Stefano Lattarini skrev 2011-10-20 11:01:
> 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

I used ar-lib as the topic, since this is about triggering that 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.

done

>> * 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.

done, I went with "unusual".

>> +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?

Not yet.  It would take a faked 'ar' which does not work.

*time passes*

I added that in the new tests ar4.test and ar5.test.

>>  @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.

done

>> 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.

I added a comment. The reason you need two is because Libtool < 2.0 didn't have
LT_INIT. The reason AM_PROG_LIBTOOL isn't needed is because that macro is
just an alias for AC_PROG_LIBTOOL, and has been for so long (since Libtool
1.2f, 1999) that AM_PROG_LIBTOOL is not interesting, agreed?

>> [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?

Since this test is not responsible for the whole of configure.in, that
additions to the greps will cause ripples when the template configure.in
is changed.  I don't like that, so I added a grep for "configure.in"
in the beginning of the line, but no line number.

>> +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'.

Hmm, no, because there is no line number output. The warning is

/home/peda/automake/lib/am/library.am: `libfoo.a': linking libraries using a 
non-POSIX
/home/peda/automake/lib/am/library.am: archiver requires `AM_PROG_AR' in 
`configure.in'

Maybe that is not good enough, but I don't know how to change it
to include the line number.
I added a grep for the "/library.am" part.

>> 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/

right

>> +# 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

I don't agree, the test then involves the ar-lib script, but it
is intended to test the AM_PROG_AR macro. Also, your version will
create ar-lib-worked when configure checks the archiver interface
which I think will cause trouble.

>> +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).

There's nothing like testing with the real thing. I'm reluctant to ditch
that test, but you might of course override that if you feel strongly...

>> [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?

Same trouble as above with not controlling the whole configure.in
file. Same remedy...

>> +$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"

Yes, nice.

>> +      ;;
>>      makedepend)
>>        echo "$me: running makedepend -f-"
>>        ( makedepend -f- ) || exit 77
> 
>>
>> [MEGA-SNIP]
>>
> 
> Thanks,
>   Stefano

New version of the patch coming up soon.

Cheers,
Peter