Changeset 52131 in webkit

Timestamp:

Dec 14, 2009 9:32:03 PM (14 years ago)

Author:

abarth@webkit.org

Message:

2009-12-14 Adam Barth <​abarth@webkit.org>

Reviewed by Eric Seidel.

[bzt] Kill WebKitApplyingScripts
​https://bugs.webkit.org/show_bug.cgi?id=32467

Ah! I've been wanting to do this for a long time. This patch brings
the applying commands into the patch processing fold.

• Scripts/bugzilla-tool:
• Scripts/modules/buildsteps.py:
• Scripts/modules/commands/download.py:
• Scripts/modules/commands/download_unittest.py:
• Scripts/modules/mock_bugzillatool.py:

Location:

trunk/WebKitTools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/bugzilla-tool (modified) (2 diffs)
• Scripts/modules/buildsteps.py (modified) (4 diffs)
• Scripts/modules/commands/download.py (modified) (3 diffs)
• Scripts/modules/commands/download_unittest.py (modified) (2 diffs)

trunk/WebKitTools/ChangeLog

@@ +1 @@
+2009-12-14  Adam Barth  <abarth@webkit.org>
+
+        Reviewed by Eric Seidel.
+
+        [bzt] Kill WebKitApplyingScripts
+        https://bugs.webkit.org/show_bug.cgi?id=32467
+
+        Ah!  I've been wanting to do this for a long time.  This patch brings
+        the applying commands into the patch processing fold.
+
+        * Scripts/bugzilla-tool:
+        * Scripts/modules/buildsteps.py:
+        * Scripts/modules/commands/download.py:
+        * Scripts/modules/commands/download_unittest.py:
+        * Scripts/modules/mock_bugzillatool.py:
+
 2009-12-14  Adam Barth  <abarth@webkit.org>
 

trunk/WebKitTools/Scripts/bugzilla-tool

@@ -35 +35 @@
 from modules.bugzilla import Bugzilla
 from modules.buildbot import BuildBot
-from modules.buildsteps import BuildSteps
 from modules.commands.download import *
 from modules.commands.early_warning_system import *
@@ -56 +55 @@
         self._scm = None
         self._status = None
-        self.steps = BuildSteps()
 
     def dry_run_callback(self, option, opt, value, parser):

trunk/WebKitTools/Scripts/modules/buildsteps.py

@@ -45 +45 @@
     parent_command = make_option("--parent-command", action="store", dest="parent_command", default=None, help="(Internal) The command that spawned this instance.")
     update = make_option("--no-update", action="store_false", dest="update", default=True, help="Don't update the working directory.")
+    local_commit = make_option("--local-commit", action="store_true", dest="local_commit", default=False, help="Make a local commit for each applied patch")
     build = make_option("--no-build", action="store_false", dest="build", default=True, help="Commit without building first, implies --no-test.")
     test = make_option("--no-test", action="store_false", dest="test", default=True, help="Commit without running run-webkit-tests.")
@@ -142 +143 @@
 
 
+class CleanWorkingDirectoryWithLocalCommitsStep(CleanWorkingDirectoryStep):
+    def __init__(self, tool, options):
+        # FIXME: This a bit of a hack.  Consider doing this more cleanly.
+        CleanWorkingDirectoryStep.__init__(self, tool, options, allow_local_commits=True)
+
+
 class UpdateStep(AbstractStep):
     @classmethod
@@ -172 +179 @@
     def run(self, state):
         self._tool.scm().apply_reverse_diff(state["revision"])
+
+
+class ApplyPatchWithLocalCommitStep(ApplyPatchStep):
+    @classmethod
+    def options(cls):
+        return [
+            CommandOptions.local_commit,
+        ] + ApplyPatchStep.options()
+    
+    def run(self, state):
+        ApplyPatchStep.run(self, state)
+        if self._options.local_commit:
+            commit_message = self._tool.scm().commit_message_for_this_commit()
+            self._tool.scm().commit_locally_with_message(commit_message.message() or state["patch"]["name"])
 
 
@@ -355 +376 @@
         # FIXME: I'm not sure state["commit_text"] is quite right here.
         self._tool.bugs.reopen_bug(bug_id, state["commit_text"])
-
-
-# FIXME: This class is a dinosaur and should be extinct soon.
-class BuildSteps:
-    # FIXME: The options should really live on each "Step" object.
-    @staticmethod
-    def cleaning_options():
-        return [
-            CommandOptions.force_clean,
-            CommandOptions.clean,
-        ]
-
-    # FIXME: These distinctions are bogus.  We need a better model for handling options.
-    @staticmethod
-    def build_options():
-        return [
-            CommandOptions.check_builders,
-            CommandOptions.quiet,
-            CommandOptions.non_interactive,
-            CommandOptions.parent_command,
-            CommandOptions.port,
-        ]
-
-    @staticmethod
-    def land_options():
-        return [
-            CommandOptions.update,
-            CommandOptions.build,
-            CommandOptions.test,
-            CommandOptions.close_bug,
-        ]
-

trunk/WebKitTools/Scripts/modules/commands/download.py

@@ -35 +35 @@
 from modules.bugzilla import parse_bug_id
 # FIXME: This list is rediculous.  We need to learn the ways of __all__.
-from modules.buildsteps import CommandOptions, BuildSteps, EnsureBuildersAreGreenStep, UpdateChangelogsWithReviewerStep, CleanWorkingDirectoryStep, UpdateStep, ApplyPatchStep, BuildStep, CheckStyleStep, RunTestsStep, CommitStep, ClosePatchStep, CloseBugStep, CloseBugForLandDiffStep, PrepareChangelogStep, PrepareChangelogForRevertStep, RevertRevisionStep, CompleteRollout
+from modules.buildsteps import CommandOptions, EnsureBuildersAreGreenStep, UpdateChangelogsWithReviewerStep, CleanWorkingDirectoryStep, CleanWorkingDirectoryWithLocalCommitsStep, UpdateStep, ApplyPatchStep, ApplyPatchWithLocalCommitStep, BuildStep, CheckStyleStep, RunTestsStep, CommitStep, ClosePatchStep, CloseBugStep, CloseBugForLandDiffStep, PrepareChangelogStep, PrepareChangelogForRevertStep, RevertRevisionStep, CompleteRollout
 from modules.changelogs import ChangeLog
 from modules.comments import bug_comment_from_commit_text
@@ -60 +60 @@
 
 
-class ApplyAttachment(Command):
-    name = "apply-attachment"
-    show_in_main_help = True
-    def __init__(self):
-        options = WebKitApplyingScripts.apply_options()
-        options += BuildSteps.cleaning_options()
-        Command.__init__(self, "Apply an attachment to the local working directory", "ATTACHMENT_ID", options=options)
-
-    def execute(self, options, args, tool):
-        WebKitApplyingScripts.setup_for_patch_apply(tool, options)
-        attachment_id = args[0]
-        attachment = tool.bugs.fetch_attachment(attachment_id)
-        WebKitApplyingScripts.apply_patches_with_options(tool.scm(), [attachment], options)
-
-
-class ApplyPatches(Command):
-    name = "apply-patches"
-    show_in_main_help = True
-    def __init__(self):
-        options = WebKitApplyingScripts.apply_options()
-        options += BuildSteps.cleaning_options()
-        Command.__init__(self, "Apply reviewed patches from provided bugs to the local working directory", "BUGID", options=options)
-
-    def execute(self, options, args, tool):
-        WebKitApplyingScripts.setup_for_patch_apply(tool, options)
-        bug_id = args[0]
-        patches = tool.bugs.fetch_reviewed_patches_from_bug(bug_id)
-        WebKitApplyingScripts.apply_patches_with_options(tool.scm(), patches, options)
-
-
-class WebKitApplyingScripts:
-    @staticmethod
-    def apply_options():
-        return [
-            make_option("--no-update", action="store_false", dest="update", default=True, help="Don't update the working directory before applying patches"),
-            make_option("--local-commit", action="store_true", dest="local_commit", default=False, help="Make a local commit for each applied patch"),
-            CommandOptions.port,
-        ]
-
-    @staticmethod
-    def setup_for_patch_apply(tool, options):
-        clean_step = CleanWorkingDirectoryStep(tool, options, allow_local_commits=True)
-        clean_step.run({})
-        update_step = UpdateStep(tool, options)
-        update_step.run({})
-
-    @staticmethod
-    def apply_patches_with_options(scm, patches, options):
-        if options.local_commit and not scm.supports_local_commits():
-            error("--local-commit passed, but %s does not support local commits" % scm.display_name())
-
-        for patch in patches:
-            log("Applying attachment %s from bug %s" % (patch["id"], patch["bug_id"]))
-            scm.apply_patch(patch)
-            if options.local_commit:
-                commit_message = scm.commit_message_for_this_commit()
-                scm.commit_locally_with_message(commit_message.message() or patch["name"])
-
-
 class LandDiff(Command):
     name = "land-diff"
@@ -220 +161 @@
         state = {"patch": patch}
         self._sequence.run_and_handle_errors(tool, options, state)
+
+
+class AbstractPatchApplyingCommand(AbstractPatchProcessingCommand):
+    def __init__(self, help_text, args_description):
+        self._prepare_sequence = StepSequence([
+            CleanWorkingDirectoryWithLocalCommitsStep,
+            UpdateStep,
+        ])
+        self._main_sequence  = StepSequence([
+            ApplyPatchWithLocalCommitStep,
+        ])
+        options = sorted(set(self._prepare_sequence.options() + self._main_sequence.options()))
+        AbstractPatchProcessingCommand.__init__(self, help_text, args_description, options)
+
+    def _prepare_to_process(self, options, args, tool):
+        if options.local_commit and not tool.scm().supports_local_commits():
+            error("--local-commit passed, but %s does not support local commits" % scm.display_name())
+        self._prepare_sequence.run_and_handle_errors(tool, options)
+
+    # FIXME: Add a base class to share this code.
+    def _process_patch(self, patch, options, args, tool):
+        state = {"patch": patch}
+        self._main_sequence.run_and_handle_errors(tool, options, state)
+
+
+class ApplyAttachment(AbstractPatchApplyingCommand):
+    name = "apply-attachment"
+    show_in_main_help = True
+    def __init__(self):
+        AbstractPatchApplyingCommand.__init__(self, "Apply an attachment to the local working directory", "ATTACHMENT_ID [ATTACHMENT_IDS]")
+
+    def _fetch_list_of_patches_to_process(self, options, args, tool):
+        return map(lambda patch_id: tool.bugs.fetch_attachment(patch_id), args)
+
+
+class ApplyPatches(AbstractPatchApplyingCommand):
+    name = "apply-patches"
+    show_in_main_help = True
+    def __init__(self):
+        AbstractPatchApplyingCommand.__init__(self, "Apply reviewed patches from provided bugs to the local working directory", "BUGID [BUGIDS]")
+
+    def _fetch_list_of_patches_to_process(self, options, args, tool):
+        all_patches = []
+        for bug_id in args:
+            patches = tool.bugs.fetch_reviewed_patches_from_bug(bug_id)
+            log("%s found on bug %s." % (pluralize("reviewed patch", len(patches)), bug_id))
+            all_patches += patches
+        return all_patches
 
 

trunk/WebKitTools/Scripts/modules/commands/download_unittest.py

@@ -56 +56 @@
         options.update = True
         options.local_commit = True
-        expected_stderr = "Updating working directory\nApplying attachment 197 from bug 42\n"
+        expected_stderr = "Updating working directory\nProcessing 1 patch from 1 bug.\nProcessing patch 197 from bug 42.\n"
         self.assert_execute_outputs(ApplyAttachment(), [197], options=options, expected_stderr=expected_stderr)
 
@@ -63 +63 @@
         options.update = True
         options.local_commit = True
-        expected_stderr = "Updating working directory\nApplying attachment 197 from bug 42\nApplying attachment 128 from bug 42\n"
+        expected_stderr = "Updating working directory\n2 reviewed patches found on bug 42.\nProcessing 2 patches from 1 bug.\nProcessing patch 197 from bug 42.\nProcessing patch 128 from bug 42.\n"
         self.assert_execute_outputs(ApplyPatches(), [42], options=options, expected_stderr=expected_stderr)