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

[Stefano Lattarini]

On Wednesday 15 September 2010, Peter Rosin wrote:
> Hi Stefano,
> 
> Den 2010-09-15 01:45 skrev Stefano Lattarini:
> >>>> +: ${AR=ar}
> >>>> +
> >>>> +AC_CACHE_CHECK([the archiver ($AR) interface],
> >>>> [am_cv_ar_interface], +  [am_cv_ar_interface=ar
> >>>> +   AC_COMPILE_IFELSE([[int some_variable = 0;]],
> >>>> +     [am_ar_try='$AR cru libconftest.a conftest.$ac_objext 
> >>>> >&AS_MESSAGE_LOG_FD'
> >>> 
> >>> What is the rationale for not redirecting stderr here, along
> >>> with stdout?
> >> 
> >> But stderr is redirected?
> > 
> > Where?  Am I mising something?
> 
> ">&foo" is the same as ">foo 2>&1", or what am I missing?
No, "&>foo" is the same as ">foo 2>&1" on bash and zsh (at least), but
is not portable. ">&n" redirects the stdout to file descriptor n, which
must be an open file descriptor:

  $ ksh -c 'echo x >&foo'
  ksh[1]: foo: bad file unit number
  $ ksh -c 'echo x >&9'
  ksh[1]: 9: cannot open [Bad file descriptor]
  $ ksh -c 'echo x >&2' 2>/dev/null
  $ ksh -c 'echo x >&2' >/dev/null
  x

In our case, AS_MESSAGE_LOG_FD is an autoconf macro expanding to a
proper file descriptor number, so the right way to do the redirection
of both stderr and stdout to configure log is:
  COMMAND >&AS_MESSAGE_LOG_FD 2>&1

BTW, bash and zsh try to be smart (and fail, IMVHO) by treating
">&foo" as "&>foo" *iff* foo is not a number:

 $ bash -c 'cp >&foo'
 $ cat foo
 cp: missing file operand
 Try `cp --help' for more information.
 $ bash -c 'cp >&foo'
 $ bash -c 'cp >&13'
 bash: 13: Bad file descriptor
 $ bash -c 'cp >&3' 3>&2
 cp: missing file operand
 Try `cp --help' for more information.

> [MEGA CUT]

>>> +  # and then we could set am__AR="\$(am_aux_dir)/ar-lib \$(AR)"
>> What about:
>>  ``... and then we could set am__AR="$am_aux_dir/ar-lib \$(AR)" or
>>    something similar''.
>> instead?  This seems somewhat more correct, without forcing us to be
>> overly precise and explicit.
> I also changed \$(AR) to $AR and went with your version.
But the \$(AR) was meant, so that the user could override AR (but not
am__AR) at make time ;-)

> [MEGA CUT]

> >> diff --git a/tests/defs.in b/tests/defs.in
> >> index af4a3cd..bd2e813 100644
> >> --- a/tests/defs.in
> >> +++ b/tests/defs.in
> >> @@ -156,6 +156,16 @@ do
> >> 
> >>        echo "$me: running $CC -V -help"
> >>        ( $CC -V -help ) || exit 77
> >>        ;;
> >> 
> >> +    lib)
> >> +      AR=lib
> >> +      export AR
> >> +      # There is no way to get any identifying output with
> >> +      # a zero exit status. So, remap exit status 76 to 0.
> >> +      echo "$me: running $AR -?"
> >> +      exit_status=0
> >> +      $AR -? || exit_status=$?
> > 
> > Very paranoid nitpicking, but... what about using "$AR -\?", to
> > be sure not to trigger unexpected file globbing?
> I added the -\? thing, and for the record I did try to make
> the same change for the cl test above in the same file, but that
> is no use since cl does the globbing itself (as Windows apps are
> supposed to),
Oh.
> but it conveniently(?) does so before parsing options.
LOL :-)
> lib does its globbing after parsing options apparently.

> > 
> >> +      test $exit_status = 76 && exit 77
> > 
> > I'm not sure I understand the semantic here; an exit status of 76
> > means you have the expected `lib' program... and in this case you
> > skip the test because it requires `lib' itself?  (BTW, sorry for
> > not having noticed this in my prevous review).
> 
> Arrgh. I had requires=lib in ar-lib5.test instead of required=lib,
> so it didn't really check if lib was there at all, it just blindly
> used it.
Probably a new maintainer-check rule would help here, by catching
assigement to (say) `requires' and `require' instead of `required'.
Do you want to try to write it?
> So the above was totally untested dead code. In this
> iteration I have tested that the test skips if lib is missing.
> Sorry for being so sloppy.
No problem.  Moreover, I could have applied your patch on my tree,
and I'd have noticed the failure right away.

> I have also added some words about the optional argument in the
> documentation, and I noticed that AM_PROG_AR does not handle
> ARFLAGS (which I thought it did), so I adjusted the code in
> automake.in:handle_libraries accordingly.
Did the tests failed on this?  If not, this is a testsuite
weakness that should be fixed.  The best way to do so IMO would
be to temporarily revert you fix to handle_libraries, add a new
test exposing the bug (ar3.test), check that the test fails,
re-apply your fix, and check that it passes.  Does the attached
test script `ar3.test' succeed in doing this?

> Cheers,
> Peter

Some more nits and not-so-nits below...

BTW, sorry if I seem overly nitpicking; please do not mistake this
attitude as a belittling of you or your good work!

> diff --git a/NEWS b/NEWS
> index 6971bd7..41a6cc8 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -8,6 +8,9 @@ New in 1.11.0a:
>    - The `compile' script now converts some options for MSVC for a
>      better user experience.  Similarly, the new `ar-lib' script wraps
>      Microsoft lib.
> 
> +  - New macro AM_PROG_AR that looks for an archiver and wraps it in the new
> +    'ar-lib' auxiliary script if the found archiver is Microsoft lib.
> +
Also, use of AM_PROG_AR is now *required* when you use LIBRARIES or
LTLIBRARIES primaries with -Werror in AUTOMAKE_OPTIONS.  This is a
backward-incompatibility that IMO should be clearly noted in NEWS.

> diff --git a/m4/ar-lib.m4 b/m4/ar-lib.m4
> new file mode 100644
> index 0000000..9e127e3
> --- /dev/null
> +++ b/m4/ar-lib.m4
> +
> +AC_CACHE_CHECK([the archiver ($AR) interface],
> [am_cv_ar_interface], +  [am_cv_ar_interface=ar
> +   AC_COMPILE_IFELSE([[int some_variable = 0;]],
> +     [am_ar_try='$AR cru libconftest.a conftest.$ac_objext 
> >&AS_MESSAGE_LOG_FD'
s/>&AS_MESSAGE_LOG_FD/>&AS_MESSAGE_LOG_FD 2>&1/ (now I think we agree on this)
> +      AC_TRY_EVAL([am_ar_try])
> +      if test "$ac_status" -eq 0; then
> +        am_cv_ar_interface=ar
> +      else
> +        am_ar_try='$AR -NOLOGO -OUT:conftest.lib conftest.$ac_objext 
> >&AS_MESSAGE_LOG_FD'
Ditto.
> +        AC_TRY_EVAL([am_ar_try])
> +        if test "$ac_status" -eq 0; then
> +          am_cv_ar_interface=lib
> +        else
> +          m4_default([$1],
> +            [AC_MSG_ERROR([could not determine $AR interface])])
> +        fi
> +      fi
> +      rm -f conftest.lib libconftest.a
> +     ])
> +   ])
> +
> +case $am_cv_ar_interface in
> +ar)
> +  ;;
> +lib)
> +  # Microsoft lib, so override with the ar-lib wrapper script.
> +  # FIXME: It is wrong to rewrite AR.
> +  # But if we don't then we get into trouble of one sort or another.
> +  # A longer-term fix would be to have automake use am__AR in this case,
> +  # and then we could set am__AR="$am_aux_dir/ar-lib $AR"
s/$AR/\$(AR)/
> +  # or something  # similar.
> +  AR="$am_aux_dir/ar-lib $AR"
> +  ;;
> +esac
> +AC_SUBST([AR])dnl
> +])

> diff --git a/tests/ar-lib5.test b/tests/ar-lib5.test
What about adding also a sister test for ar-lib5.test, which would
use a `lib' script that fakes the Microsoft lib interface?  Such a
test could also run on Unix, thus improving coverage.

I've gone ahead and tried this idea; see the attached scripts
ar-lib5a.test (which is basically your old ar-lib5.test) and
ar-lib5b.test (which fakes microsoft lib).

I've also extended them a bit, by making them check that configure
detects the right archiver interface.

> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 01acd76..ecc6ab6 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -89,6 +89,12 @@ ansi8.test \
>  ansi9.test \
>  ansi10.test \
>  ar-lib.test \
> +ar-lib2.test \
> +ar-lib3.test \
> +ar-lib4.test \
> +ar-lib5.test \
This might have to be adjusted to ar-lib5a.test and ar-lib5b.test,
if you decide to follow my idea above.
> +ar-lib6.test \
> +ar-lib7.test \
>  ar.test \
>  ar2.test \
Please add ar3.test here if it works.

> diff --git a/tests/ar.test b/tests/ar.test
I think you could fix also ar2.test right away, since you are at it.

> diff --git a/tests/defs.in b/tests/defs.in
> index af4a3cd..3e2235b 100644
> --- a/tests/defs.in
> +++ b/tests/defs.in
> @@ -156,6 +156,16 @@ do
>        echo "$me: running $CC -V -help"
>        ( $CC -V -help ) || exit 77
>        ;;
> +    lib)
> +      AR=lib
> +      export AR
> +      # There is no way to get any identifying output with
> +      # a zero exit status. So, remap exit status 76 to 0.
> +      echo "$me: running $AR -?"
> +      exit_status=0
> +      $AR -\? || exit_status=$?
> +      test $exit_status = 76 || exit 77
> +      ;;
Since the `errexit' shell flag is not active in `tests/defs', this could
be simplified even more, as e.g.:
   # Microsoft lib exit with status `76 'when asked to identify output.
   echo "$me: running $AR -?"
   $AR -\?
   test $? = 76 || exit 77
Also, is "identify output" really meant, or would it better written as
"identify version"?

-*-*-

Thanks for working of this, and for you patience.

Regards,
   Stefano

ar3.test
Description: application/shellscript

ar-lib5b.test
Description: application/shellscript

ar-lib5a.test
Description: application/shellscript