Re: [PATCH 2/2] Extended tests on AC_CONFIG_AUX_DIR.

[Ralf Wildenhues]

* Stefano Lattarini wrote on Wed, Dec 15, 2010 at 09:49:55PM CET:
> On Tuesday 14 December 2010, Ralf Wildenhues wrote:
> > * Stefano Lattarini wrote on Sat, Dec 11, 2010 at 03:00:34PM CET:

> > Can you update this patch to not require the previous 1/2?
> >
> Yes, the updated patch is attached.  I will push it in 72 hours, but I'd
> like to have it reviewed anew if possible.

> > > -# The "././" prefix confuses Automake into thinking it is doing a
> > > -# subdir build.  Yes, this is hacky.
> > 
> > (This comment should be retained, along with the usage below.)
> >
> Why?  Isn't such a relying on automake internals a Bad Thig?

Sure it is.

> That said, I've restored the original auxdir.test (with minor
> improvements, see the attached patch) to retain such an usage.
> What was the previous `auxdir.test' resulting from the application
> of my original patch has been moved in a new test `auxdir8.test'

OK.

> > > --- a/tests/auxdir2.test
> > > +++ b/tests/auxdir2.test
> > > @@ -25,4 +25,6 @@ set -e
> > >  : > Makefile.am
> > >  
> > >  $ACLOCAL
> > > -$AUTOMAKE
> > > +$AUTOMAKE -a
> > 
> > This changes the code paths exercised (i.e., potentially removes
> > coverage);
> >
> Not really IMHO;

It may easily, after we modify automake.in a bit.  At least it's not
far-fetched to assume that.  Granted, it requires knowing the code a
bit to see or assume that.

> the test is expected to fail anyway, and the
> `-a' just help to ensure that it fails "for the right reason"
> (i.e. use of computed AC_CONFIG_AUX_DIR, not missing auxiliary
> file).  That said ...
> 
> > please use either
> >   $AUTOMAKE -a
> >   $AUTOMAKE

> ... I've done this (the "former").

Good.

> > > +if mkdir aux; then
> > 
> > What happens with this command on w32?  I checked:
> >   MinGW mkdir fails
> >   DJGPP mkdir fails
> >   Cygwin mkdir passes
> > 
> > It seems that none of the systems actually cause harmful problems.
> >
> Good, because they shouldn't;

Sure, but that's one of the things where I'm wary.  We've had configure
tests in Libtool which froze a system, or required root access to fix,
on some of the older unices.  I simply wasn't sure the above wasn't one
of them.

> this second, conditional check...
> 
> > > +  AUTOMAKE_fails
> > > +  grep '^configure\.in:2:.*aux.*W32' stderr
> > >
> ... serves just to ensure that Automake fails with a "correct" error
> (i.e. about portability) even on platforms where the use of `aux' is
> valid, and even when an `aux' directory actually exist.

Sure.

> > > +nil=__no_such_program
> > > +unset NONESUCH || : # just to be sure
> > 
> > "just to be sure" is a fairly meaningless comment.  It actually made me
> > wonder whether you meant the "|| :" or the "unset" part with it.
> >
> I meant the "unset" part.
> 
> > I'm not sure what you want to be sure of with the unset here.
> >
> That no `NONESUCH' variable is in the environment.
> 
> I've gone with this now:
> 
>   # Make sure that we don't have a NONESUCH variable set
>   # in the environment.
>   unset NONESUCH || :

Where I'd just remove the comment, because it's redundant: the code
explains what it does.  ;-)

> > > +out=out0 $MAKE test
> > 
> > I'd write
> >   env out=out0 ...
> > 
> > here (and below), but only to make it clearer what is happening.  No big
> > deal either way, so use whatever you prefer.  (It makes a difference
> > when the command is a shell special one.)
> >
> Well, not using `env' means one less fork ;-)
> And I'm pretty confident that `make' is not going to become a shell
> builtin ;-)

OK.

> Subject: [PATCH] Extended tests on AC_CONFIG_AUX_DIR.
> 
> * tests/auxdir.test: Enable `errexit' shell flag.  Prefer `$me'
> over hard-coded test name.  Use proper m4 quoting.  Add trailing
> `:' command.
> * tests/auxdir2.test: Likewise.  Try to call automake also with
> the `-a' option, so that it will not fail for spurious reasons.
> * tests/auxdir3.test: Add an explanatory comment and a trailing
> `:' command.
> * tests/auxdir4.test: Prefer `$me' over hard-coded test name.
> Make grepping of automake stderr slightly stricter.  Also, now
> this just checks for unportable auxdir names (and it has been

Generally, prefer active voice rather than passive in log entries:
"Check for unportable foo..." (this is documented in GCS).

FWIW I'd write  s/auxdir/auxiliary directory/

> extended in this respect).  Checks for non-existent auxdirs has

also here (...ies)

> been moved out to ...
> * tests/auxdir5.test: .. this new test.
> * tests/auxdir6.test: New test.
> * tests/auxdir7.test: Likewise.
> * tests/auxdir8.test: Likewise.
> * tests/auxdir9.test: Likewise.
> * tests/Makefile.am (TESTS): Updated.

> --- a/tests/auxdir2.test
> +++ b/tests/auxdir2.test

> +# Both these two invocations are meant.

Comment writing is hard!  :-)
The above still doesn't explain *why* both are done, so while it does
greater than zero information, it could still be better.  What about
this?

  # Exercise both code paths concerning auxiliary files.

> +$AUTOMAKE -a
>  $AUTOMAKE

Patch is ok then.

Thanks,
Ralf