Re: [FYI] {testsuite-work} tests: use `$SHELL' to run the shell scripts from `lib/'

[Stefano Lattarini]

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