At Saturday 10 October 2009, Ralf Wildenhues <ralf.wildenh...@gmx.de>
wrote:
> * Stefano Lattarini wrote on Sat, Oct 10, 2009 at 03:42:52AM CEST:
> [CUT]
> > > Hmm, I see a few inconsistencies cropping up here. First, we
> > > already have AUTOMAKE_run. It has slightly different syntax.
> > > With your patch, some automake invocations that capture output
> > > use AUTOMAKE_run, while others use run_command.
> > >
> > > These inconsistencies should be resolved. I'm fine with having
> > > all automake invocations use AUTOMAKE_run.
> >
> > Don't you think this should be done in a separate patch?
>
> I didn't make myself clear enough, sorry. What I meant was that we
> shouldn't change uses of $AUTOMAKE with redirections to
> 'run_command $AUTOMAKE' when we also have AUTOMAKE_run. We should
> only use one of the two, and the latter is already used. There is
> no need to convert uses of $AUTOMAKE which do not have
> redirections.
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')?
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.
Opinions?
> [CUT]
>
> > > > + _run_evald_cmd="${_run_evald_cmd} || _run_exitcode=\$?"
> > > > + eval "${_run_evald_cmd}"
> > >
> > > Why not simplify these two lines to
> > > eval "${_run_evald_cmd}"
> > > run_exitcode=$?
> > > and drop the _run_exitcode initialization above?
> >
> > This way, a failing `eval' would make the shell abort if `set -e'
> > is active. Or am I mistaken? Feedback needed.
>
> 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? In the meantime, I modified the patch to follow your
advice.
> > > > + if test x"${_run_mix_stdout_and_stderr}" = x"yes"; then
> > > > + echo "=== stdout and stderr"
> > >
> > > Instead of all the file boundary markers, can we just re-enable
> > > xtrace here? That makes the trace output look more similar to
> > > that we get from commands not run through this function. You
> > > can reorder this chunk of code to be after the exit code
> > > handling to not deal with xtrace twice.
> >
> > Well, I liked the `===' marker: it sticked out clearly without
> > being obtrusive.
>
> The question is: why do you want it to stick out further than the
> rest of the trace output? I can't see any reason.
>
> > But if you prefer a stricter consistency, I might just
> > use 'echo "+ cat stdout" >&2' etc. instead of 'echo "=== stdout"'
> > etc.
>
> You can have that easier by just using wrapping the output in "set
> -x"/ "set +x".
Agreed (in fact, the traces' format changes quite a bit between
different shells, so it's more consistent to temporarly re-enable
traces). Patch modified accordingly to your suggestion.
>
> 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?
> I didn't think of the need to Exit within run_command when I wrote
> my previous review; it makes the issue moot of reordering output
> and exit handling: output handling should come first.
Sorry, but I failed to parse this sentence. Can you reformulate it in
a simpler way?
> I don't understand why you'd want to make run_command not exit in
> case of failure, in case that was intentional.
Yes, it was intentional. I think it provides more flexibility (which
is useful for example in the test `init.test',). Moreover, in the
usual case, the test scripts are run with the `-e' flag on, so that an
"uncatched" failing `run_command' call makes the testcase fail.
In case `set -e' is not used, the testcase's writer should anyway be
prepared to add manually add `|| Exit 1' wherever deemed appropriate.
So I'd prefer not to ever call `Exit' in run_command. Objections?
> BTW2, do you have access to a Solaris or OpenBSD box to test this
> function with its shells?
Alas no, but at least I installed the "Heirloom Shell":
http://heirloom.sourceforge.net/sh.html
which should be quite similar to the Solaris Shell: no `$(...)' for
command substitution, no `${var#prefix}' expansions, common namespace
for variables and functions, `while ...; do ...; done <file' spawning a
subshell, and many other "goodies". I will test the final patch with
that shell (and of course with zsh too, which is the very reason to
have this patch in the first place).
>
> > > > # AUTOMAKE_run status [options...]
> > > > # --------------------------------
> > > > -# Run Automake with OPTIONS, and fail if automake
> > > > +# Run Automake with OPTIONS, and make the testcase FAIL if
> > > > automake
> > >
> > > Why this change? I'd drop it, likewise for the comment to
> > > AUTOMAKE_fails.
> >
> > It seemed clearer: it's not the function that it's failing, but
> > the whole testcase.
>
> OK, but please s/testcase/test/g, and the ChangeLog entry should
> mention this ((AUTOMAKE_run): Update comment.).
Done. Also added `(AUTOMAKE_fail): Updated comment.' to the ChangeLog.
> [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.
> > And another aside: I was about to modify also the file
> > `tests/dejagnu-p.test', before remembering it is automatically
> > generated (and, well, also before noticing it was a false
> > positive).
> >
> > I think it would be useful to make the autogoenerated files
> > (tests/defs, tests/*-p.test, etc) readonly. It could be done in
> > a separate patch. What do you think?
>
> Hmm, I'm a bit leery of making things read-only, but making sure
> the files contain a "generated by ... " line near the top seems a
> good idea.
In truth, I don't find that much useful in order to prevent
"unintentional" editing: I have ended up too many times modifying
tests/defs instead of tests/defs.in, even if it has the customary
`...@configure_input@' line at the top (yeah, stupid me, but I lost time
anyway). On the contrary, the fact that a file is read-only is a much
clearer and outstanding indicator of the fact it should not be
modified, and is anyway easily circumvented in the case you really
need to modify that file. I'm stressing this because I think that
making generated files readonly would be very very useful to the
absent-minded developer or contributor (e.g., me).
Regards,
Stefano
