On Monday 06 September 2010, Bruno Haible wrote:
> Hello Stefano,
Hi Bruno. Thanks for your all your quick reviews.
> Ralf said:
> > Give Bruno some time for comments before applying to the
> > pot-primary branch, please.
>
> Yes, please. Give me always between 2 days (48 hours) and 7 days to
> react.
I was planning to give you all the time you needed :-). I would have
pinged you in a week or so if you would have not yet commented at that
point. Anyway, I won't apply any patch to the "pot-primary" branch
without your review and/or approval.
> > tests/README suggest
>
> Oops, I had completely missed this file. Now that I read it, I see
> that the 'pot-macro-warn' test will also need an 'rm -rf
> autom4te.cache'.
Will you take care of that after I've pushed my patches?
> > 1. You sometimes failed to use `$MAKE' instead of `make'
>
> Yes, sorry, I did not think at this.
>
> > and `Exit' instead of `exit'
>
> Likewise, sorry, I wasn't aware of this.
Don't worry, "make distcheck" caught those for me ;-)
> > 2. IMHO it's better to avoid redirecting output (from e.g. grep
> > and
> >
> > cmp) to /dev/null, since our testsuite is already very
> > verbose, and some extra output (even if redundant) won't
> > hurt.
>
> I agree for the cases where one expects 'grep' not to find
> anything. But for the positive cases, I don't agree:
> After changing
> grep '^pot-linguas-1\.0/posub/foo-bar-de\.po$' filelist
> >/dev/null to
> grep '^pot-linguas-1\.0/posub/foo-bar-de\.po$' filelist
> the output log will contain the line
> pot-linguas-1.0/posub/foo-bar-de.po
> which is useless since it is always this same string.
Absolutely true. But IMHO is still better to be consistent, and
avoid redirecting any output to /dev/null, unless there is a very
very good reason to do so (for example, the output is in binary
format, unsuitable for a log file and/or for a human to analyze,
or it is a riduculously verbose output of, say, 20 Megabaytes).
> But anyway, your preference here matters more than mine.
More importantly, in this particular case, the following patch
removes all these greps, so this fortunately becomes a mostly
moot point.
> > 3. tests/README suggest to end test scripts with a `:':
> > ``End the test script with a `:' or `Exit 0'. Otherwise,
> > when
> >
> > somebody changes the test by adding a failing command
> > after the last command, the test will spuriously fail
> > because $? is nonzero at the end.''
>
> I have no objection. But for tests that are run in 'set -e' mode,
> the rationale is void: if somebody changes the test by adding a
> failing command after the last command, the test will fail anyway,
> regardless\ whether there is a ':' command after it.
False, and tests/README explains why:
``... Note that this is relevant also for tests using `set -e', if
they contain commands like "grep ... Makefile.in && Exit 1" (and
there are indeed a lot of such tests).''
Yes, this is not obvious at first glance; that's why I asked this
exaplantion to be added to tests/README in the first place, as
I made your same (wrong) objection myself in the past.
> > 4. Ralf and I agreed that we shouldn't put `gzip' in $required
>
> I copied this idiom from one of the tests install2.test, lex3.test,
> pr9.test.
These is a pending patch to fix this:
<http://www.mail-archive.com/automake-patches@gnu.org/msg02271.html>
Thanks for reminding us.
> > Also, you forgot ... to add them to $(TESTS) in
> > tests/Makefile.am.
>
> Yes, I noticed this later myself, sorry.
>
> > - if grep '^pot-download1-1\.0/clisp-de\.po$' filelist
> > >/dev/null; then - exit 1
> > - fi
> > + grep '^pot-download1-1\.0/clisp-de\.po$' filelist && Exit 1
>
> Today you taught me about how 'command && exit 1' works in 'set -e'
> mode.
It has been a surprise for myself too at first.
> > OK to push the three attached follow-up patches?
>
> Part 1 and 2 are OK.
>
> In part 3, in pot-linguas.test, pot-noinst.test,
> pot-topsrcdir.test, you simplified
>
> cat $sourcedir/posub/foo-bar-de.po | grep great >/dev/null
>
> to
>
> grep great $sourcedir/posub/foo-bar-de.po
> or
> grep great < $sourcedir/posub/foo-bar-de.po
>
> This is an invalid simplification for files that contain non-ASCII
> characters, because 'grep' on OpenBSD 4.0 then merely outputs
> "Binary file (standard input) matches"
> for the case where the input is standard input, and similarly for a
> command- line argument. The only workaround I found (on
> 2008-01-12) for this problem is to use 'grep' with a pipe, not a
> regular file, as input.
You're right, this apparently useless use of cat is done in some other
places in the automake testsuite too. I'll try to revert all the
wrong cat removals; BTW, do you think looking for them with:
$ grep -i 'grep.*\.po\>'
is enough?
> This is not an issue for POT files, which - in the Automake test
> cases - only contain ASCII characters. But in the PO files, I want
> to be able to use non-ASCII characters here and there without much
> thinking. It's only by luck that the German and French PO files
> that I used there happen to contain only ASCII characters; this
> may change in the future.
>
> The rest of part 3 is OK.
>
> Thanks for all these fixes!
Thanks four your reviews. I'll post a revised "patch n.3" soonish.
Regards,
Stefano
