On Saturday 04 June 2011, Peter Rosin wrote: > Den 2011-06-02 17:36 skrev Stefano Lattarini: > > This should offer greater testsuite coverage for those developers > > that override CONFIG_SHELL at configure time in order to test more > > shells on a single system, instead of just the default `/bin/sh'. > > Do we not want to test these scripts in the same way that they are > later used? > Absolutely -- but I think that, ideally, all the Automake-provided shell scripts should be run with configure-time detected $SHELL. To be honest, I thought this was already the case, but now I see that it is not always true unfortunately. For example, it's not for the `compile' script (see `m4/minuso.m4') or the `py-compile' script (see `automake.in' and `lib/am/python.am'), while it is true for `mkinstalldirs' and `depcomp' (see `automake.in') as for `ylwrap' (see `lib/am/yacc.am' and `lib/am/lex.am'), `mdate-sh' (see `lib/am/texi-vers.am') and `elisp-comp' (see `lib/am/lisp.am').
I think this inconsistency should be fixed by always using $(SHELL) to run the Automake-provided shell scripts (that might turn out a little tricky for `compile', though). And an addition to `HACKING' in this respect would be nice too. > Isn't that more important compared to the convenience > it might be to test things using various shells on a single machine? > Yes, but I consider this convenience more an added value than a basic motivation. A believe which I failed to reflect properly in the ChangeLog entry, BTW :-( Sorry about that, my bad. If you have any improvement for the ChangeLog entry to propose, I'm all ears. > If we don't run the scripts with /bin/sh in the testsuite, we might > miss some instance where the script is broken on the "lesser" shell > even though the path taken through the script _should_ not reguire > an xsi-shell, e.g. in the compile2.test case. > $SHELL does not have to be an "xsi-shell"; in fact, when testing on solaris, I usually force CONFIG_SHELL (and thus SHELL) to `/bin/sh', which is not even a POSIX shell (e.g., it has no `$(...)' support). And I'm assuming that the Automake developers are prepared override CONFIG_SHELL by hand to point to a lesser shell, on systems where that can offer an increased testsuite coverage. That's what I usually do, and the same (if I'm not badly mistaken) does Ralf. > I.e. IMHO, it _might_ be ok to do this change for tests that require > an xsi-shell, but otherwise not. I think it's better to keep skipping > the tests if not using an xsi-shell because the "increased coverage" > argument has flaws: > 1. it decreases test coverage for code intended for /bin/sh > But the code tested is intended for $SHELL, not for /bin/sh; and forcing the use of /bin/sh can indeed *reduce* coverage. In fact, by forcing the use of /bin/sh when testing the Automake-provided scripts, you ignore the real-life possibility of those scripts being run with, say, /bin/ksh instead, in case `configure' determined that ksh is a better shell, and set CONFIG_SHELL (and thus SHELL) accordingly. Now, what happens if the tested script tickles a bug in /bin/ksh but not in /bin/sh? Your test, which uses /bin/sh unconditionally, passes, but when someone later uses an Automake-generated Makefile making use of that same script on the *same* system you've tested, he might experience a failure that have escaped you, because configure has chosen /bin/ksh over /bin/sh for him. > 2. it is common to run the testsuite with an xsi-shell > > Or are you saying that the xsi-shell requirement checks if $SHELL > is an xsi-shell? > After commit `v1.11-874-g1321be7', it does: <http://git.savannah.gnu.org/gitweb/?p=automake.git;a=commit;h=1321be7068464238d1c626abad0f52cb1cd6cba2> Well, that's not completely true if the user overrides SHELL at runtime; but than he's doing something he's not supposed to, and he's on his own. > In that case we need a new xsi-bin-sh requirement instead of this > patch. > I had thought about that too originally, but I believe the approach of this patch is better and more correct (again, assuming the user does not force unexpected overrides at make time). > > This change also fixes few spurious failures in tests using the > > `xsi-shell' requirement, where inconsistencies could crop up if > > the shell probed for XSI features (which, by default, is $SHELL) > > was not the same shell later used to run the scripts using those > > features (which was hard-coded to `/bin/sh'). Such failures have > > already occurred in practice, for examples on Solaris systems > > which had also GNU Bash installed. > > s/fixes few/fixes a few/ > s/for examples/for example/ > s/which had also GNU Bash/with GNU Bash/ > > > * tests/ar-lib.test: Run the `ar-lib' script with `$SHELL', rather > > than directly with `./ar-lib', which would make run unconditionally > > with `/bin/sh'. > > s/would make run/makes it run/ > Will fix all of these in a follow-up patch. Thanks for spotting them. > > * tests/compile.test: Likewise, but for the `compile' script. > > * tests/compile2.test: Likewise. > > * tests/compile3.test: Likewise. > > * tests/compile4.test: Likewise. > > * tests/compile5.test: Likewise. > > * tests/compile6.test: Likewise. > > * tests/instsh2.test: Likewise, but for the `install' script. > > * tests/instsh3.test: Likewise. > > * tests/mkinst3.test: Likewise, but for the `mkinstalldirs' script. > > * tests/missing.test: Likewise, but for the `missing' script. > > * tests/missing2.test: Likewise. > > * tests/missing3.test: Likewise. > > * tests/missing5.test: Likewise. > > *snip* > > > diff --git a/tests/compile3.test b/tests/compile3.test > > index f949d1c..141a17a 100755 > > --- a/tests/compile3.test > > +++ b/tests/compile3.test > > @@ -30,23 +30,23 @@ END > > chmod +x ./cl > > > > # Check if compile handles "-o foo", -I, -l, -L, -Xlinker -Wl, > > -opts=`LIB= ./compile ./cl foo.c -o foo -lbar -Lgazonk -Ibaz -Xlinker > > foobar -Wl,-foo,bar` > > +opts=`LIB='' $SHELL compile ./cl foo.c -o foo -lbar -Lgazonk -Ibaz > > -Xlinker foobar -Wl,-foo,bar` > > The LIB='' change is not mentioned under tests/compile3.test in > ChangeLog. What purpose do the ticks serve anyway? > Nothing, this was just an unwarranted change, sorry. No point in reverting it now though I think, that would just add more noise. > *snip* > > > diff --git a/tests/instsh3.test b/tests/instsh3.test > > *snip* > > > + > > +: > > Not mentioned in ChangeLog. > > *snip* > > > diff --git a/tests/missing3.test b/tests/missing3.test > > *snip* > > > grep WARNING stderr && Exit 1 > > grep Unknown stderr > > + > > +: > > Not mentioned in ChangeLog. > > *snip* > > > diff --git a/tests/missing5.test b/tests/missing5.test > > index 010b344..91e5857 100755 > > --- a/tests/missing5.test > > +++ b/tests/missing5.test > > @@ -33,9 +33,9 @@ AC_OUTPUT > > EOF > > > > for tool in $needed_tools; do > > - cat >$tool.in <<EOF > > -#! /bin/sh > > -exec @$tool@ "\$@" > > + unindent >$tool.in <<EOF > > + #! /bin/sh > > + exec @$tool@ "\$@" > > Not mentioned in ChangeLog. > > *snip* > > > + > > +: > > Not mentioned in ChangeLog. > > *snip* > > > diff --git a/tests/mkinst3.test b/tests/mkinst3.test > > *snip* > > > test -d "`pwd`/~a b/-x y" > > rm -rf '~a b' > > + > > +: > > Not mentioned in ChangeLog. > > Now, the million dollar question: What other changes that were not > mentioned in the ChangeLog did I miss? > > Sneaking in a few unrelated changes in a big "scriptable change"- > type patch makes it hard to find the unrelated changes. It appears > that the above "sneaky changes" that I point out are "just" style > issues? > Yes, and you're absolutely right complaining about the LIB= -> LIB='' useless change, and the cat -> unindent "unreported" change; for them I can only apologize, and fix the ChangeLog entry to report the latter. OTOH, I think that adding a trailing `:' when doing unrelated changes to a test script is fine, and there's no real need to report such an edit in the ChangeLog (that would be mostly noise IMHO); but I've no strong feelings about this, and might change my habit if you insist that you find a more faithful ChangeLog entry always useful, regardless of the possible extra noise. Thanks, Stefano
