[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] Extended tests on AC_CONFIG_AUX_DIR.
From: |
Ralf Wildenhues |
Subject: |
Re: [PATCH 2/2] Extended tests on AC_CONFIG_AUX_DIR. |
Date: |
Wed, 15 Dec 2010 22:04:23 +0100 |
User-agent: |
Mutt/1.5.20 (2010-08-04) |
* 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
Re: [PATCH 0/2] Tests initialization: put default definition of AC_CONFIG_AUX_DIR in the pre-populated configure.in., Ralf Wildenhues, 2010/12/13