Bug 33496 - webkit-commit-queue status page is confusing
Summary: webkit-commit-queue status page is confusing
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
: 33871 (view as bug list)
Depends on:
Reported: 2010-01-11 15:41 PST by Eric Seidel (no email)
Modified: 2010-06-10 20:35 PDT (History)
3 users (show)

See Also:

Patch (11.93 KB, patch)
2010-01-19 17:16 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (17.60 KB, patch)
2010-01-20 02:21 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (18.06 KB, patch)
2010-01-20 02:50 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-01-11 15:41:39 PST
webkit-commit-queue status page is confusing

<WildFox> eseidel: wondering about this commit-queue status page, when it says "patch 46256... from bug...: Fail" - what does that mean

Most of those "Fail" messages are temporary.  The page does not express that very well.
Comment 1 Eric Seidel (no email) 2010-01-12 16:09:28 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.
Comment 2 Eric Seidel (no email) 2010-01-13 11:30:13 PST
I've had two more complaints since about FAIL being confusing.  Need to improve status reporting here. :(
Comment 3 Eric Seidel (no email) 2010-01-19 17:16:34 PST
Created attachment 46964 [details]
Comment 4 Eric Seidel (no email) 2010-01-19 17:19:10 PST
*** Bug 33871 has been marked as a duplicate of this bug. ***
Comment 5 Adam Barth 2010-01-19 20:02:23 PST
Comment on attachment 46964 [details]

         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?
Comment 6 Eric Seidel (no email) 2010-01-19 20:36:43 PST
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:
Comment 7 Adam Barth 2010-01-19 21:16:06 PST
(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.
Comment 8 Eric Seidel (no email) 2010-01-19 21:25:37 PST
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. :)
Comment 9 Eric Seidel (no email) 2010-01-20 02:21:19 PST
Created attachment 46995 [details]
Comment 10 Eric Seidel (no email) 2010-01-20 02:50:20 PST
Created attachment 46997 [details]
Comment 11 Adam Barth 2010-01-20 02:55:27 PST
Comment on attachment 46997 [details]

This is much better
Comment 12 Eric Seidel (no email) 2010-01-20 03:21:34 PST
Committed r53537: <http://trac.webkit.org/changeset/53537>
Comment 13 Eric Seidel (no email) 2010-01-20 15:01:41 PST
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 14 WebKit Commit Bot 2010-01-20 19:09:50 PST
Comment on attachment 46997 [details]

Clearing flags on attachment: 46997

Committed r53593: <http://trac.webkit.org/changeset/53593>
Comment 15 WebKit Commit Bot 2010-01-20 19:09:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Eric Seidel (no email) 2010-01-20 19:17:07 PST
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...
Comment 17 Eric Seidel (no email) 2010-01-20 23:35:36 PST
About to roll this out again.  Filed a bug about webkit-patch rollout needing to clear flag when re-opening bugs.
Comment 18 Eric Seidel (no email) 2010-01-20 23:50:46 PST
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>