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

[Stefano Lattarini]

On Wednesday 15 December 2010, Ralf Wildenhues wrote:
> * 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:
> > > > +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.
>
LOL, I didn't suspect that could really happen :-/

> 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.  ;-)
>
True enough.  Comment removed.

> > > > +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/
>
OK, adjusted.  Even if to be really precise we should call it at least
"directory of auxiliary scripts" ;-)

> > extended in this respect).  Checks for non-existent auxdirs has
> 
> also here (...ies)
>
Adjusted.

> > 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.
> 
I fixed also some other typos and grammaros in the ChangeLog entry; see
the attached squash-in fore more info.

> > --- 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,
>
Well, before reading your latest reply, I could have written only
"Both these two invocations are meant, because Ralf said so." ;-)

> so while it does greater than zero information, it could still
> be better.  What about this?
> 
>   # Exercise both code paths concerning auxiliary files.
>
Better; added.

> > +$AUTOMAKE -a
> >  $AUTOMAKE
> 
> Patch is ok then.
> 
I'll push it tomorrow if there are no further objections.

Thanks,
   Stefano

diff --git a/ChangeLog b/ChangeLog
index 60c80ca..f18c0ff 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -10,10 +10,11 @@
        `:' 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
-       extended in this respect).  Checks for non-existent auxdirs has
-       been moved out to ...
-       * tests/auxdir5.test: .. this new test.
+       this test just checks about Automake's reaction to unportable
+       auxiliary directory names (and it has been extended in this
+       respect).  Moved the checks about non-existent auxiliary
+       directories to ...
+       * tests/auxdir5.test: ... this new test.
        * tests/auxdir6.test: New test.
        * tests/auxdir7.test: Likewise.
        * tests/auxdir8.test: Likewise.
diff --git a/tests/auxdir2.test b/tests/auxdir2.test
index 7936a2b..430abad 100755
--- a/tests/auxdir2.test
+++ b/tests/auxdir2.test
@@ -32,6 +32,7 @@ END
 
 $ACLOCAL
 # Both these two invocations are meant.
+# They exercise both code paths concerning auxiliary files.
 $AUTOMAKE -a
 $AUTOMAKE
 
diff --git a/tests/auxdir9.test b/tests/auxdir9.test
index 3ee1dbc..db85ac1 100755
--- a/tests/auxdir9.test
+++ b/tests/auxdir9.test
@@ -22,8 +22,7 @@
 set -e
 
 nil=__no_such_program
-# Make sure that we don't have a NONESUCH variable set
-# in the environment.
+
 unset NONESUCH || :
 
 cat >>configure.in << END