On Thu, 2017-06-22 at 23:07 +0200, Patrick Ohly wrote:
> On Thu, 2017-06-22 at 15:47 -0500, Leonardo Sandoval wrote:
> > On Thu, 2017-06-22 at 19:39 +0200, Patrick Ohly wrote:
> > > On Thu, 2017-06-22 at 11:18 -0500, Leonardo Sandoval wrote:
> > > > On Thu, 2017-06-22 at 17:59 +0200, Patrick Ohly wrote:
> > > > > On Thu, 2017-06-22 at 10:37 -0500, Leonardo Sandoval wrote:
> > > > > > On Thu, 2017-06-22 at 17:14 +0200, Patrick Ohly wrote:
> > > > > > > On Thu, 2017-06-22 at 09:58 -0500, Leonardo Sandoval wrote:
> > > > > > > > On Thu, 2017-06-22 at 16:17 +0200, Patrick Ohly wrote:
> > > > > > > > > On Mon, 2017-06-19 at 07:39 -0700,
> > > > > > > > > leonardo.sandoval.gonza...@linux.intel.com wrote:
> > > > > > > > > > From: Leonardo Sandoval
> > > > > > > > > > <leonardo.sandoval.gonza...@linux.intel.com>
> > > > > > > > > >
> > > > > > > > > > Do not mix the stderr into stdout, allowing test cases to
> > > > > > > > > > query
> > > > > > > > > > the specific output.
> > > > > > > > >
> > > > > > > > > This changes the behavior of functions that are also used
> > > > > > > > > outside of
> > > > > > > > > OE-core in a way that won't be easy to notice. I also don't
> > > > > > > > > think that
> > > > > > > > > it is the right default. For example, for bitbake it is
> > > > > > > > > easier to
> > > > > > > > > understand where an error occurred when stderr goes to the
> > > > > > > > > same stream
> > > > > > > > > as stdout.
> > > > > > > >
> > > > > > > > how would that make it easier?
> > > > > > >
> > > > > > > Because then output will be properly interleaved, as it would be
> > > > > > > on a
> > > > > > > console.
> > > > > > >
> > > > > > > Actually, the entire error reporting in runCmd() only prints
> > > > > > > result.output, so with stderr going to result.error by default,
> > > > > > > you
> > > > > > > won't get the actual errors reported anymore at all, will you?
> > > > > > >
> > > > > >
> > > > > > process stderr will go into result.error and process stdout into
> > > > > > result.output. So when the process is executed ignoring the return
> > > > > > status, then test must check result.error. I find the latter cleaner
> > > > > > that checking errors into stdout.
> > > > >
> > > > > It depends on how the result is used. That you prefer split output for
> > > > > some tests does not mean that everyone wants the same in their tests.
> > > > > I
> > > > > don't want it in my own usage of runCmd() or bitbake() because I don't
> > > > > care about where a message was printed. I just want it in proper
> > > > > order.
> > > > >
> > > > > If you change the default, then you will also have to enhance
> > > > > runCmd()'s
> > > > > error handling to include results.error. That's currently missing in
> > > > > your patch.
> > > >
> > > > it is not missing, it is on 2/2
> > >
> > > I'm talking about this code:
> > >
> > > def runCmd(command, ignore_status=False, timeout=None, assert_error=True,
> > > native_sysroot=None, limit_exc_output=0, **options):
> > > ...
> > > if result.status and not ignore_status:
> > > exc_output = result.output
> > > if limit_exc_output > 0:
> > > split = result.output.splitlines()
> > > if len(split) > limit_exc_output:
> > > exc_output = "\n... (last %d lines of output)\n" %
> > > limit_exc_output + \
> > > '\n'.join(split[-limit_exc_output:])
> > > if assert_error:
> > > raise AssertionError("Command '%s' returned non-zero exit
> > > status %d:\n%s" % (command, result.status, exc_output))
> > > else:
> > > raise CommandError(result.status, command, exc_output)
> > >
> >
> >
> >
> >
> > > You are not extending that in either 2/2, are you? At the moment, when a
> > > command fails, one gets stdout+stderr. With your path, one only gets
> > > stdout, which typically won't have the error message that caused the
> > > non-zero status.
> >
> > that is not true. I tested my patch and all tests are green.
>
> That's not addressing the point that I raised. I am pointing out a
> functional deficiency in runCmd that is caused by the first patch.
> Probably there are no tests which rely on the AssertionError, so you
> won't see test failures due to the changed exception message. But when a
> command fails unexpectedly, the error reporting will be incomplete.
>
> The exception is supposed to explain why a command failed. With your
> patch, it doesn't achieve that goal anymore because error messages of
> the command are not included (only stdout is).
>
> Regarding your argument that "all tests are green": you are changing the
> API of oeqa in a way that made it necessary to change tests in OE-core.
> Other layers will be affected the same way. You haven't run "all tests"
> that use oeqa, so you can't know that they "are green".
>
fair enough. I just tested with poky, that is my tiny world.
> Just as an aside, your patch series breaks testing of OE-core (in the
> first commit) and fixes that (in the second). That's bad for bisecting.
> You would have to combine both changes in one commit to avoid that.
>
as I mentioned before, you noticed (and I agreed) that the series needed
a refactor and I was going to send a v2 in case needed (atomicity was
not meet at the series).
Leo
> > If you look
> > at the code, the 'if len(split) > limit)exc)output' body is not
> > changing the result object, so what you get from cmd.run() is what what
> > is it returned.
>
> But it's not the same result as before, so you are changing a public API
> of OE-core.
>
--
_______________________________________________
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-core
