On Monday 10 January 2011, Ralf Wildenhues wrote: > Hi Stefano, > > sorry for the delay. > > * Stefano Lattarini wrote on Tue, Jan 04, 2011 at 03:41:45PM CET: > > On Monday 03 January 2011, Ralf Wildenhues wrote: > > > * Stefano Lattarini wrote on Mon, Jan 03, 2011 at 02:38:48PM CET: > > > > Subject: [PATCH] Improve, extend and tweak tests on Texinfo support. > [...] > > > > OK with nits addressed. > > > > > I have a couple of questions below; I'll wait to push until they've > > been addressed. > > > > > --- /dev/null > > > > +++ b/tests/comments-in-var-definition.test > > > > > > How about s/definition/defn/? That is still unique, easily understood, > > > and a lot shorter. > > > > > Fine with me (even if I still don't understand this bias against longer > > test names ;-) > > The longer the names, and the more the tests, the earlier we will exceed > the command line length limit in our 'check' rules (important to fix on > all systems it happens) and our 'distdir' rule (important at least for > the maintainer's machine). > Ouch, I never tought about these issues :-(
> So, support for more than one parallel-tests testsuite per Makefile.am > is needed soonish. > Or better (if possible) finding out a way to transparently avoid commandline-lenght issues when calling $(MAKE) recursively. There was a previous attempt of yours at this IIRC, but it didn't work out. Maybe it's time to give it a second shot? > Besides, while I agree > that the 8+3 names are often lacking descriptiveness, I also don't like > typing too much. > But how often do you type the name of the testcases after all? (I mean, without the help of tab completion of course ;-). > For example, I'm not sure why we named the 'posixsubst*.test' files > that way; there is little specifically posixy about these substitution > rules. > Well, they are the only POSIX-mandated textual substitutions for make macros, so I thought the test names were appropriated -- or am I missing something? > > > > +grep '^am__TEXINFO_TEX_DIR *=.*[/ ]tex *$' Makefile.in > > > > +$EGREP 'am__TEXINFO_TEX_DIR.*=.*(comment|#)' Makefile.in && Exit 1 > > > > > > These two lines access internal details that could change. Acceptable > > > if it must be that way but better if we can do without. > > > > > I added those lines to avoid reducing coverage in the code I moved > > from `txinfo22.test' -- which indeed had a check: > > test -d "$(am__TEXINFO_TEX_DIR)" > > in its Makefile.am. > > Oh, ok, I didn't realize we were already using internal details before. > > > Hhmm... but maybe it would be simpler & safer to just add back that check > > (and new similar ones) in the Makefile.am of `comments-in-var-defn.test'? > > I think so; here is what I'll squash in if there are no objections: > > Fine as well, but I don't see how it makes much of a difference. > am__TEXINFO_TEX_DIR is still an internal detail. > Yes, but this new amended version of the checks is more similar to the old version in txinfo22.test. Not a big deal, but since it's already done, let's keep it. > > > Hmm, the autoconf.texi blurb on `touch' states that this is no longer > > > a practical issue, but IIRC the policy was still enforced in GCC > > > sources, making me wonder whether there still are broken systems out > > > there ... > > > > > > Anyway, you can easily avoid the issue by > > > echo stamp > ... > > > > > I'd prefer to use `touch' if that's ok with you, since it makes the > > purpose of the commands even clearer (and is used in other parts of > > the automake testsuite). Objections? > > Well, that's worse in that it has the same issue on those ancient > systems. Oh well, maybe I should stop caring about them. > Well, that's what the autoconf manual suggest ;-) [see the last line in the excerpt below] ``On ancient BSD systems, touch or any command that results in an empty file does not update the timestamps, so use a command like echo as a workaround. Also, GNU touch 3.16r (and presumably all before that) fails to work on SunOS 4.1.3 when the empty file is on an NFS-mounted 4.2 volume. However, these problems are no longer of practical concern.'' FTM I'm pushing this hunk as is; you are obviously free to amend it if you think portability to those ancient system is still important. > > > > +day=`LC_ALL=C date '+%d'` || Exit 77 > > > > +month=`LC_ALL=C date '+%B'` || Exit 77 > > > > +year=`LC_ALL=C date '+%Y'` || Exit 77 > > > > > > Not all shells propagate exit status from commands substitutions in > > > assignments (see autoconf.texi Assignments). > > Hmpf :-( > > > > Luckily this issue seems of little pratical concern at least: listed > > affected shells are just ash 0.2 (!) and QNX 4.25 shell. > > Did you check Sven's page about set -e? > No, I just read the relevant excerpt from the autoconf manual. And aren't we speaking about the "propagation of exit status from command substitutions in assignments" here? What does 'set -e' have to do with it? Note: that's an honest question, not a rethorical one; maybe I'm missing something? > > > You might want to test for nonempty variable contents here. > > > > > In fact, to be even more reliable in case of broken/non-POSIX `date' > > commands, this is what I'd like to squash in if there are no > > objections: > > > +case `LC_ALL=C date '+%u'` in > > + [1-7]) date_is_posix=:;; > > + *) date_is_posx=false;; > > +esac > > +$date_is_posix \ > > + && day=`LC_ALL=C date '+%d'` && test -n "$day" \ > > + && month=`LC_ALL=C date '+%B'` && test -n "$month" \ > > + && year=`LC_ALL=C date '+%Y'`&& test -n "$year" \ > > + || { echo "$me: 'date' is not POSIX-complaint enough"; Exit 77; } > > compliant. > Oops, sorry. Fixed. > > day=`echo "$day" | sed 's/^0//'` > > > OK? > > Sure. > > > Also, thinking over it again, this test doesn't *really* require a grep > > that can parse nonprinting characters! It just requires a grep that can > > work on input that is not pure text. So, what about the following > > squash-in? > > OK. > I've now merged the patch into maint, merged maint into master, and pushed. Thanks, Stefano