On Monday 06 June 2011, Peter Rosin wrote: > > [SNIP] > > Den 2011-06-05 12:57 skrev Stefano Lattarini: > > > (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. > I suspect that this Someone Else (TM) might prefer to read a thread with (say) three or four messages showing mostly agreement, rather than an old thread were we quibbled endlessly about the color of the shed ;-)
> >> 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. > Agreed. > >> 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. > In the tests I've touched, yes, since they just use $SHELL directly to run the checked script; and for what concerns these checked scripts, doing: $ export SHELL=/bin/bash; $SHELL ar-lib ... or: $ /bin/bash ar-lib ... should be perfectly equivalent. > > [SNIP] > > >> 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? > Yes, unless the user forces the use of a different shell (you know, always give the user enough rope to hang himself). On the other hand, $SHELL cannot currently be overridden in the test scripts, since `tests/defs' defines it unconditionally (and letting it being overridden from the environment would be a royally bad idea, since it's probably not uncommon for users to have, say, SHELL=/bin/tcsh). > *time passes* > > Ahhh, you are thinking about the case when test scripts are executed > standalone, > No, because the test scripts, even when executed standalone, takes care of re-executing themselves with $SHELL. See commit `v1.11-874-g1321be7'. > so scratch that. > > *time passes* > > Hmmm, $SHELL is potentially different when running tests standalone and > when running them from make check, > No, it's not. See above. > leading to different test results. Is that acceptable? > Hardly so; but luckily, it doesn't happen in the present setup :-) > > 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. > I plan to avoid code duplication as follows: the test code should be moved in a new *.sh file, so that the sister tests could then just run by sourcing that file after setting one or two variables that will configure its behaviour (for a reference, similarly to what is currently done in master with the auto-generated `instpc*.test' tests). > 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. > I think my idea above is even simpler, and should address your (justified) concerns about code duplication. Anyway, I'll let you judge my idea when I have a patch to back it up. > > [SNIP] > > Cheers, > Peter > I'll go ahead and revert the patch then, and prepare new ones on the lines sketched above. Regards, Stefano
