Hi Stefano, * Stefano Lattarini wrote on Tue, Oct 13, 2009 at 04:24:38PM CEST: > At Tuesday 13 October 2009, Ralf Wildenhues wrote: > > > > 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?
This is making things too complicated for my taste. Your run_command already has an -e option. If we're going to return the exit status anyway from the function, then we wouldn't need that -e *at all*, we could just return whatever status the command caused. So if it pleases you better, then we can change it to either of the following: - '-e ignore' ignores the status of the command; run_command returns it; - no '-e' just lets run_command return the status of the command, rather than checking the command against zero. I prefer the first, but only on the grounds that more typing is done only in the rare case. > > 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. OK, sorry about that glitch. > > 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). OK. > > 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? OK. > > > > 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"? Either way is fine. > > > > > 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. Do you mean "should be introduced with a separate patch"? If yes, then I agree. > > 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. Yes. > --- a/tests/defs.in > +++ b/tests/defs.in > @@ -395,26 +395,91 @@ is_newest () > test -z "$is_newest_files" > } > > +# run_command [-e STATUS] [-i FILE] [-m] [--] COMMAND [ARGUMENTS..] > +# ----------------------------------------------------------------- > +# Run the given COMMAND with ARGUMENTS and fail if COMMAND does not exit > +# with STATUS. If status is "VOID" or "IGNORE", any exit value of the > +# command is acceptable. If STATUS is "FAIL", then any exit value of the > +# command *but 0* is acceptable. Default STATUS is `0'. > +# Also, save standard output and standard error from COMMAND, by default > +# respectively in files `stdout' and `stderr' (in the current directory), > +# or togheter in the file `stdall' (in the current directory) if the `-m' typo togheter > +# option is given. > +run_command () > +{ > + set +x # xtrace verbosity temporarly disabled in `run_command' > + run_exitcode_expected=0 > + run_mix_stdout_and_stderr=no > + while test $# -gt 0; do > + case $1 in > + -e) run_exitcode_expected=$2; shift;; > + -m) run_mix_stdout_and_stderr=yes;; > + --) shift; break;; > + -?) echo "run_commmand(): invalid switch \`$1'" >&2; Exit 99;; s/()// > + *) break;; > + esac > + shift > + done > + case $# in > + 0) echo "run_command(): missing COMMAND argument" >&2; Exit 99;; Likewise. > + *) run_cmd=$1; shift;; > + esac > + if test x"$run_mix_stdout_and_stderr" = x"yes"; then > + run_evald_cmd='"$run_cmd" ${1+"$@"} >stdall 2>&1' > + else > + run_evald_cmd='"$run_cmd" ${1+"$@"} >stdout 2>stderr' > + fi > + if eval "$run_evald_cmd"; then > + run_exitcode_got=0 > + else > + run_exitcode_got=$? > + fi > + if test x"$run_mix_stdout_and_stderr" = x"yes"; then > + set -x > + cat stdall > + { set +x; } 2>/dev/null > + else > + set -x > + cat stderr >&2 > + cat stdout > + { set +x; } 2>/dev/null > + fi > + case $run_exitcode_expected in > + VOID|void|IGNORE|ignore|IGNORED|ignored|$run_exitcode_got) > + run_rc=0 > + ;; > + FAIL|fail|FAILURE|failure) > + if test $run_exitcode_got -gt 0; then > + run_rc=0 > + else > + run_rc=1 > + fi > + ;; > + *) > + run_rc=1 > + ;; > + esac > + echo "run_command: exit status $run_exitcode_got (expecting" \ > + "$run_exitcode_expected)" > + set -x # reactivating temporarly turned-off xtrace verbosity > + return $run_rc > +} Thanks, Ralf