* Stefano Lattarini wrote on Sat, Oct 10, 2009 at 03:42:52AM CEST:
> At Friday 09 October 2009, Ralf Wildenhues wrote:
> > I'd shorten this to:
> >
> > Use run_command throughout.
> I'd prefer to keep it a bit more verbose. I settled for this:
> "Use new subroutine run_command instead of hand-crafted redirections
> of stdout and/or stderr."
> Are you OK with this?
Sure.
> > 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.
> > > +run_CMD ()
> > > +{
> > > + # NOTE: all internal variables used here starts with the
> > > `_run' + # prefix, to minimize possibility of name clashes with
> > > global + # variables defined in user code.
> > > + : 'entering run_CMD(): become quiet'
> > > + set +x # xtrace verbosity stops here
> >
> > No need to comment the obvious, here as well as at the end of this
> > function; these 5 lines can be replaced with
> > set +x
> Well, I thought that things were clearer the way I did, but if more
> experienced developers find that comments to be too "patronizing" or
> annoying, I think it's better to remove them. I'd just like to keep
> two comments:
> set +x # xtrace verbosity temporarly disabled in `run_command'
> at the beginning, and:
> set -x # reactivating temporarly turned-off xtrace verbosity
> at the end. Objections?
Fine with me.
> > If you care about reusability of this function in a context where
> > you can't be sure whether xtrace is enabled at this point, you can
> > use something like this instead:
> > case $i in *x*) run_xtrace=;; *) run_xtrace=:;; esac
> > $run_xtrace set +x
> > ...
> > $run_xtrace set -x
> Well, ATM I'd prefer to keep the function simpler. It can be made
> more reusable and general later, if the need will arise.
> What do you think?
Sure.
> > > + _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
> > > + 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".
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 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.
I don't understand why you'd want to make run_command not exit in case
of failure, if case that was intentional.
BTW2, do you have access to a Solaris or OpenBSD box to test this
function with its shells?
> > > # 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.).
> > > # does not exit with STATUS.
> > > AUTOMAKE_run ()
> > > {
> > > - expected_exitcode=$1
> > > + _am_run_expected_exitcode=$1
> or is OK to use `am_run_expected_exitcode' instead
> of `_am_run_expected_exitcode'?
Sure.
> 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.
> 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.
Thanks,
Ralf
