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

[Peter Rosin]

Stefano Lattarini skrev 2011-10-20 16:48:
> Hi Peter.
> 
> I will only comment the parts that still have unresolved issues ...
> 
>> diff --git a/tests/ar-lib3.test b/tests/ar-lib3.test
>> new file mode 100755
>> index 0000000..be0d6df
>> --- /dev/null
>> +++ b/tests/ar-lib3.test
>> @@ -0,0 +1,45 @@
>> +#! /bin/sh
>> +# Copyright (C) 2011 Free Software Foundation, Inc.
>> + #
>> + ... [SNIP]
>> +
>> +$ACLOCAL
>> +AUTOMAKE_fails
>> +
>> +grep '/library.am:.*requires.*AM_PROG_AR' stderr
>>
> Please remove `/library.am' from here.  Sorry for the confusion.

No problem.

>> diff --git a/tests/ar-lib4.test b/tests/ar-lib4.test
>> new file mode 100755
>> index 0000000..0232d45
>> --- /dev/null
>> +++ b/tests/ar-lib4.test
>> @@ -0,0 +1,57 @@
>> +#! /bin/sh
>> +# Copyright (C) 2011 Free Software Foundation, Inc.
>> +#
>> + ... [SNIP]
>> +
>> +grep '/library.am:.*requires.*AM_PROG_AR' stderr
>>
> Ditto.
> 
>> diff --git a/tests/ar-lib6.test b/tests/ar-lib6.test
>> new file mode 100755
>> index 0000000..af3cb2e
>> --- /dev/null
>> +++ b/tests/ar-lib6.test
>> @@ -0,0 +1,38 @@
>> +#! /bin/sh
>> +# Copyright (C) 2011 Free Software Foundation, Inc.
>> +#
>> +# 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 >&2; Exit 1; }
>> +cat stderr >&2
>> +
>> +$EGREP '(AC_PROG_LIBTOOL|LT_INIT).*before.*AM_PROG_AR' stderr
>> +
> I think it would be better to do two separate checks here, one
> for AC_PROG_LIBTOOL and one for LT_INIT.  This can be done in
> a follow-up patch, though, so no need to re-roll this test again.

The problem is that older Libtools do not have LT_INIT, so the
test which checks LT_INIT ordering would have to be skipped in
that case. That seemed like a lot of trouble compared to an
m4_ifdef.

I could keep the existing test and then do another test which
uses AC_PROG_LIBTOOL unconditionally. I assume you want to
guarantee coverage for AC_PROG_LIBTOOL in a world were LT_INIT
is the norm?

>> diff --git a/tests/ar4.test b/tests/ar4.test
>> new file mode 100755
>> index 0000000..018722d
>> --- /dev/null
>> +++ b/tests/ar4.test
>> @@ -0,0 +1,35 @@
>> +#! /bin/sh
>> +# Copyright (C) 2011 Free Software Foundation, Inc.
>> +#
>> +# Test if configure bails out if $AR does not work and AM_PROG_AR is used.
>> +
>> +. ./defs || Exit 1
>> +
>> +set -e
>> +
>> +cat >> configure.in << 'END'
>> +AM_PROG_AR
>> +END
>> +
>> +$ACLOCAL
>> +$AUTOCONF
>> +
>> +./configure AR=/bin/false 2>stderr && Exit 1
>> +cat stderr >&2
>>
> You should always display the captured stderr before exiting:
> 
>   ./configure AR=/bin/false 2>stderr && { cat stderr >&2; Exit 1; }
>   cat stderr >&2

Consider it done.

>> +
>> +grep 'configure: error: could not determine /bin/false interface' stderr
>> +
> Nice test for the rest.
> 
>> diff --git a/tests/ar5.test b/tests/ar5.test
>> new file mode 100755
>> index 0000000..60d1015
>> --- /dev/null
>> +++ b/tests/ar5.test
>> @@ -0,0 +1,38 @@
>> +#! /bin/sh
>> +# Copyright (C) 2011 Free Software Foundation, Inc.
>> +
>> +# Test the optional argument of AM_PROG_AR.
>> +
>> +. ./defs || Exit 1
>> +
>> +set -e
>> +
>> +cat >> configure.in << 'END'
>> +spy_archiver=
>> +AM_PROG_AR([spy_archiver=nope])
>> +test "$spy_archiver" = nope && echo "archiver trigger" >&2
>> +:
>> +END
>> +
>> +$ACLOCAL
>> +$AUTOCONF
>> +
>> +./configure AR=/bin/false 2>stderr
>> +cat stderr
>> +
> (Again, you should always display the captured stderr before exiting.
> But see below).

dito.

>> +grep 'archiver trigger' stderr
>> +
> Hmmm... what about simplifying the test as follows?
> 
>   cat >> configure.in << 'END'
>   AM_PROG_AR([echo ok > bad-archiver-interface-detected])
>   END
>   $ACLOCAL
>   $AUTOCONF
>   ./configure AR=/bin/false
>   test -f bad-archiver-interface-detected

Yes, nice, consider it done.

>> diff --git a/tests/defs.in b/tests/defs.in
>> index 2959f8b..5046a40 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 ) || skip_ "Microsoft \`lib' utility not 
>> available"
>> +      ;;
> Micro-nit: the subshell here shouldn't be needed.

I'll remove the brackets.

>>      makedepend)
>>        echo "$me: running makedepend -f-"
>>        ( makedepend -f- ) || exit 77
> 
> Thanks,
>   Stefano

Cheers,
Peter