RESOLVED FIXED 29914
bugzilla-tool post-diff inserts stderr into patch 
https://bugs.webkit.org/show_bug.cgi?id=29914
Summary bugzilla-tool post-diff inserts stderr into patch
Shinichiro Hamaji
Reported 2009-09-30 00:48:46 PDT
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()?
Attachments
proposed patch (15.58 KB, patch)
2009-11-18 02:15 PST, Zoltan Horvath 		abarth: review-
DetailsFormatted DiffDiff
other solution (2.87 KB, patch)
2009-11-19 01:22 PST, Zoltan Horvath 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2009-10-28 14:55:37 PDT
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
Comment 2 2009-11-18 02:15:55 PST
Created attachment 43419 [details] proposed patch
Adam Barth
Comment 3 2009-11-18 08:21:50 PST
Do we really need to proliferate [0] all over the code? That's pretty ugly and error-prone.
Zoltan Horvath
Comment 4 2009-11-18 15:13:36 PST
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)
Comment 5 2009-11-18 15:16:59 PST
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
Comment 6 2009-11-18 15:27:20 PST
By the way, we can use [stderr] and [stdout] instead of [1], [0]. It's more clearly.
Zoltan Horvath
Comment 7 2009-11-19 01:22:48 PST
Created attachment 43493 [details] other solution
Adam Barth
Comment 8 2009-11-19 22:13:25 PST
Comment on attachment 43493 [details] other solution I think we should do this.
WebKit Commit Bot
Comment 9 2009-11-19 22:21:00 PST
Comment on attachment 43493 [details] other solution Clearing flags on attachment: 43493 Committed r51230: <http://trac.webkit.org/changeset/51230>
WebKit Commit Bot
Comment 10 2009-11-19 22:21:08 PST
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 11 2009-11-19 22:58:05 PST
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.
Note You need to log in before you can comment on or make changes to this bug.