Changeset 53570 in webkit

Timestamp:

Jan 20, 2010 3:01:22 PM (14 years ago)

Author:

eric@webkit.org

Message:

2010-01-20 Eric Seidel <​eric@webkit.org>

No review, rolling out r53537.
​http://trac.webkit.org/changeset/53537
​https://bugs.webkit.org/show_bug.cgi?id=33496

Added a failure condition to the commit-queue and looks to
have broken the EWS bots

• Scripts/webkitpy/commands/early_warning_system.py:
• Scripts/webkitpy/commands/queues.py:
• Scripts/webkitpy/queueengine.py:
• Scripts/webkitpy/scm.py:
• Scripts/webkitpy/scm_unittest.py:

Location:

trunk/WebKitTools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/commands/early_warning_system.py (modified) (1 diff)
• Scripts/webkitpy/commands/queues.py (modified) (7 diffs)
• Scripts/webkitpy/queueengine.py (modified) (1 diff)
• Scripts/webkitpy/scm.py (modified) (3 diffs)
• Scripts/webkitpy/scm_unittest.py (modified) (3 diffs)

trunk/WebKitTools/ChangeLog

@@ +1 @@
+2010-01-20  Eric Seidel  <eric@webkit.org>
+
+        No review, rolling out r53537.
+        http://trac.webkit.org/changeset/53537
+        https://bugs.webkit.org/show_bug.cgi?id=33496
+
+        Added a failure condition to the commit-queue and looks to
+        have broken the EWS bots
+
+        * Scripts/webkitpy/commands/early_warning_system.py:
+        * Scripts/webkitpy/commands/queues.py:
+        * Scripts/webkitpy/queueengine.py:
+        * Scripts/webkitpy/scm.py:
+        * Scripts/webkitpy/scm_unittest.py:
+
 2010-01-20  Jon Honeycutt  <jhoneycutt@apple.com>
 

trunk/WebKitTools/Scripts/webkitpy/commands/early_warning_system.py

@@ -71 +71 @@
 
     @classmethod
-    def _report_patch_failure(cls, tool, patch, script_error):
+    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)
         results_link = tool.status_server.results_url_for_status(status_id)
-        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)
+        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)
 
 

trunk/WebKitTools/Scripts/webkitpy/commands/queues.py

@@ -107 +107 @@
         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"
@@ -128 +126 @@
 
     @classmethod
-    def _update_status_for_script_error(cls, tool, state, script_error, patch_has_failed_this_queue=True):
+    def _update_status_for_script_error(cls, tool, state, script_error, is_error=False):
         message = script_error.message
-        # 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:
+        if is_error:
             message = "Error: %s" % message
-        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)
+        return tool.status_server.update_status(cls.name, message, state["patch"], StringIO(script_error.output))
 
 
@@ -176 +160 @@
         return patches[0]
 
-    def _current_checkout_builds_and_passes_tests(self):
+    def _can_build_and_test(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
@@ -194 +179 @@
         if not self._builders_are_green():
             return False
-        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)
+        if not self._can_build_and_test():
             return False
         if not self._builders_are_green():
@@ -204 +186 @@
         return True
 
-    # FIXME: This should be unified with AbstractReviewQueue's implementation.  (Mostly _review_patch just needs a rename.)
     def process_work_item(self, patch):
-        # 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)
+        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
 
     def handle_unexpected_error(self, patch, 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.
+        self.committer_validator.reject_patch_from_commit_queue(patch.id(), message)
 
     # 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, 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.
+        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))
+
 
 class AbstractReviewQueue(AbstractQueue, PersistentPatchCollectionDelegate, StepSequenceErrorHandler):
@@ -260 +251 @@
 
     def process_work_item(self, patch):
-        self._review_patch(patch)
-        self._did_pass(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
 
     def handle_unexpected_error(self, patch, 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)
+        log(message)
 
     # StepSequenceErrorHandler methods
@@ -300 +282 @@
 
     @classmethod
-    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)
+    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)

trunk/WebKitTools/Scripts/webkitpy/queueengine.py

@@ -108 +108 @@
                     if e.exit_code == self.handled_error_code:
                         continue
-                    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())
+                    message = "Unexpected failure when landing patch!  Please file a bug against webkit-patch.\n%s" % e.message_with_output()
                     self._delegate.handle_unexpected_error(work_item, message)
             except KeyboardInterrupt, e:

trunk/WebKitTools/Scripts/webkitpy/scm.py

@@ -234 +234 @@
         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.
@@ -366 +363 @@
         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):
@@ -466 +460 @@
     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:

trunk/WebKitTools/Scripts/webkitpy/scm_unittest.py

@@ -205 +205 @@
         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)
@@ -469 +466 @@
         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()
@@ -563 +557 @@
     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):