Re: [PATCH 1/2] commands: send stderr to a new pipe

[Leonardo Sandoval <leonardo.sandoval.gonzalez@linux.intel.com>]

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.gonzalez@linux.intel.com wrote:
> > > > > > From: Leonardo Sandoval <leonardo.sandoval.gonzalez@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, but yes, the latter should be on 1/2. I
can re-factor and send v2 but lets see other opinions.

Leo

> 
> > > > > Can't you keep the current semantic and just override it explicitly in
> > > > > those tests that need separate stdout/stderr?
> > > > > 
> > > > 
> > > > My proposed patch was mainly based on a RP's comment [1], suggesting to
> > > > split std[out|err].
> > > 
> > > He did not suggest to change the default behavior. I agree that using
> > > split stdout/stderr in those specific tests which specifically want to
> > > check for error messages makes sense, but only in those tests.
> > 
> > No tests require splitting the output (all tests pass with and without
> > this series). The series is actually an enhancement and without it, we
> > saw (specially when the python 2 to 3 was going on) past warnings going
> > into stdout, so Chris and RP suggested the approach.
> 
> Richard, can you please comment on whether changing the default is
> really what you meant?
>