Re: [PATCH] {maint} Improve, extend and tweak tests on Texinfo support.
Hi Stefano, * Stefano Lattarini wrote on Mon, Jan 03, 2011 at 02:38:48PM CET: From f236e0eb21df62ff4e5c01ecfaefe75d2e3d3d9b Mon Sep 17 00:00:00 2001 From: Stefano Lattarini stefano.lattar...@gmail.com Date: Fri, 24 Dec 2010 02:56:35 +0100 Subject: [PATCH] Improve, extend and tweak tests on Texinfo support. * tests/instdir-texi.test: Add a call to `ls -l' after that to `make', for debugging. When looking for required tools, do not redirect the output of $tool --help to /dev/null, and do not uselessly run it in a subshell. * tests/txinfo.test: Rewritten to run autoconf, ./configure and make. All checks moved into Makefile.am. * tests/txinfo8.test: Likewise, and modernize the generated configure.in. [...] This patch has caused a couple of regressions, txinfo.test and txinfo8.test now fail when makeinfo is not found. See here: http://hydra.nixos.org/build/856303/log/raw How about not requiring makeinfo in these tests? Thanks, Ralf
Re: [PATCH] {maint} Improve, extend and tweak tests on Texinfo support.
On Thursday 13 January 2011, Ralf Wildenhues wrote: Hi Stefano, * Stefano Lattarini wrote on Mon, Jan 03, 2011 at 02:38:48PM CET: From f236e0eb21df62ff4e5c01ecfaefe75d2e3d3d9b Mon Sep 17 00:00:00 2001 From: Stefano Lattarini stefano.lattar...@gmail.com Date: Fri, 24 Dec 2010 02:56:35 +0100 Subject: [PATCH] Improve, extend and tweak tests on Texinfo support. * tests/instdir-texi.test: Add a call to `ls -l' after that to `make', for debugging. When looking for required tools, do not redirect the output of $tool --help to /dev/null, and do not uselessly run it in a subshell. * tests/txinfo.test: Rewritten to run autoconf, ./configure and make. All checks moved into Makefile.am. * tests/txinfo8.test: Likewise, and modernize the generated configure.in. [...] This patch has caused a couple of regressions, txinfo.test and txinfo8.test now fail when makeinfo is not found. See here: http://hydra.nixos.org/build/856303/log/raw How about not requiring makeinfo in these tests? What about a middle-ground solution to ensure more coverage on systems lacking Texinfo? See the patch below. I will wait for approval before pushing. Regards, Stefano -*-*- tests: fix spurious failures in two texinfo tests * tests/txinfo.test ($required): Add 'makeinfo'. * tests/txinfo8.test: Create a dummy 'textutils.info' file, so that make won't try to run makeinfo (which could be unavailable) to build it. Found by NixOS Hydra, reported by Ralf Wildenhues. --- ChangeLog |9 + tests/txinfo.test |1 + tests/txinfo8.test |7 +++ 3 files changed, 17 insertions(+), 0 deletions(-) diff --git a/ChangeLog b/ChangeLog index dc59375..9d5a296 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,12 @@ +2011-01-13 Stefano Lattarini stefano.lattar...@gmail.com + + tests: fix spurious failures in two texinfo tests + * tests/txinfo.test ($required): Add 'makeinfo'. + * tests/txinfo8.test: Create a dummy 'textutils.info' file, so + that make won't try to run makeinfo (which could be unavailable) + to build it. + Found by NixOS Hydra, reported by Ralf Wildenhues. + 2011-01-11 Stefano Lattarini stefano.lattar...@gmail.com Improve, extend and tweak tests on Texinfo support. diff --git a/tests/txinfo.test b/tests/txinfo.test index 2e94486..b764e53 100755 --- a/tests/txinfo.test +++ b/tests/txinfo.test @@ -18,6 +18,7 @@ # Test to ensure texinfo.tex is included in distribution. Bug report by # Jim Meyering. +required=makeinfo . ./defs || Exit 1 set -e diff --git a/tests/txinfo8.test b/tests/txinfo8.test index 8dd24a4..13c8e31 100755 --- a/tests/txinfo8.test +++ b/tests/txinfo8.test @@ -53,6 +53,13 @@ $AUTOMAKE -a test -f auxdir/texinfo.tex ./configure + +# Create textutils.info by hand, so that we don't have to require +# makeinfo. Also ensure it's really newer than textutils.texi, so +# that make won't try to re-create it. +$sleep +: textutils.info + $MAKE test1 test2 : -- 1.7.2.3
Re: [PATCH] {maint} Improve, extend and tweak tests on Texinfo support.
On Thursday 13 January 2011, Ralf Wildenhues wrote: * Stefano Lattarini wrote on Thu, Jan 13, 2011 at 08:57:49PM CET: What about a middle-ground solution to ensure more coverage on systems lacking Texinfo? See the patch below. OK but you have to try it with non-GNU makes to ensure that touching this one file only is sufficient. Tested with FreeBSD, NetBSD and Heirloom make (having `MAKEINFO=false' in the environment). The test passed, so I pushed the patch. Regards, Stefano
Re: [PATCH] {maint} Improve, extend and tweak tests on Texinfo support.
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). So, support for more than one parallel-tests testsuite per Makefile.am is needed soonish. Besides, while I agree that the 8+3 names are often lacking descriptiveness, I also don't like typing too much. For example, I'm not sure why we named the 'posixsubst*.test' files that way; there is little specifically posixy about these substitution rules. +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. --- a/tests/comments-in-var-defn.test +++ b/tests/comments-in-var-defn.test @@ -21,11 +21,20 @@ set -e +cat configure.in 'END' +AC_OUTPUT +END + # Use a slash in the comment, because automake takes the dirname # of TEXINFO_TEX to compute $(am__TEXINFO_TEX_DIR). cat Makefile.am 'END' TEXINFO_TEX = tex/texinfo.tex# some comment w/ a slash info_TEXINFOS = main.texi +.PHONY: test +test: + test tex/texinfo.tex = $(TEXINFO_TEX) + test -d '$(am__TEXINFO_TEX_DIR)' + case '$(am__TEXINFO_TEX_DIR)' in tex|./tex) :;; *) exit 1;; esac END cat main.texi 'END' @@ -41,7 +50,9 @@ $AUTOMAKE grep TEX Makefile.in # for debugging grep '^TEXINFO_TEX *= *tex/texinfo\.tex *# some comment w/ a slash *$' Makefile.in -grep '^am__TEXINFO_TEX_DIR *=.*[/ ]tex *$' Makefile.in -$EGREP 'am__TEXINFO_TEX_DIR.*=.*(comment|#)' Makefile.in Exit 1 + +$AUTOCONF +./configure +$MAKE test --- /dev/null +++ b/tests/vtexi3.test +day='([1-9]|1[0-9]|2[0-9]|3[01])' +month='(January|February|March|April|May|June|July|August|September|October|November|December)' +year='20[0-9][0-9]' # hopefully automake will be obsolete in 80 years ;-) I do not agree with the tone of the comment, and as punishment will require that the code also works later than the mentioned date. ;- Eh eh :-) (because that's a joke, right?) Not totally. Why do you hope that it will be obsolete? ;-) Also, your comment writing style seems to be degrading away from writing whole sentences (including leading capitalization and final period) again here and below. Yes, I tend to do so for short comments, especially in tests. If you would prefer to set a policy mandating that comments are always to be capitalized correctly and to consist of whole sentences, please just do so (ideally stating that in HACKING and tests/README ;-) and I'll follow the new policy as consistently as I can (while I like writing casual-style comments sometimes, I have no strong feeling on the matter). Me neither, at least not strongly enough to do something about it. I merely noted it though. For the moment, I've amended the comments in this test for proper capitalization, punctuation and grammar (see the attached squash-in). Thanks. +: ../foobar.info +: ../quux.info +: ../zardoz.info These commands are not guaranteed to portably update the time stamp of the files in question on old systems. :-O 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
Re: [PATCH] {maint} Improve, extend and tweak tests on Texinfo support.
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
Re: [PATCH] {maint} Improve, extend and tweak tests on Texinfo support.
* Stefano Lattarini wrote on Tue, Jan 11, 2011 at 02:14:26AM CET: On Monday 10 January 2011, Ralf Wildenhues wrote: 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? I don't think there is any way to avoid the limit with portable make alone. When gnu-make infrastructure is in place, we can think about a replacement rule for that, but we should provide multiple test suites anyway, that's also nice for subsetting in general. 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 ;-). Oh, this is obviously not a big deal, but I actually try NetBSD csh sometimes which doesn't seem to provide it; this is mostly to ensure that the $SHELL setting from the environment doesn't leak into our code. 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? What's wrong with s/^posix// though? Lots of other things are Posix-mandated too, but we don't make a big deal out of that either? Cheers, Ralf
Re: [PATCH] {maint} Improve, extend and tweak tests on Texinfo support.
Ping on this? I will push in 72 hours if there are no further comments or objections. Regards, Stefano
Re: [PATCH] {maint} Improve, extend and tweak tests on Texinfo support.
* Stefano Lattarini wrote on Mon, Jan 03, 2011 at 02:38:48PM CET:
Subject: [PATCH] Improve, extend and tweak tests on Texinfo support.
* tests/instdir-texi.test: Add a call to `ls -l' after that to
`make', for debugging. When looking for required tools, do not
redirect the output of $tool --help to /dev/null, and do not
uselessly run it in a subshell.
* tests/txinfo.test: Rewritten to run autoconf, ./configure and
make. All checks moved into Makefile.am.
* tests/txinfo8.test: Likewise, and modernize the generated
configure.in.
* tests/txinfo2.test: Moved checks into Makefile.am, and other
minor improvements.
* tests/txinfo5.test: Enable `errexit' shell flag, and related
changes. Add trailing `:' command.
* tests/txinfo6.test: Likewise, and make grepping of generated
Makefile.in stricter.
* tests/txinfo7.test: Enable `errexit' shell flag, and related
changes. Add trailing `:' command. Do not add unnecessary stuff
to Makefile.am.
* tests/txinfo9.test: Verify that more targets which are expected
to be generated only once really are. Make grepping less strict,
to avoid exposing too much internal details. More minor changes.
* tests/txinfo16.test: Add trailing `:'. Prefer cat over echo
for appending to configure.in. Updated/fixed heading comments.
* tests/txinfo23.test: Likewise, and extended a little by making
it check that no info file is created in the $(srcdir).
* tests/txinfo24.test: Likewise.
* tests/txinfo25.test: Likewise.
* tests/txinfo18.test: Add trailing `:'. Prefer cat over echo
for appending to configure.in. Also, check that index files are
cleaned also by make clean, not only by make distclean.
* tests/txinfo22.test: Prefer `$me' over hard-coded test name,
and added trailing `:' command. This testcase also used to check
that automake ignores in-line comments when using variables, but
preserves them in the output; these checks (added in commit
Release-1-7f-4-g9177ef8) do not really pertain to this test,
so they have been moved ...
* tests/comments-in-var-definition.test: ... into this new test.
* tests/txinfo4.test: Escape literal dots in grep regexps. Add
trailing `:' command.
* tests/txinfo29.test: Likewise. Relax grepping of generated
Makefile.in w.r.t. whitespaces. Prefer `cat' over `echo' to
append to configure.in.
* tests/txinfo3.test: Likewise.
* tests/vtexi.test: Improve grepping of Makefile.in (sometimes
make it stricter, sometimes laxer). Move `set -e' setting just
after the inclusion of ./defs. De-uglify a sed command. Other
minor cosmetic improvements.
* tests/vtexi2.test: Make grepping of Makefile.in stricter. Add
trailing `:' command.
* tests/vtexi3.test: New test on version.texi support.
* tests/vtexi4.test: Likewise.
* tests/Makefile.am (TESTS): Updated.
OK with nits 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.
@@ -0,0 +1,47 @@
+# Make sure Automake ignores in-line comments when using variables,
+# but preserve them in the output.
preserves
+
+. ./defs || Exit 1
+
+set -e
+
+# Use a slash in the comment, because automake takes the dirname
+# of TEXINFO_TEX to compute $(am__TEXINFO_TEX_DIR).
+cat Makefile.am 'END'
+TEXINFO_TEX = tex/texinfo.tex# some comment w/ a slash
+info_TEXINFOS = main.texi
+END
+
+cat main.texi 'END'
+\input texinfo
+...@setfilename main.info
+END
+
+mkdir tex
+: tex/texinfo.tex
+
+$ACLOCAL
+$AUTOMAKE
+
+grep TEX Makefile.in # for debugging
+grep '^TEXINFO_TEX *= *tex/texinfo\.tex *# some comment w/ a slash *$'
Makefile.in
+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.
--- /dev/null
+++ b/tests/vtexi3.test
+# Check that vers*.texi files are automatically created and distributed
+# if @included into a texi source. Also check that they correctly contain
+# the @values definitions they are advertised to.
+# See also the related test `vtexi4.test', which does similar checks, but
+# for version.texi only, and requires makeinfo, tex and texi2dvi.
+day='([1-9]|1[0-9]|2[0-9]|3[01])'
+month='(January|February|March|April|May|June|July|August|September|October|November|December)'
+year='20[0-9][0-9]' # hopefully automake will be obsolete in 80 years ;-)
I do not agree with the tone of the comment, and as punishment will
require that the code also works later than the mentioned date.
;-
Also, your comment writing style seems to be degrading away from writing
whole sentences (including leading capitalization and final period)
again here and below.
+date=$day $month $year
+
+do_check ()
+{
+ # basename of the vers*.texi file
+ vfile=$1
+ # $(srcdir) of the current build
+ srcdir=$2
+ # vers*.texi must be
