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
