Re: testsuite failures when test scripts are run with zsh

[Stefano Lattarini]

I attached a new version of the patch.  Please note that it still 
doesn't address the remarks and objections made in the present mail.

At Tuesday 13 October 2009, Ralf Wildenhues <address@hidden> 
wrote:
> Hi Stefano,
>
> > I understand.  But what about instead susbstituting everywhere
> > `run_command $AUTOMAKE' instead of `AUTOMAKE_run' and
> > `run_command -e 1 $AUTOMAKE' instead of `AUTOMAKE_fails'
> > (of course remembering to add a proper `|| Exit 1' in tests not
> > using `set -e')?
>
> First off, I think that run_command really should Exit when the
> command does not produce the intended status.  It should not be
> necessary to do run_command -e 1 $command || Exit 1
>
> That is much safer, and less maintenance.
I see your point.  It's OK with me to have `run_command' calling Exit 
on failures, since (as you stressed) that's by far the most common 
scenario.  However, we should then really add a similar subroutine 
(say `run_redirect' -- tell me if you have a better name) which only 
takes care of portably redirecting stdout/stderr of a command (and, 
obviously, rewrite `run_command' implementation to advantage of the 
new function).
Is this OK with you?

> If we really need to run some command where we need to ignore
> the exit status, then we can still use
>
>   run_command '$command || :'
This is wrong, as currently `run_command' do not `eval' its COMMAND.
And the exit status of $command would be lost, which might not be what 
we really want.
>
> or make this command another function or so.
Yes, please.

>
> Which brings me to the second inconsistent issue with this API: the
> 'eval'uation level of the command is part of the API.
> This is important, because when the absolute source and build
> directories contain white space in the name (and Automake mostly
> works in this case now), we should be doing the right thing.
I don't understand. The `eval' used in the run_command implementation 
is there just to provide a shortand: no argument passed by the caller 
is ever eval'd (and this is the right thing to do, I think).

> Then to your question above: yes it is ok to replace all instances
> of AUTOMAKE_run and AUTOMAKE_fails (there is no need to replace
> plain $AUTOMAKE without redirection).
>
> > If you think about it, the testsuite don't have `ACLOCAL_run' or
> > `ACLOCAL_fails', but simply uses `run_command $ACLOCAL' and
> > `run_COMMAND -e 1 $ACLOCAL'.
> > By the way, this change should require a small change in
> > `tests/README' too.
> > If you agree with it, I think it should be done with a distinct
> > patch.
> Sounds good.
Mmhh, I see another possible misunderstanding creeping in here.
Better to clear it out, just to be absolutely sure.  What I was trying
to say is that we should get rid of AUTOMAKE_run and AUTOMAKE_fails,
not add ACLOCAL_run and ACLOCAL_fails (especially now that I'm going
to follow your advice and make run_command use `Exit 1' on failures).
Is this OK?

> > > Yeah, so how about
> > >   if eval "${_run_evald_cmd}"; then
> > >     run_exitcode=0
> > >   else
> > >     run_exitcode=$?
> > >   fi
> >
> > This seems better; however, are we sure that the value of `$?' is
> > reliable there?
> Yes.
Good, I'll use your code then.

> > > BTW, your run_command doesn't do what it advertizes to do: it
> > > doesn't necessarily cause a test failure when it should, esp.
> > > when a test doesn't use 'set -e'.
> >
> > I think that here there's a misunderstanding about the meaning of
> > `fail': you mean "the testcase fails", while I mean "the function
> > fails", i.e. it return a value != 0.  Do you have a rewording to
> > suggest to make things clearer?
>
> "fail" is FAIL, and exit status != 0 is returning a nonzero exit
> status.
This will become mostly a moot issue once run_command will call
`Exit 1' on unexpected exit status.  Should I anyway use "FAIL" 
instead of "fail"?

> > > [CUT]
> > >
> > > > By the way, your observation has made me think: wouldn't it
> > > > be better to enable `set -e' in defs.in, so that all the test
> > > > cases could have a more uniform environment?
> > >
> > > This would require an audit of all tests that currently don't
> > > use set -e.  This needs testing on Solaris, OpenBSD, NetBSD,
> > > Tru64, because of bugs and warts in their shell's
> > > implementation of errexit.
> >
> > Unfortunately, I don't have access to any of those system, so
> > I'll have to drop the ball on this.
>
> I can test the final iteration of this patch.
Well, even if we are going to make `set -e' the default, I think that 
this change should be introduced with patch.

> BTW, now that we have TEST_LOG_COMPILER, and correctly unset it in
> defs.in, too, we can set it in tests/Makefile.am and worry less
> about shells like Solaris /bin/sh.  Of course, that would require
> us to warn that running tests directly (i.e., without 'make'
> in-between), might require to use a decent shell.
Or we could add proper code in `tests/defs.in', to make it re-execute 
the test scripts with CONFIG_SHELL.  But this should be obviously done 
in a distinct patch.

Regards,
     Stefano

Fix-testsuite-avoid-Zsh-related-problems-with-set-x.patch
Description: Text Data