bugzilla-tool post-diff inserts stderr into patch

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Shinichiro Hamaji]

After Bug 29316, bugzilla-tool post-diff inserts unnecessary stderr outputs into patches.

Maybe we should add an optional parameter into SCM.run_command to determine how to handle stderr and use the parameter in create_patch()?

[Eric Seidel (no email)]

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/scm.py#L133
is the line which is "wrong".  We should not be piping stderr to stdout but rather we should rout it to a separate buffer (including to system.stderr) in some circumstances.

[Zoltan Horvath]

Created attachment 43419 [details]
proposed patch

[Adam Barth]

Do we really need to proliferate [0] all over the code?  That's pretty ugly and error-prone.

[Zoltan Horvath]

No, we don't. We can give back only the stdout by default (with run_command). Currently we use stderr output only in one unittest. 
So this is also a possible way. If it's better I'll file a new patch at the morning.

[Eric Seidel (no email)]

Yeah, I think we should just go back to teh "just return stdout" model.  The one problem with this new implementation is that places which want both stderr and stdout ouput can't get them interlaced anymore. :(  I think that if scripts like run-webkit-tests fail we want to report both outputs.  Although I'm not sure that any of our scripts use both, so we could go with this change for now and see what harm it does.

I also need to fix run_command to know how to echo to the current stdout and stderr as well.

[Zoltan Horvath]

By the way, we can use [stderr] and [stdout] instead of [1], [0]. It's more clearly.

[Zoltan Horvath]

Created attachment 43493 [details]
other solution

[Adam Barth]

Comment on attachment 43493 [details]
other solution

I think we should do this.

[WebKit Commit Bot]

Comment on attachment 43493 [details]
other solution

Clearing flags on attachment: 43493

Committed r51230: <http://trac.webkit.org/changeset/51230>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Comment on attachment 43493 [details]
other solution

So the behavior of this is that stderr will now go to the parent's stderr.  Which is OK.  And might actually be better.  But if you intended for None to mean /dev/null, that is not the POpen behavior. :)

I think this is better than what we had before.  I think that some of our callsites definitely want the return_stderr behavior.

run-webkit-tests calls being one such callsite.