* 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
