Summary: | webkit-commit-queue status page is confusing | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | REOPENED --- | ||||||||||
Severity: | Normal | CC: | abarth, commit-queue, zimmermann | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2010-01-11 15:41:39 PST
This is actually partially teh commit-queue bots fault. It shouldn't post "FAIL" in those cases, it shoudl post "Error" when the failure is temporary. I've had two more complaints since about FAIL being confusing. Need to improve status reporting here. :( Created attachment 46964 [details]
Patch
*** Bug 33871 has been marked as a duplicate of this bug. *** Comment on attachment 46964 [details]
Patch
if is_svn_apply:
- QueueEngine.exit_after_handled_error(script_error)
+ return # No need to update the bug for svn-apply failures.
[...]
- exit(1)
How is the master process supposed to distinguish these cases if we don't return a different exit code?
Why does the master process need to care? We've updated the server, the master process will notice next time it queries the server. Exiting 1 will cause the master process to also update the server, because it thinks things failed. The master process doesnt' need to do anything in that case. This is the only place that handle_script_error is called: http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/stepsequence.py#L76 (In reply to comment #6) > Why does the master process need to care? The master process decides whether to log a "FAIL" message to the server depending on the return code used by the child process. If we land this change, the purple bubbles will become red because the master will not know that the child was able to handle svn-apply failures. exit code 1 should cause FAIL. 2 should cause nothing because the child handled it. You're seeing something I"m not, or you're reading the code backwards. :) Created attachment 46995 [details]
Patch
Created attachment 46997 [details]
Patch
Comment on attachment 46997 [details]
Patch
This is much better
Committed r53537: <http://trac.webkit.org/changeset/53537> Reverted r53537 for reason: Added a failure condition to the commit-queue and looks to have broken the EWS bots Committed r53570: <http://trac.webkit.org/changeset/53570> Comment on attachment 46997 [details] Patch Clearing flags on attachment: 46997 Committed r53593: <http://trac.webkit.org/changeset/53593> All reviewed patches have been landed. Closing bug. Bah! This is why we clear flags. The commit-queeu just re-landed what I had rolled out. I guess I'll just fix it this time... About to roll this out again. Filed a bug about webkit-patch rollout needing to clear flag when re-opening bugs. Reverted r53593 for reason: Re-rollout this patch, the commit-queue should not have landed it again, but it did due to land-diff and rollout both not clearing flags. Committed r53609: <http://trac.webkit.org/changeset/53609> |