On Thursday 20 October 2011, Peter Rosin wrote:
> Hi Stefano,
>
> Thanks for the review!
>
> Stefano Lattarini skrev 2011-10-20 11:01:
> > Micro-nit: we've started to write all the git commit summaries according
> > to the format "<topic>: <message>"
>
> I used ar-lib as the topic, since this is about triggering that script.
>
Perfect!
> >> +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.
>
Thanks.
> >>
> >> 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.
>
I know. I was asking something else: why do you need to ensure AM_PROG_AR is
called before LT_INIT? A comment explaining that would be worthwhile IMHO.
(BTW, sorry if I failed to make myself properly understood, and thanks for
bearing with me).
> 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?
>
OK.
> >> +# 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.
>
Agreed, that should be good enough.
> >> +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'
>
Oh, ok, similar to what happens e.g. when one uses 'subdir-objects'
option without having AM_PROG_CC_C_O in configure.ac.
> 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.
>
No need to, that should remain an internal detail. Your previous version
of the test was correct, so please revert back to it. Also, the error
message can be improved by later patches if the need arise (my guess is
that it won't). Sorry for the noise.
> >> 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.
> >> +# ...
> >> +# 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.
>
Hmmm... OK, than I've not really understood what the test is supposed
to check. So Let's just keep it as it is now, prospective improvements
and fixes can be done in a follow-up patch.
> >> 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...
>
Given that I was confused about `ar-lib5b.test', I'm probably confused
about this test too. So for the moment keep it as-is; I will re-propose
updates and cleanups once (and if) I've a better understanding of these
tests. Sorry for the noise.
> >> 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.
>
After doing a `s/ no / not /' ;-)
> New version of the patch coming up soon.
>
Looking forward to it :-)
Thanks,
Stefano
