Changeset 56964 in webkit

Timestamp:

Apr 1, 2010 9:31:54 PM (14 years ago)

Author:

abarth@webkit.org

Message:

2010-04-01 Adam Barth <​abarth@webkit.org>

Reviewed by Eric Seidel.

Improve the error handling in rollout a bit
​https://bugs.webkit.org/show_bug.cgi?id=36995

This patch does a few things to make the error handling in rollout a
bit more robust.

• Scripts/webkitpy/common/checkout/api.py:

The old logic here was wrong. We don't want to resolve the
ChangeLogs (that would remove the old ChangeLog entry). Instead,
we want to revert the ChangeLogs so we can fill them with the new
message.

• Scripts/webkitpy/tool/commands/download_unittest.py:

Update test expectations because we're using a different mock object.

• Scripts/webkitpy/tool/commands/download.py:
  • Added an update command to make updating from the SheriffBot more robust.
  • Now that we have CommitInfo, we can automatically CC the responsible parties on the bug we create.
  • Re-ordered the steps in create-rollout. Our original thinking was that we always wanted to create the bug, but that's not really true given how things appear to be playing out. If we fail to apply the reverse diff, we don't want to create the bug.
• Scripts/webkitpy/tool/commands/sheriffbot.py:
  • Use the new, more robust update command.
• Scripts/webkitpy/tool/steps/createbug.py:

Allow commands to pre-load who they want to be CCed on a new bug.

Location:

trunk/WebKitTools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/common/checkout/api.py (modified) (1 diff)
• Scripts/webkitpy/tool/commands/download.py (modified) (3 diffs)
• Scripts/webkitpy/tool/commands/download_unittest.py (modified) (1 diff)
• Scripts/webkitpy/tool/commands/sheriffbot.py (modified) (4 diffs)
• Scripts/webkitpy/tool/steps/createbug.py (modified) (1 diff)

trunk/WebKitTools/ChangeLog

@@ +1 @@
+2010-04-01  Adam Barth  <abarth@webkit.org>
+
+        Reviewed by Eric Seidel.
+
+        Improve the error handling in rollout a bit
+        https://bugs.webkit.org/show_bug.cgi?id=36995
+
+        This patch does a few things to make the error handling in rollout a
+        bit more robust.
+
+        * Scripts/webkitpy/common/checkout/api.py:
+            The old logic here was wrong.  We don't want to resolve the
+            ChangeLogs (that would remove the old ChangeLog entry).  Instead,
+            we want to revert the ChangeLogs so we can fill them with the new
+            message.
+        * Scripts/webkitpy/tool/commands/download_unittest.py:
+            Update test expectations because we're using a different mock object.
+        * Scripts/webkitpy/tool/commands/download.py:
+            - Added an update command to make updating from the SheriffBot more
+              robust.
+            - Now that we have CommitInfo, we can automatically CC the
+              responsible parties on the bug we create.
+            - Re-ordered the steps in create-rollout.  Our original thinking
+              was that we always wanted to create the bug, but that's not
+              really true given how things appear to be playing out.  If we
+              fail to apply the reverse diff, we don't want to create the bug.
+        * Scripts/webkitpy/tool/commands/sheriffbot.py:
+            - Use the new, more robust update command.
+        * Scripts/webkitpy/tool/steps/createbug.py:
+            Allow commands to pre-load who they want to be CCed on a new bug.
+
 2010-04-01  Kent Tamura  <tkent@chromium.org>
 

trunk/WebKitTools/Scripts/webkitpy/common/checkout/api.py

@@ -128 +128 @@
         self._scm.apply_reverse_diff(revision)
 
-        # Fix any ChangeLogs if necessary.
+        # We don't want to remove the old ChangeLog entry.
         changelog_paths = self.modified_changelogs()
         if len(changelog_paths):
-            # FIXME: Move _scm.script_path here once we get rid of all the dependencies.
-            run_command([self._scm.script_path('resolve-ChangeLogs')] + changelog_paths)
+            self._scm.revert_files(changelog_paths)
 
         conflicts = self._scm.conflicted_files()

trunk/WebKitTools/Scripts/webkitpy/tool/commands/download.py

@@ -44 +44 @@
 
 
+class Update(AbstractSequencedCommand):
+    name = "update"
+    help_text = "Update working copy (used internally)"
+    steps = [
+        steps.CleanWorkingDirectory,
+        steps.Update,
+    ]
+
+
 class Build(AbstractSequencedCommand):
     name = "build"
@@ -244 +253 @@
     argument_names = "REVISION REASON"
 
-    def _bug_id_for_revision(self, revision):
-        bug_id = self.tool.checkout().bug_id_for_revision(revision)
-        if bug_id:
-            log("Preparing rollout for bug %s." % bug_id)
-            return bug_id
+    def _commit_info(self, revision):
+        commit_info = self.tool.checkout().commit_info_for_revision(revision)
+        if commit_info.bug_id():
+            # Note: Don't print a bug URL here because it will confuse the
+            #       SheriffBot because the SheriffBot just greps the output
+            #       of create-rollout for bug URLs.  It should do better
+            #       parsing instead.
+            log("Preparing rollout for bug %s." % commit_info.bug_id())
+            return commit_info
         log("Unable to parse bug number from diff.")
 
     def _prepare_state(self, options, args, tool):
         revision = args[0]
+        commit_info = self._commit_info(revision)
+        cc_list = sorted([party.bugzilla_email()
+                          for party in commit_info.responsible_parties()
+                          if party.bugzilla_email()])
         return {
-            "revision" : revision,
-            "bug_id" : self._bug_id_for_revision(revision),
-            "reason" : args[1],
+            "revision": revision,
+            "bug_id": commit_info.bug_id(),
+            # FIXME: We should used the list as the canonical representation.
+            "bug_cc": ",".join(cc_list),
+            "reason": args[1],
         }
 
@@ -279 +298 @@
     help_text = "Creates a bug to track a broken SVN revision and uploads a rollout patch."
     steps = [
+        steps.CleanWorkingDirectory,
+        steps.Update,
+        steps.RevertRevision,
         steps.CreateBug,
-        steps.CleanWorkingDirectory,
-        steps.Update,
-        steps.RevertRevision, # Notice that we still create the bug if this step fails.
         steps.PrepareChangeLogForRevert,
         steps.PostDiffForRevert,

trunk/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py

@@ -118 +118 @@
 
     def test_prepare_rollout(self):
-        expected_stderr="Preparing rollout for bug 12345.\nUpdating working directory\nRunning prepare-ChangeLog\n"
+        expected_stderr = "Preparing rollout for bug 42.\nUpdating working directory\nRunning prepare-ChangeLog\n"
         self.assert_execute_outputs(PrepareRollout(), [852, "Reason"], options=self._default_options(), expected_stderr=expected_stderr)
 
     def test_create_rollout(self):
-        expected_stderr="Preparing rollout for bug 12345.\nMOCK create_bug\nbug_title: REGRESSION(r852): Reason\nbug_description: http://trac.webkit.org/changeset/852 broke the build:\nReason\nUpdating working directory\nRunning prepare-ChangeLog\n"
+        expected_stderr = "Preparing rollout for bug 42.\nUpdating working directory\nMOCK create_bug\nbug_title: REGRESSION(r852): Reason\nbug_description: http://trac.webkit.org/changeset/852 broke the build:\nReason\nRunning prepare-ChangeLog\n"
         self.assert_execute_outputs(CreateRollout(), [852, "Reason"], options=self._default_options(), expected_stderr=expected_stderr)
 
     def test_rollout(self):
-        expected_stderr = "Preparing rollout for bug 12345.\nUpdating working directory\nRunning prepare-ChangeLog\nBuilding WebKit\n"
+        expected_stderr = "Preparing rollout for bug 42.\nUpdating working directory\nRunning prepare-ChangeLog\nBuilding WebKit\n"
         self.assert_execute_outputs(Rollout(), [852, "Reason"], options=self._default_options(), expected_stderr=expected_stderr)
 

trunk/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py

@@ -34 +34 @@
 from webkitpy.tool.bot.sheriffircbot import SheriffIRCBot
 from webkitpy.tool.commands.queues import AbstractQueue
+from webkitpy.tool.commands.stepsequence import StepSequenceErrorHandler
 
-class SheriffBot(AbstractQueue):
+
+class SheriffBot(AbstractQueue, StepSequenceErrorHandler):
     name = "sheriff-bot"
 
-    def update(self):
-        self.tool.executive.run_and_throw_if_fail(WebKitPort.update_webkit_command(), quiet=True)
+    def _update(self):
+        self.run_webkit_patch(["update", "--force-clean", "--quiet"])
 
     # AbstractQueue methods
@@ -54 +56 @@
     def next_work_item(self):
         self._irc_bot.process_pending_messages()
-        self.update()
+        self._update()
         for svn_revision, builders in self.tool.buildbot.revisions_causing_failures().items():
             if self.tool.status_server.svn_revision(svn_revision):
@@ -75 +77 @@
         builders = failure_info["builders"]
 
-        self.update()
+        self._update()
         commit_info = self.tool.checkout().commit_info_for_revision(svn_revision)
         self._sheriff.post_irc_warning(commit_info, builders)
@@ -87 +89 @@
     def handle_unexpected_error(self, failure_info, message):
         log(message)
+
+    # StepSequenceErrorHandler methods
+
+    @classmethod
+    def handle_script_error(cls, tool, state, script_error):
+        # Ideally we would post some information to IRC about what went wrong
+        # here, but we don't have the IRC password in the child process.
+        pass

trunk/WebKitTools/Scripts/webkitpy/tool/steps/createbug.py

@@ -43 +43 @@
         if state.get("bug_id"):
             return
-        state["bug_id"] = self._tool.bugs.create_bug(state["bug_title"], state["bug_description"], blocked=state.get("bug_blocked"), component=self._options.component, cc=self._options.cc)
+        cc = self._options.cc
+        if not cc:
+            cc = state["bug_cc"]
+        state["bug_id"] = self._tool.bugs.create_bug(state["bug_title"], state["bug_description"], blocked=state.get("bug_blocked"), component=self._options.component, cc=cc)