* 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