On Thursday 04 November 2010, Ralf Wildenhues wrote: > * Stefano Lattarini wrote on Wed, Nov 03, 2010 at 06:48:16PM CET: > > Hello Ralf. Again, just a couple of nits w.r.t. the test cases... > > Thanks; but I didn't mean to actually commit the second patch > (just in case that wasn't clear). I didn't get that, sorry. Anyway, why you don't want to commit this second patch? While its usefulness is admittedly limited, being a little bit more correct (even if only theoretically) wouldn't hurt IMHO. That said, please notice that I have no strong opionion in this matter, so if you still prefer not to commit the patch, by any means don't do that. > That said, I'll reply to your comments inline. > > > On Monday 01 November 2010, Ralf Wildenhues wrote: > > > --- /dev/null > > > +++ b/tests/remakedry.test > > > @@ -0,0 +1,82 @@ > > > > +# Make sure GNU `make -n' doesn't trigger file updates when Makefile > > > +# is out of date. > > > + > > > +# The subdir rebuilding rules rely on GNU make automatically updating > > > +# the makefile and its prerequisites (through am--refresh). > > > +required=GNUmake > > Why requiring GNU make here? After all, if a make implementation does not > > implement automatic Makefile-rebuild rules, it should trivially pass this > > test, and if implements them, it makes sense to run this test on it too. > > But that requires me to think about the test again: was this really > meant to test portable code? The rebuilding rules, esp. the am--refresh > trick, rely on GNU make-specifics, and special-casing some test of it > just because it may or may not happen to trigger the specifics doesn't > seem robust. I don't want the next small patch to this test cause a > failure for non-GNU make, a failure I'm really not interested in in this > test. > > Does that make sense? Yes. I'd still prefer to leave the GNU make requirement out, and add it back when (and if) the necessity arise, but I can see your point too. It's mostly a matter of tastes here, and since I have no strong opinion on the issue, I'm OK with keeping GNU make in $required.
> > > +cat > Makefile.am <<'END' > > > +SUBDIRS = sub > > > +END > > > +mkdir sub > > > +: > sub/Makefile.am > > > + > > > +$ACLOCAL > > > +$AUTOMAKE > > > +$AUTOCONF > > > +./configure > > > +$MAKE > > > + > > > +# The toplevel rule is not problematic, as it is not recursive. > > > +$sleep > > > +touch Makefile.in > > > +$sleep > > Is this second sleep really needed? More instances below. > > Definitely is. I want Makefile to be strictly newer so that ls -1t > is guaranteed to work not because of locale ordering but because of > different time stamps. Ah, right, I didn't think of that. Sorry. > > > +$MAKE -n Makefile > > > +test `ls -1t Makefile Makefile.in | sed 1q` = Makefile.in > > Waht about using here the `is_newest' function provided by `tests/defs'? > > That would be a viable alternative that would also allow to ditch the > second sleep, I guess (untested). Nice. > > > +# Go through contortions to avoid GNU make's first, internal "rebuild > > > Makefile > > > +# even in dry mode" cycle in the recursive (am--refresh) part. > > > +(cd sub && $MAKE -n Makefile AM_MAKEFLAGS="-n Makefile") > > I'd add a "|| Exit 1" here, just to be sure w.r.t. shells with buggy `set > > -e'. > > What bug would you be trying to work around with it? Not a specific bug, just a "potential" one (we already know how problematic `set -e' can be). Also, notice that I've written "I would add", not "We should add" ;-) so feel free to ignore the comment if it seems irrelevant or overly paranoid to you. > > One more intance below. > > > +test `ls -1t sub/Makefile config.status | sed 1q` = config.status > > > +(cd sub && $MAKE Makefile) > > > +test `ls -1t sub/Makefile config.status | sed 1q` = sub/Makefile > > > + > > > +: > > Cheers, > Ralf > Thanks, Stefano
