automake-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Stefano Lattarini
Subject: Re: [PATCH] Add new 'AM_PROG_AR' macro, triggering the 'ar-lib' script.
Date: Wed, 15 Sep 2010 01:45:17 +0200
User-agent: KMail/1.13.3 (Linux/2.6.30-2-686; KDE/4.4.4; i686; ; )

On Tuesday 14 September 2010, Peter Rosin wrote:
> Hi Stefano,
> 
> Den 2010-09-14 20:14 skrev Stefano Lattarini:
> > On Tuesday 14 September 2010, Peter Rosin wrote:
> [CUT]
> >> or otherwise difficult to merge due to other changes. I'm also
> >> not sure if this is the desired route. Please advise.
> > 
> > Done :-).  But let's hear what Ralf has to say, he might know
> > better.
> 
> Yes. I don't want to do that bit of work in vain.
Good idea, there's no haste after all.
 
> >> This patch is on top of the msvc branch.
> >> 
> >> AC_PROG_RANLIB is rendered obsolete by LT_INIT. Is it also
> >> rendered obsolete by AC_PROG_LIBTOOL? Should I not care about
> >> libtool 1.5?
> >> 
> >> Cheers,
> >> Peter
> >> 
> >> 
> >> diff --git a/ChangeLog b/ChangeLog
> >> +  * tests/ar-lib2.test: New test.  Test if AM_PROG_AR triggers
> >> +  install of ar-lib.
> > 
> > I feel that something like "New test, checking that AM_PROG_AR
> > triggers install of ar-lib" would sound more natural.  More
> > instances below.  This is admittedly a bike-shedding issue, so
> > feel free to ignore it.
> 
> ok
Thanks.

> >> +: ${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?

> >> +      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'
> > 
> > What is the rationale for not redirecting stdout/stderr here,
> > considered how AC_TRY_EVAL is used above (with redirection)?
> 
> No rationale at all, I just didn't see any trash. But it's better
> to redirect now than to wait for the bug report. Fixed.
As above: where is stderr redirected?

> >> +  # 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="\$(top_srcdir)/ar-lib \$(AR)"
> > 
> > Do you mean  am__AR="$am_aux_dir/ar-lib $AR" ?
> 
> I'm not sure what I mean actually. I stole shamelessly from
> m4/minuso.m4.
Yep, there are some bogus comments there too.  They seem to be a
leftover from a time when automake still placed all auxiliary scripts
in $(top_srcdir) unconditionally.
> I replaced top_srcdir with am_aux_dir for this round, but kept
> the Makefile notation, or is that totally bogus too?
I'd go with the shell syntax.  See more details below.
 
> >> diff --git a/tests/ar-lib4.test b/tests/ar-lib4.test
> >> +$AUTOCONF
> > useless autoconf call
> 
> Zapped. Also for ar-lib3.test
Nicely spotted.  Thanks.

> > This testcase checks for two distinct automake failures; I'd
> > prefer that to be done in two different tests, one checking for
> > missing `AM_PROG_AR' call, the other one for missing `ar-lib'
> > auxfile. WDYT?
> 
> I added ar-lib7.test and removed the second AUTOMAKE_fails from
> both ar-lib3.test
Thanks for spotting this too, I missed it.
> and ar-lib4.test.
 
> >> diff --git a/tests/ar-lib5.test b/tests/ar-lib5.test
> > 
> > Hmm... grepping the output of make might be fragile...
> > 
> > What about rewriting the test on the following lines, to make it
> > even more "semantic"?
> > [CUT]
> >  5. Run "$MAKE check" and "$MAKE distcheck"
> 
> I had to also add a line
>  DISTCHECK_CONFIGURE_FLAGS = AR=lib RANLIB=:
> to Makefile.am
Yes, sorry I didn't mention it explicitly.  BTW, you could have also
set DISTCHECK_CONFIGURE_FLAGS on the make command line:
  $MAKE distcheck DISTCHECK_CONFIGURE_FLAGS='AR=lib RANLIB=:'
That said, your way too is perfectly fine for the purposes of the test.
> and adjust the creation on configure.in to get
> the correct ordering.
Yes, that's an unfortunate necessity with the current `tests/defs.in'.
There's a pending patch of mine about this too, somewhere in the list
archives (reviews are welcome ;-)
 
> >> diff --git a/tests/ar-lib6.test b/tests/ar-lib6.test
> >> +$AUTOCONF 2>stderr
> >> +cat stderr
> > 
> > Please ansure the stderr is displayed even if autoconf fails:
> >   $AUTOCONF 2>stderr || { cat stderr >&2; Exit 1; }
> >   cat stderr >&2
> > 
> > Yes, I admit this idiom is annoying and should be factored out
> > somehow... patch in preparation ;-)
> 
> Ok, can you please hold that until after this is through the pipe
> though, so that I don't have to fixup when merging?
Don't worry, the patch is for the tests-init branch, so no merge problems
for you :-)  Besides, there is a good share of pending patches holding
that one back, so your patch will almost surely be applied before mine
anyway.

> >> diff --git a/tests/defs.in b/tests/defs.in
> >> +      ( $AR -? ) || exit_status=$?
> > 
> > Nitpicking: why a subshell here?
> 
> I started out by copying the cl) branch and it stuck after I
> realized that lib exited non-zero no matter what when given
> non-work options (for options that produce output that is,
> "lib -nologo" does return zero but without output so feels
> a bit uninformative).
> 
> Thanks for the review! Below is the next iteration.
And below is next round comments and annoyances ;-)

> Cheers,
> Peter
 
> diff --git a/ChangeLog b/ChangeLog
> index 02f2fcd..36adf60 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,38 @@
> +2010-09-14  Peter Rosin  <address@hidden>
> +         Ralf Wildenhues  <address@hidden>
> +
> +     Add new 'AM_PROG_AR' macro, triggering the 'ar-lib' 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: Add above.
Better to do this:
  * m4/Makefile.am (dist_m4data_DATA): Add above.
or even:
  * m4/Makefile.am (dist_m4data_DATA): Updated.
(and ditto in the git commit message).  Sorry for not noticing it in
the previous review.
> +     * automake.in (seen_ar): New variable.
s/seen_ar/$seen_ar/ (again, sorry for not noticing it before).
> +     (scan_autoconf_traces): Set it.
> +     (handle_libraries): Don't set default values for AR and ARFLAGS
> +     if AM_PROG_AR has been seen.
> +     (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/ar.test: Adjust to avoid portability warnings.
> +     * 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-lib5.test: New test, checking that AM_PROG_AR triggers
> +     use of ar-lib when the archiver is Microsoft 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/defs.in: New required entry 'lib'.
> +     * tests/Makefile.am: Add new tests.
Or better:
  * tests/Makefile.am (TESTS): Update.
(and ditto in the git commit message)
> +     * NEWS: Update.
> +

> diff --git a/m4/ar-lib.m4 b/m4/ar-lib.m4
> new file mode 100644
> index 0000000..f670f6e
> --- /dev/null
> +++ b/m4/ar-lib.m4
> @@ -0,0 +1,60 @@
> +##                                            -*- Autoconf -*-
> +# Copyright (C) 2010
> +# Free Software Foundation, Inc.
I think this should be written as:
  # Copyright (C) 2010 Free Software Foundation, Inc.
on a single line.
> +#
> +# 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 reserved.
> +
> +# serial 1
> +
> +# AM_PROG_AR
> +# --------------
This should be removed, since ...
> +# AM_PROG_AR([ACT-IF-FAIL])
> +# -------------------------
... you added this (which is correct AFAICS).
> +# Try to determine the the archiver interface and trigger the
> +# ar-lib wrapper if it is needed;
> +# If unable to, run ACT-IF-FAIL (default: abort configure).
What about this instead?
  # 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
> +AC_REQUIRE([AM_AUX_DIR_EXPAND])dnl
> +AC_REQUIRE_AUX_FILE([ar-lib])dnl
> +AC_CHECK_TOOLS([AR], [ar lib "link -lib"], [false])
> +: ${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'
Maybe s|>&AS_MESSAGE_LOG_FD|>&AS_MESSAGE_LOG_FD 2>&1| ? (not sure about 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 onftest.$ac_objext 
> >&AS_MESSAGE_LOG_FD'
Maybe s|>&AS_MESSAGE_LOG_FD|>&AS_MESSAGE_LOG_FD 2>&1| here too?
> +        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)"
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.
> +  AR="$am_aux_dir/ar-lib $AR"
> +  ;;
> +esac
> +AC_SUBST([AR])dnl
> +])

> diff --git a/tests/ar-lib5.test b/tests/ar-lib5.test
> new file mode 100755
> index 0000000..63ae2a3
> --- /dev/null
> +++ b/tests/ar-lib5.test
> @@ -0,0 +1,64 @@
> +#! /bin/sh
> +
> +requires=lib
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat > configure.in << 'END'
> +AC_INIT([ar-lib5.test], [1.0])
We prefer not to hard-code the test name if possible; what about:
  cat > configure.in << END
  AC_INIT([$me], [1.0])
instead?  This shouldn't be a problem, since the rest of the
configure.in content doesn't require to be quoted.

> +AC_CONFIG_AUX_DIR([auxdir])
> +AM_INIT_AUTOMAKE
> +AC_CONFIG_FILES([Makefile])
> +AC_PROG_CC
> +AM_PROG_AR
> +AC_PROG_RANLIB
> +AC_OUTPUT
> +END


> diff --git a/tests/ar-lib7.test b/tests/ar-lib7.test
> new file mode 100755
> index 0000000..5182894
> --- /dev/null
> +++ b/tests/ar-lib7.test
> @@ -0,0 +1,41 @@
> +#! /bin/sh
> +# Copyright (C) 2010 Free Software Foundation, Inc.
>
> +# Test if automake warns if ar-lib is missing.
Nitpicking again: s/\.$/when AM_PROG_AR is used./ ?
> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >> configure.in << 'END'
> +AC_PROG_CC
> +AC_PROG_RANLIB
> +AM_PROG_AR
> +END
> +
> +cat > Makefile.am << 'END'
> +lib_LIBRARIES = libfoo.a
> +libfoo_a_SOURCES = foo.c
> +END
>
Are these truly required?  I mean, if AM_PROG_AR does not require
AC_PROG_CC and AC_PROG_RANLIB to be called explicitly, the above
could be simplified to just:

  cat >> configure.in << 'END'
  AM_PROG_AR
  END

  : > Makefile.am

right?

> +
> +$ACLOCAL
> +AUTOMAKE_fails
> +
> +grep 'ar-lib.*not found' stderr
> +
> +$AUTOMAKE --add-missing
> +
> +:

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

Thanks,
  Stefano



reply via email to

[Prev in Thread] Current Thread [Next in Thread]