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 <p...@lysator.liu.se> >> 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 <p...@lysator.liu.se> >> >> [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). >> >> +@item AM_PROG_AR(@ovar{act-if-fail}) >> +@acindex AM_PROG_AR >> +@vindex 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 >> -@code{RANLIB} (Automake will complain otherwise). @code{AR} and >> -@code{ARFLAGS} default to @code{ar} and @code{cru} respectively; you >> -can override these two variables my setting them in your >> -@file{Makefile.am}, by @code{AC_SUBST}ing them from your >> -@file{configure.ac}, or by defining a per-library @code{maude_AR} >> +@code{RANLIB} (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 >> +@code{ARFLAGS} will default to @code{cru}; you can override >> +this variable by setting it in your @file{Makefile.am} or by >> +@code{AC_SUBST}ing 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
