Re: [PATCH] {maint} Improve, extend and tweak tests on Texinfo support.

[Stefano Lattarini]

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