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

[Peter Rosin]

Den 2011-06-05 12:57 skrev Stefano Lattarini:
> On Sunday 05 June 2011, Peter Rosin wrote:
>> 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").
>>
> (BTW, this would be another good reason to resurrect you patch on
> AM_PROG_AR; our disagreement there ended up with me acknowledging
> that you were 90% right, so it shouldn't be too difficul to reach
> consensus now).

IIRC, the discussion ended when we got tired of quibbling over how
the warning message should be worded and in what circumstances it
should appear, mostly the latter. We then waited for someone else<tm>
to just decide for us, but that never happened.

>> 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,
>>
> Not exactly true; it only changes the rules for the tests on these scripts.
> Yes, this implies that now the usage we care *more* about for these scripts
> is when they are run with configure-selected $SHELL; but that doesn't mean
> we don't care about keeping the them runnable with /bin/sh anymore.

You're right, not exactly true, but implicitly so. I was extrapolating
and exaggerating a teeny-weeny bit in order to drive home the point that
we should be testing the endorsed usage patterns; by changing the tests
those endorsed usage patterns also (implicitly) change.

>> 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).
>>
> Yes, that should be the case when `xtrace' is in effect:
>   $ sh -c 'SHELL=ksh; set -x; :; $SHELL -c ":"'
>   + :
>   + ksh -c :

Oh, I meant that it should be drop-dead easy to copy-paste from the
normal build output when you want to re-run some (failing) part of
the build process (with tweaked arguments), without worrying about
if $SHELL is set in the environment. Again, that may very well already
be 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.
>>
> OK, you've made good points as usual.
> 
> I now think I should revert this version of the patch, and repropose it so
> that the affected tests will run the scripts once with /bin/sh and once with
> $SHELL (skipping these latter checks if $SHELL happens to be /bin/sh).  This
> should cater both for the "lab" and the "wild".
> 
> Oh, and we'll need a couple of new requirements `shell-not-bin-sh' and
> `xsi-bin-sh' in 'tests/defs' (and the `xsi-shell' requirement could be
> adjusted to check $SHELL instead of the shell that is running the test
> script itself).  These changes could be proposed in preliminary patches.

Re xsi-shell adjustment, isn't $SHELL always running the test scripts?

*time passes*

Ahhh, you are thinking about the case when test scripts are executed
standalone, so scratch that.

*time passes*

Hmmm, $SHELL is potentially different when running tests standalone and
when running them from make check, leading to different test results.
Is that acceptable?

> Would this scenario be more acceptable for you?  FWIW, I think it would
> be an improvement both over the previous situation and my previous patch.

Yes, that would be fine. I thought about that myself, but didn't mention
it because I'm not too fond of "sister tests"; code duplication makes me
suspicious. Maybe the affected tests could somehow re-exec themselves
(only if /bin/sh != $SHELL) with some knob to trigger the sibling behavior
(I don't know if that is reasonable, just throwing out an idea). That way
there would be less chance of future errors when updating the tests.

> BTW, the "reverting patch" is attached, for reference.
> 
>>>> 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.
>>
> These objections are wrong.  To reiterate what I've said before, I wasn't
> saying that we should drop support for plain /bin/sh in the Automake
> provided scripts (I agree that would be ridicoulous); I was proposing that
> the *primary* concern of our tests on those scripts should shift from
> /bin/sh to $SHELL.  This made sense because Automake-generated Makefiles
> (mostly) use $(SHELL) to run those scripts.  But given my new proposal
> above, all this is moot now.

Yes, all moot now (but I still think the objection was valid, because you
removed the testsuite coverage -> bit-rot, but let's drop that line of
discussion).

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

Cheers,
Peter