[SCM] WebKit Debian packaging branch, webkit-1.1, updated. upstream/1.1.19-706-ge5415e9
eric at webkit.org
eric at webkit.org
Thu Feb 4 21:21:31 UTC 2010
The following commit has been merged in the webkit-1.1 branch:
commit 1447342c097d8b82a3953a4f4ff5f7424bc8ff2c
Author: eric at webkit.org <eric at webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date: Wed Jan 20 11:20:32 2010 +0000
2010-01-20 Eric Seidel <eric at webkit.org>
Reviewed by Adam Barth.
webkit-commit-queue status page is confusing
https://bugs.webkit.org/show_bug.cgi?id=33496
This should improve the status page by removing more Fail messages.
To do this, I re-factored the CommitQueue and the AbstractReviewQueues
to behave more like one another. This meant moving where the failure reporting was done.
Previously the AbstractReviewQueue always used the parent process to report the error,
while CommitQueue used the subprocess when possible, and the parent only reported errors
that we didn't know how to handle (bugs in the commit-queue itself).
Now the AbstractReviewQueue follow's the commit-queue's model. This got rid of a try-block
in both implementations and required teaching handle_script_error in each to post Fail messages
to the status server instead of calling exit(1).
This will also make the style-queue share more bug posting logic with other queues:
https://bugs.webkit.org/show_bug.cgi?id=33871
* Scripts/webkitpy/commands/early_warning_system.py:
- Don't exit(1) as that will cause the calling queue to also report Fail to the status server.
Implementors of handle_script_error are expected to update the status server if needed, but only exit if the error could not be handled.
So we instead pass patch_has_failed_this_queue=True to _update_status_for_script_error in the case that this was a real failure.
_update_status_for_script_error knows how to post the Fail message to the status server.
- Teach _update_status_for_script_error how to post Fail messages to the status server.
* Scripts/webkitpy/commands/queues.py:
- Remove the try block from process_work_item since the caller already has one.
- Only CC watchers on failure to cut down on commit-queue generated mail.
- handle_unexpected_error needs to mark _did_fail now that the try block is gone from process_work_item.
- Abstract _format_script_error_output_for_bug to share code between all queues.
- The new _format_script_error_output_for_bug allows the style-queue to share the posting limit with other queues, as well as support linking to the full output.
- Rename _can_build_and_test to _current_checkout_builds_and_passes_tests to better explain what revision it's testing.
- Move logging out of _can_build_and_test and make the logs explain what revision we're testing.
- handle_script_error now posts Fail instead of the try block in process_work_item handling it.
* Scripts/webkitpy/queueengine.py:
- QueueEngine is no longer used just by the commit-queue, update the logging to say "processing" instead of landing.
* Scripts/webkitpy/scm.py:
- Add new checkout_revision function.
* Scripts/webkitpy/scm_unittest.py:
- Test our new checkout_revision function.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@53537 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
index 122d829..807378a 100644
--- a/WebKitTools/ChangeLog
+++ b/WebKitTools/ChangeLog
@@ -1,3 +1,45 @@
+2010-01-20 Eric Seidel <eric at webkit.org>
+
+ Reviewed by Adam Barth.
+
+ webkit-commit-queue status page is confusing
+ https://bugs.webkit.org/show_bug.cgi?id=33496
+
+ This should improve the status page by removing more Fail messages.
+ To do this, I re-factored the CommitQueue and the AbstractReviewQueues
+ to behave more like one another. This meant moving where the failure reporting was done.
+ Previously the AbstractReviewQueue always used the parent process to report the error,
+ while CommitQueue used the subprocess when possible, and the parent only reported errors
+ that we didn't know how to handle (bugs in the commit-queue itself).
+ Now the AbstractReviewQueue follow's the commit-queue's model. This got rid of a try-block
+ in both implementations and required teaching handle_script_error in each to post Fail messages
+ to the status server instead of calling exit(1).
+
+ This will also make the style-queue share more bug posting logic with other queues:
+ https://bugs.webkit.org/show_bug.cgi?id=33871
+
+ * Scripts/webkitpy/commands/early_warning_system.py:
+ - Don't exit(1) as that will cause the calling queue to also report Fail to the status server.
+ Implementors of handle_script_error are expected to update the status server if needed, but only exit if the error could not be handled.
+ So we instead pass patch_has_failed_this_queue=True to _update_status_for_script_error in the case that this was a real failure.
+ _update_status_for_script_error knows how to post the Fail message to the status server.
+ - Teach _update_status_for_script_error how to post Fail messages to the status server.
+ * Scripts/webkitpy/commands/queues.py:
+ - Remove the try block from process_work_item since the caller already has one.
+ - Only CC watchers on failure to cut down on commit-queue generated mail.
+ - handle_unexpected_error needs to mark _did_fail now that the try block is gone from process_work_item.
+ - Abstract _format_script_error_output_for_bug to share code between all queues.
+ - The new _format_script_error_output_for_bug allows the style-queue to share the posting limit with other queues, as well as support linking to the full output.
+ - Rename _can_build_and_test to _current_checkout_builds_and_passes_tests to better explain what revision it's testing.
+ - Move logging out of _can_build_and_test and make the logs explain what revision we're testing.
+ - handle_script_error now posts Fail instead of the try block in process_work_item handling it.
+ * Scripts/webkitpy/queueengine.py:
+ - QueueEngine is no longer used just by the commit-queue, update the logging to say "processing" instead of landing.
+ * Scripts/webkitpy/scm.py:
+ - Add new checkout_revision function.
+ * Scripts/webkitpy/scm_unittest.py:
+ - Test our new checkout_revision function.
+
2010-01-20 Adam Barth <abarth at webkit.org>
Reviewed by Darin Adler.
diff --git a/WebKitTools/Scripts/webkitpy/commands/early_warning_system.py b/WebKitTools/Scripts/webkitpy/commands/early_warning_system.py
index e3e14dd..110c67e 100644
--- a/WebKitTools/Scripts/webkitpy/commands/early_warning_system.py
+++ b/WebKitTools/Scripts/webkitpy/commands/early_warning_system.py
@@ -70,15 +70,10 @@ class AbstractEarlyWarningSystem(AbstractReviewQueue):
patch.id()])
@classmethod
- def handle_script_error(cls, tool, state, script_error):
- is_svn_apply = script_error.command_name() == "svn-apply"
- status_id = cls._update_status_for_script_error(tool, state, script_error, is_error=is_svn_apply)
- if is_svn_apply:
- QueueEngine.exit_after_handled_error(script_error)
+ def _report_patch_failure(cls, tool, patch, script_error):
results_link = tool.status_server.results_url_for_status(status_id)
- message = "Attachment %s did not build on %s:\nBuild output: %s" % (state["patch"].id(), cls.port_name, results_link)
- tool.bugs.post_comment_to_bug(state["patch"].bug_id(), message, cc=cls.watchers)
- exit(1)
+ message = "Attachment %s did not build on %s:\nBuild output: %s" % (patch.id(), cls.port_name, results_link)
+ tool.bugs.post_comment_to_bug(patch.bug_id(), message, cc=cls.watchers)
class GtkEWS(AbstractEarlyWarningSystem):
diff --git a/WebKitTools/Scripts/webkitpy/commands/queues.py b/WebKitTools/Scripts/webkitpy/commands/queues.py
index 350c1ea..1346a2e 100644
--- a/WebKitTools/Scripts/webkitpy/commands/queues.py
+++ b/WebKitTools/Scripts/webkitpy/commands/queues.py
@@ -106,6 +106,8 @@ class AbstractQueue(Command, QueueEngineDelegate):
def process_work_item(self, work_item):
raise NotImplementedError, "subclasses must implement"
+ # Subclasses are expected to post the message somewhere it can be read by a human.
+ # They should also prevent the queue from processing this patch again.
def handle_unexpected_error(self, work_item, message):
raise NotImplementedError, "subclasses must implement"
@@ -125,11 +127,25 @@ class AbstractQueue(Command, QueueEngineDelegate):
return engine(self.name, self).run()
@classmethod
- def _update_status_for_script_error(cls, tool, state, script_error, is_error=False):
+ def _update_status_for_script_error(cls, tool, state, script_error, patch_has_failed_this_queue=True):
message = script_error.message
- if is_error:
+ # Error: turns the status bubble purple and means that the error was such that we can't tell if the patch fails or not.
+ if not patch_has_failed_this_queue:
message = "Error: %s" % message
- return tool.status_server.update_status(cls.name, message, state["patch"], StringIO(script_error.output))
+ status_id = tool.status_server.update_status(cls.name, message, state["patch"], StringIO(script_error.output))
+ # In the case we know that this patch actually failed the queue, we make a second status entry (mostly because the server doesn't
+ # understand messages prefixed with "Fail:" to mean failures, and thus would leave the status bubble yellow instead of red).
+ # FIXME: Fail vs. Error should really be stored on a separate field from the message on the server.
+ if patch_has_failed_this_queue:
+ tool.status_server.update_status(cls.name, cls._fail_status)
+ return status_id # Return the status id to the original message with all the script output.
+
+ @classmethod
+ def _format_script_error_output_for_bug(tool, status_id, script_error):
+ if not script_error.output:
+ return script_error.message_with_output()
+ results_link = tool.status_server.results_url_for_status(status_id)
+ return "%s\nFull output: %s" % (script_error.message_with_output(), results_link)
class CommitQueue(AbstractQueue, StepSequenceErrorHandler):
@@ -159,11 +175,10 @@ class CommitQueue(AbstractQueue, StepSequenceErrorHandler):
self.log_progress([patch.id() for patch in patches])
return patches[0]
- def _can_build_and_test(self):
+ def _current_checkout_builds_and_passes_tests(self):
try:
self.run_webkit_patch(["build-and-test", "--force-clean", "--non-interactive", "--build-style=both", "--quiet"])
except ScriptError, e:
- self._update_status("Unabled to successfully build and test", None)
return False
return True
@@ -178,43 +193,37 @@ class CommitQueue(AbstractQueue, StepSequenceErrorHandler):
def should_proceed_with_work_item(self, patch):
if not self._builders_are_green():
return False
- if not self._can_build_and_test():
+ current_revision = self.tool.scm().checkout_revision()
+ self._update_status("Building and testing r%s before landing patch" % current_revision, patch)
+ if not self._current_checkout_builds_and_passes_tests():
+ self._update_status("Failed to build and test r%s, will try landing later" % current_revision, patch)
return False
if not self._builders_are_green():
return False
self._update_status("Landing patch", patch)
return True
+ # FIXME: This should be unified with AbstractReviewQueue's implementation. (Mostly _review_patch just needs a rename.)
def process_work_item(self, patch):
- try:
- self._cc_watchers(patch.bug_id())
- # We pass --no-update here because we've already validated
- # that the current revision actually builds and passes the tests.
- # If we update, we risk moving to a revision that doesn't!
- self.run_webkit_patch(["land-attachment", "--force-clean", "--non-interactive", "--no-update", "--parent-command=commit-queue", "--build-style=both", "--quiet", patch.id()])
- self._did_pass(patch)
- except ScriptError, e:
- self._did_fail(patch)
- raise e
+ # We pass --no-update here because we've already validated
+ # that the current revision actually builds and passes the tests.
+ # If we update, we risk moving to a revision that doesn't!
+ self.run_webkit_patch(["land-attachment", "--force-clean", "--non-interactive", "--no-update", "--parent-command=commit-queue", "--build-style=both", "--quiet", patch.id()])
+ self._did_pass(patch)
def handle_unexpected_error(self, patch, message):
- self.committer_validator.reject_patch_from_commit_queue(patch.id(), message)
+ self._did_fail(patch)
+ self._cc_watchers(patch.bug_id())
+ self.committer_validator.reject_patch_from_commit_queue(patch.id(), message) # Should instead pass cls.watchers as a CC list here.
# StepSequenceErrorHandler methods
- @staticmethod
- def _error_message_for_bug(tool, status_id, script_error):
- if not script_error.output:
- return script_error.message_with_output()
- results_link = tool.status_server.results_url_for_status(status_id)
- return "%s\nFull output: %s" % (script_error.message_with_output(), results_link)
-
@classmethod
def handle_script_error(cls, tool, state, script_error):
- status_id = cls._update_status_for_script_error(tool, state, script_error)
- validator = CommitterValidator(tool.bugs)
- validator.reject_patch_from_commit_queue(state["patch"].id(), cls._error_message_for_bug(tool, status_id, script_error))
-
+ status_id = cls._update_status_for_script_error(tool, state, script_error, patch_has_failed_this_queue=True)
+ script_error_output = cls._format_script_error_output_for_bug(tool, status_id, script_error)
+ self._cc_watchers(patch.bug_id())
+ CommitterValidator(tool.bugs).reject_patch_from_commit_queue(state["patch"].id(), script_error_output) # Should instead pass cls.watchers as a CC list here.
class AbstractReviewQueue(AbstractQueue, PersistentPatchCollectionDelegate, StepSequenceErrorHandler):
def __init__(self, options=None):
@@ -250,16 +259,25 @@ class AbstractReviewQueue(AbstractQueue, PersistentPatchCollectionDelegate, Step
raise NotImplementedError, "subclasses must implement"
def process_work_item(self, patch):
- try:
- self._review_patch(patch)
- self._did_pass(patch)
- except ScriptError, e:
- if e.exit_code != QueueEngine.handled_error_code:
- self._did_fail(patch)
- raise e
+ self._review_patch(patch)
+ self._did_pass(patch)
def handle_unexpected_error(self, patch, message):
- log(message)
+ log(message) # Unit tests expect us to log, we could eventually remove this since post_comment_to_bug will log as well.
+ self._did_fail(patch)
+ self.tool.bugs.post_comment_to_bug(patch.bug_id(), message, cc=self.watchers)
+
+ @classmethod
+ def _report_patch_failure(cls, tool, patch, script_error):
+ raise NotImplementedError, "subclasses must implement"
+
+ @classmethod
+ def handle_script_error(cls, tool, state, script_error):
+ patch_has_failed = script_error.command_name() != "svn-apply" # svn-apply failures do not necessarily mean the patch would fail to build.
+ status_id = cls._update_status_for_script_error(tool, state, script_error, patch_has_failed_this_queue=patch_has_failed)
+ if patch_has_failed:
+ return # No need to update the bug for svn-apply failures.
+ cls._report_patch_failure(tool, patch, script_error)
# StepSequenceErrorHandler methods
@@ -281,11 +299,14 @@ class StyleQueue(AbstractReviewQueue):
self.run_webkit_patch(["check-style", "--force-clean", "--non-interactive", "--parent-command=style-queue", patch.id()])
@classmethod
- def handle_script_error(cls, tool, state, script_error):
- is_svn_apply = script_error.command_name() == "svn-apply"
- status_id = cls._update_status_for_script_error(tool, state, script_error, is_error=is_svn_apply)
- if is_svn_apply:
- QueueEngine.exit_after_handled_error(script_error)
- message = "Attachment %s did not pass %s:\n\n%s\n\nIf any of these errors are false positives, please file a bug against check-webkit-style." % (state["patch"].id(), cls.name, script_error.message_with_output(output_limit=3*1024))
- tool.bugs.post_comment_to_bug(state["patch"].bug_id(), message, cc=cls.watchers)
- exit(1)
+ def _style_error_message_for_bug(cls, patch_id, script_error_output):
+ check_style_command = "check-webkit-style"
+ message_header = "Attachment %s did not pass %s:" % (patch_id, cls.name)
+ message_footer = "If any of these errors are false positives, please file a bug against %s." % check_style_command
+ return "%s\n\n%s\n\n%s" % (message_header, script_error_output, message_footer)
+
+ @classmethod
+ def _report_patch_failure(cls, tool, patch, script_error):
+ script_error_output = self._format_script_error_output_for_bug(tool, status_id, script_error)
+ bug_message = cls._style_error_message_for_bug(patch.id(), script_error_output)
+ tool.bugs.post_comment_to_bug(patch.bug_id(), message, cc=cls.watchers)
diff --git a/WebKitTools/Scripts/webkitpy/queueengine.py b/WebKitTools/Scripts/webkitpy/queueengine.py
index d14177d..fe11ca5 100644
--- a/WebKitTools/Scripts/webkitpy/queueengine.py
+++ b/WebKitTools/Scripts/webkitpy/queueengine.py
@@ -107,7 +107,9 @@ class QueueEngine:
# handled in the child process and we should just keep looping.
if e.exit_code == self.handled_error_code:
continue
- message = "Unexpected failure when landing patch! Please file a bug against webkit-patch.\n%s" % e.message_with_output()
+ tool_name = "webkit-patch" # QueueEngine has no way to get to tool.name() at current.
+ message_header = "Unexpected failure while processing patch! Please file a bug against %s." % tool_name
+ message = "%s\n\n%s" % (message_header, e.message_with_output())
self._delegate.handle_unexpected_error(work_item, message)
except KeyboardInterrupt, e:
log("\nUser terminated queue.")
diff --git a/WebKitTools/Scripts/webkitpy/scm.py b/WebKitTools/Scripts/webkitpy/scm.py
index 743f3fe..4a81b7d 100644
--- a/WebKitTools/Scripts/webkitpy/scm.py
+++ b/WebKitTools/Scripts/webkitpy/scm.py
@@ -233,6 +233,9 @@ class SCM:
def last_svn_commit_log(self):
raise NotImplementedError, "subclasses must implement"
+ def checkout_revision(self):
+ raise NotImplementedError, "subclasses must implement"
+
# Subclasses must indicate if they support local commits,
# but the SCM baseclass will only call local_commits methods when this is true.
@staticmethod
@@ -362,6 +365,9 @@ class SVN(SCM):
# http://svnbook.red-bean.com/en/1.0/ch03s03.html
return self.svn_commit_log('BASE')
+ def checkout_revision(self):
+ return int(self.value_from_svn_info(self.checkout_root, 'Revision'))
+
# All git-specific logic should go here.
class Git(SCM):
def __init__(self, cwd, dryrun=False):
@@ -460,6 +466,9 @@ class Git(SCM):
def last_svn_commit_log(self):
return run_command(['git', 'svn', 'log', '--limit=1'])
+ def checkout_revision(self):
+ return int(run_command(["git", "svn", "find-rev", "HEAD"]).rstrip())
+
# Git-specific methods:
def create_patch_from_local_commit(self, commit_id):
diff --git a/WebKitTools/Scripts/webkitpy/scm_unittest.py b/WebKitTools/Scripts/webkitpy/scm_unittest.py
index 73faf40..9859598 100644
--- a/WebKitTools/Scripts/webkitpy/scm_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/scm_unittest.py
@@ -204,6 +204,9 @@ class SCMTest(unittest.TestCase):
self.assertTrue(re.search('test2', r3_patch))
self.assertTrue(re.search('test2', self.scm.diff_for_revision(2)))
+ def _shared_test_checkout_revision(self):
+ self.assertEqual(self.scm.checkout_revision(), 4)
+
def _shared_test_svn_apply_git_patch(self):
self._setup_webkittools_scripts_symlink(self.scm)
git_binary_addition = """diff --git a/fizzbuzz7.gif b/fizzbuzz7.gif
@@ -465,6 +468,9 @@ Q1dTBx0AAAB42itg4GlgYJjGwMDDyODMxMDw34GBgQEAJPQDJA==
def test_diff_for_revision(self):
self._shared_test_diff_for_revision()
+ def test_checkout_revision(self):
+ self._shared_test_checkout_revision()
+
def test_svn_apply_git_patch(self):
self._shared_test_svn_apply_git_patch()
@@ -557,6 +563,9 @@ class GitTest(SCMTest):
def test_diff_for_revision(self):
self._shared_test_diff_for_revision()
+ def test_checkout_revision(self):
+ self._shared_test_checkout_revision()
+
def test_svn_apply_git_patch(self):
self._shared_test_svn_apply_git_patch()
--
WebKit Debian packaging
More information about the Pkg-webkit-commits
mailing list