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

[Stefano Lattarini]

Hi Peter, thanks for not giving up ;-)

On Thursday 16 September 2010, Peter Rosin wrote:
> Den 2010-09-15 12:44 skrev Stefano Lattarini:
> >> 
> >> ">&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:
> <snip>
> 
> Facepalm
> Ok, Ralf set us both straight...
Eh eh :-)

> >> 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?
> 
> No, you go ahead. I wouldn't have had any use of it this time
> anyway since I'm not running the full testsuite just for fun.
Oh, but "maintainer-check" just run some (many) grepping checks
on the automake's sources, test scripts, makefile fragements, etc,
so it shouldn't take long to run, even on Windows.  For example,
on my Debian system:
  $ time make -k maintainer-check
  ...
  real    0m11.081s
  user    0m3.292s
  sys     0m5.580s
A full testsuite run, on the contrary, takes at least an hour.
That said...

> It takes too bloody long. If you could fix that instead, that'd
> be grand? :-)
.. I can it without problems (for the "maint" branch).  I understand
you might be fed up with this thread.

> >> 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?
> 
> No, it was not exposed previously, but your ar3 doesn't expose it
> either.
Yeah, I think that's because make implicitly defines `$(AR)' to `ar' 
when the variable is not explicitly defined; moreover, my test did 
nothing to check the definition of $(ARFLAGS).  My turn to say
"sorry for the sloppyiness", I guess ;-)
> I'm squashing in an ar3.test that exposes the issue since
> it is related to AM_PROG_AR. ar3.test failed with the code in the
> first two (broken) versions of the patch, but passes with the third
> and this fourth version.
That's good, thanks.
 
> > 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!
> 
> No problem, it's hard to do reviews.
Glad you take it the right way.

> >> 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).
> 
> Ok, but I used an absolute file name instead of fiddling with
> $PATH.
That might be problematic when the automake testsuite runs in a build 
directory whose absolute path contains e.g. spaces.  But please do not
feel forced to do Yet Another Respin of the patch: the issue can be 
easily fixed in follow-up patch (besides, I suspect there's a good 
share of scripts in our testsuite which would blow up if run with a 
space-containing abs_builddir).

> I also added your name to the ChangeLog.
There's really no need IMO, you did the serious work here.
But I won't object either ;-)

> > I've also extended them a bit, by making them check that
> > configure detects the right archiver interface.
> That's a nice addition.
Well, one doesn't spend months fiddling with the testsuite
without learning a trick or two ;-)
 
> In this version (I think) I have fixed all affected tests. I
> ventured ahead since Ralf has had enough time to scream stop if
> this is going in some totally undesired direction. I only picked
> one random test as an example for the first rounds.
> 
> The rationale for the fixes is to add AM_PROG_AR to most of the
> library using tests, but not all. In some cases I added
> -Wno-portability instead, those cases being tests related to
> problem reports (on the grounds that the report surely didn't use
> AM_PROG_AR, and that it is desired to test the original case), and
> some ar tests ensuring that some things still work even if
> AM_PROG_AR is not used.
> 
> When I have added AM_PROG_AR, I have also added ": > ar-lib" if
> that is sufficient to make the test fly, or --add-missing if that
> is required. For tests that use AUTOMAKE_fails that will just fail
> for one more reason, but uses grep to check for the original
> failure case after that, I have not bothered to adjust so that the
> test doesn't have portability problems (e.g. reqd2.test).
>
Good thinking; improvements and nitpicking can be done in follow-up 
patches anyway. There's no need to overload this already big patch.   
All in all, I agree with your decisions.
 
> >> diff --git a/tests/defs.in b/tests/defs.in
> >> +      # 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"?
> 
> "Identify output" is indeed not meant, which is why I wrote
> "identifying output", i.e. some output that can be used to identify
> the one doing the output,
Another "english fail" on my part. :-(  Sorry.
> in this case lib.exe. lib.exe has no
> explicit option that makes it print just the version, that part
> will just show up when you ask it for help with -? (or if you ask
> it to do anything really, unless you also supply the -nologo
> option of course).
> 
> I did some further tests, and if I run
>       lib -out:gazonk.lib
> it will exit with zero exit status and it will output only the logo
> part Microsoft (R) Library Manager Version 8.00.50727.762
> Copyright (C) Microsoft Corporation.  All rights reserved.
> 
> and it will also not actually create gazonk.lib. Seems like a
> winner to me, so I'm going with that.
Nice!  And the resulting check is more similar to the existing ones,
so we've also a consistency gain.


Oh, and now that I notice, another minor nit (the last one from me): 
when you edit a test script, you should update the list of years in 
the copyright notice (to do this more automatically, you can use
the aptly-named script `update-copyright' which comes with gnulib). 
*But* please, feel free not to to amend your patch yet again, since 
the copyright updating can be done in a follow-up patch without 
problems.

Thanks,
   Stefano