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

[Peter Rosin]

Den 2011-06-04 11:24 skrev 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.

And I don't see how the inconsistency /can/ be reasonably solved, since
e.g the ar-lib script currently has to be explicitly used by the end
user (AR="/here/is/ar-lib lib" -> AR="$SHELL /here/is/ar-lib lib"). Now,
the ar-lib script is perhaps not the best example, but the same holds
for the compile script since not all projects do AM_PROG_CC_C_O. Your
patch changes the rules for the scripts, and the reason is a dubious
argument to increase testsuite coverage, see more below.

We both think the testsuite should execute the script as they are executed
when they are used. And I think that it should only involve $SHELL if there
is a guarantee that $SHELL will always be used, otherwise there will be
inconsistencies.

It should also be safe to copy-paste things to the command line and
re-run commands (i.e. $SHELL has to be expanded when displayed, I don't
know if this is already the case).

>> 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 didn't say that $SHELL is always an xsi-shell. Some tests (such as
compile2.test) were once written to run the scripts with /bin/sh,
whatever that might be. Now that scripts are run by $SHELL, which when
/bin/sh is a "lesser" shell, is more likely to be bash you have actually
reduced coverage (not for Solaris with GNU bash installed perhaps,
because you seem to have that covered, but something else which you did
not think of).

Yes, I understand that *you* (and Ralf) can test a handful (or whatever)
of shells easily when $SHELL runs the scripts, but this "increase" in
coverage comes with the cost that all systems with *both* a weird
/bin/sh and a better shell available will not run the scripts with
the weird /bin/sh, thus reducing the testsuite coverage.

I.e. the gain is that *you* can do lab style tests more easily, but the
loss is less testsuite coverage in the wild. It just has to be better to
fix the lab instead.

>> 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.

What makes you say that the compile script is intended to be executed by
$SHELL? How will you guarantee that compile is always executed by $SHELL?
That seems pretty hard to fix, and it will make it more difficult to use
compile in the correct way. It will also needlessly clutter up the commands
with $SHELL once the compile script is in effect.

Do you see how this change causes a regression in the 'compile' interface.
You are saying that it is no longer supported (it's not tested anyway) to
do:

        .../configure "CC=/here/is/compile foocc"

for projects that do not bother to AM_PROG_CC_C_O. Which is a fair share
or projects.

If you do go through with this, the interface change should be mentioned
in NEWS. By the way, how should the user specify CC above? Should the
user guess how configure sets $SHELL? Should the user re-run configure
after looking up what $SHELL was set to? Painful.

BTW, I only know about compile and ar-lib, and can't speak for the other
scripts affected by this patch.

> 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.

I think that a fair share of this patch should be reverted anyway,
and in that case this might as well be reverted too.

>> *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.

Did you look for more stuff that may have wiggled its way in? What is
the answer to the million dollar question?

You need to read through your own patches more carefully. Slow down.
What's the rush? Especially when there is virtually no review process.
You are also not inviting review when you mostly push FYI patches
where the review is an afterthought at best.

Where will the testsuite-work branch go anyway? If you intend to merge
it, it would perhaps make sense to try to keep it reasonably clean and
leave at least some room for review? And don't make the mistake that my
random "reviews" somehow means that I have no issues with stuff that
I'm not commenting on. Assume that *noone* read a patch (other than
you, hopefully) if there are no comments.

> 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.

Why do you ask me? The GNU Coding Standards are pretty clear, the first
sentence from the ChangeLog chapter reads "Keep a change log to describe
all the changes made to program source files."

I wouldn't personally be bothered with adding trailing ':' *if* that was
the only thing that went in w/o mention. But in this case there were
unintended changes, and then it quickly gets complicated. Since we're
all fallible I guess it is best to describe *all* changes just as the
GCS says. But I'll settle for the intended changes :-)

Cheers,
Peter