Changeset 52129 in webkit


Ignore:
Timestamp:
Dec 14, 2009 9:21:33 PM (14 years ago)
Author:
abarth@webkit.org
Message:

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

Reviewed by Eric Seidel.

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

Removes LandingSequence in favor of StepSequence. This required
changing the Step API slightly to carry a general notion of state
instead of carrying patches specifically.

  • Scripts/modules/buildsteps.py:
  • Scripts/modules/commands/download.py:
  • Scripts/modules/commands/queues.py:
  • Scripts/modules/landingsequence.py: Removed.
  • Scripts/modules/stepsequence.py:
Location:
trunk/WebKitTools
Files:
1 deleted
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebKitTools/ChangeLog

    r52118 r52129  
     12009-12-14  Adam Barth  <abarth@webkit.org>
     2
     3        Reviewed by Eric Seidel.
     4
     5        [bzt] Kill LandingSequence
     6        https://bugs.webkit.org/show_bug.cgi?id=32464
     7
     8        Removes LandingSequence in favor of StepSequence.  This required
     9        changing the Step API slightly to carry a general notion of state
     10        instead of carrying patches specifically.
     11
     12        * Scripts/modules/buildsteps.py:
     13        * Scripts/modules/commands/download.py:
     14        * Scripts/modules/commands/queues.py:
     15        * Scripts/modules/landingsequence.py: Removed.
     16        * Scripts/modules/stepsequence.py:
     17
    1182009-12-14  Robert Hogan  <robert@roberthogan.net>
    219

  • trunk/WebKitTools/Scripts/modules/buildsteps.py

    r51940 r52129  
    4848    close_bug = make_option("--no-close", action="store_false", dest="close_bug", default=True, help="Leave bug open after landing.")
    4949    port = make_option("--port", action="store", dest="port", default=None, help="Specify a port (e.g., mac, qt, gtk, ...).")
     50    reviewer = make_option("-r", "--reviewer", action="store", type="string", dest="reviewer", help="Update ChangeLogs to say Reviewed by REVIEWER.")
    5051
    5152
    5253class AbstractStep(object):
    53     def __init__(self, tool, options, patch=None):
     54    def __init__(self, tool, options):
    5455        self._tool = tool
    5556        self._options = options
    56         self._patch = patch
    5757        self._port = None
    5858
    … …  
    7272        return []
    7373
    74     def run(self, tool):
     74    def run(self, state):
    7575        raise NotImplementedError, "subclasses must implement"
    7676
    7777
    7878class PrepareChangelogStep(AbstractStep):
    79     def run(self):
     79    def run(self, state):
    8080        self._run_script("prepare-ChangeLog")
    8181
    8282
    8383class CleanWorkingDirectoryStep(AbstractStep):
    84     def __init__(self, tool, options, patch=None, allow_local_commits=False):
    85         AbstractStep.__init__(self, tool, options, patch)
     84    def __init__(self, tool, options, allow_local_commits=False):
     85        AbstractStep.__init__(self, tool, options)
    8686        self._allow_local_commits = allow_local_commits
    8787
    … …  
    9393        ]
    9494
    95     def run(self):
     95    def run(self, state):
    9696        os.chdir(self._tool.scm().checkout_root)
    9797        if not self._allow_local_commits:
    … …  
    109109        ]
    110110
    111     def run(self):
     111    def run(self, state):
    112112        if not self._options.update:
    113113            return
    … …  
    123123        ]
    124124
    125     def run(self):
    126         log("Processing patch %s from bug %s." % (self._patch["id"], self._patch["bug_id"]))
    127         self._tool.scm().apply_patch(self._patch, force=self._options.non_interactive)
     125    def run(self, state):
     126        log("Processing patch %s from bug %s." % (state["patch"]["id"], state["patch"]["bug_id"]))
     127        self._tool.scm().apply_patch(state["patch"], force=self._options.non_interactive)
    128128
    129129
    … …  
    135135        ]
    136136
    137     def run(self):
     137    def run(self, state):
    138138        if not self._options.check_builders:
    139139            return
    … …  
    145145
    146146
     147class UpdateChangelogsWithReviewerStep(AbstractStep):
     148    @classmethod
     149    def options(cls):
     150        return [
     151            CommandOptions.reviewer,
     152        ]
     153
     154    def guess_reviewer_from_bug(self, bug_id):
     155        patches = self._tool.bugs.fetch_reviewed_patches_from_bug(bug_id)
     156        if len(patches) != 1:
     157            log("%s on bug %s, cannot infer reviewer." % (pluralize("reviewed patch", len(patches)), bug_id))
     158            return None
     159        patch = patches[0]
     160        reviewer = patch["reviewer"]
     161        log("Guessing \"%s\" as reviewer from attachment %s on bug %s." % (reviewer, patch["id"], bug_id))
     162        return reviewer
     163
     164    def run(self, state):
     165        bug_id = state["patch"]["bug_id"]
     166        reviewer = self._options.reviewer
     167        if not reviewer:
     168            if not bug_id:
     169                log("No bug id provided and --reviewer= not provided.  Not updating ChangeLogs with reviewer.")
     170                return
     171            reviewer = self.guess_reviewer_from_bug(bug_id)
     172
     173        if not reviewer:
     174            log("Failed to guess reviewer from bug %s and --reviewer= not provided.  Not updating ChangeLogs with reviewer." % bug_id)
     175            return
     176
     177        os.chdir(self._tool.scm().checkout_root)
     178        for changelog_path in self._tool.scm().modified_changelogs():
     179            ChangeLog(changelog_path).set_reviewer(reviewer)
     180
     181
    147182class BuildStep(AbstractStep):
    148183    @classmethod
    … …  
    153188        ]
    154189
    155     def run(self):
     190    def run(self, state):
    156191        if not self._options.build:
    157192            return
    … …  
    161196
    162197class CheckStyleStep(AbstractStep):
    163     def run(self):
     198    def run(self, state):
    164199        self._run_script("check-webkit-style")
    165200
    … …  
    176211        ]
    177212
    178     def run(self):
     213    def run(self, state):
    179214        if not self._options.build:
    180215            return
    … …  
    191226
    192227class CommitStep(AbstractStep):
    193     def run(self):
     228    def run(self, state):
    194229        commit_message = self._tool.scm().commit_message_for_this_commit()
    195         return self._tool.scm().commit_with_message(commit_message.message())
     230        state["commit_text"] = self._tool.scm().commit_with_message(commit_message.message())
    196231
    197232
    198233class ClosePatchStep(AbstractStep):
    199     def run(self, commit_log):
    200         comment_text = bug_comment_from_commit_text(self._tool.scm(), commit_log)
    201         self._tool.bugs.clear_attachment_flags(self._patch["id"], comment_text)
     234    def run(self, state):
     235        comment_text = bug_comment_from_commit_text(self._tool.scm(), state["commit_text"])
     236        self._tool.bugs.clear_attachment_flags(state["patch"]["id"], comment_text)
    202237
    203238
    … …  
    209244        ]
    210245
    211     def run(self):
     246    def run(self, state):
    212247        if not self._options.close_bug:
    213248            return
    214249        # Check to make sure there are no r? or r+ patches on the bug before closing.
    215250        # Assume that r- patches are just previous patches someone forgot to obsolete.
    216         patches = self._tool.bugs.fetch_patches_from_bug(self._patch["bug_id"])
     251        patches = self._tool.bugs.fetch_patches_from_bug(state["patch"]["bug_id"])
    217252        for patch in patches:
    218253            review_flag = patch.get("review")
    … …  
    220255                log("Not closing bug %s as attachment %s has review=%s.  Assuming there are more patches to land from this bug." % (patch["bug_id"], patch["id"], review_flag))
    221256                return
    222         self._tool.bugs.close_bug_as_fixed(self._patch["bug_id"], "All reviewed patches have been landed.  Closing bug.")
     257        self._tool.bugs.close_bug_as_fixed(state["patch"]["bug_id"], "All reviewed patches have been landed.  Closing bug.")
     258
     259
     260class CloseBugForLandDiffStep(AbstractStep):
     261    @classmethod
     262    def options(cls):
     263        return [
     264            CommandOptions.close_bug,
     265        ]
     266
     267    def run(self, state):
     268        comment_text = bug_comment_from_commit_text(self._tool.scm(), state["commit_text"])
     269        bug_id = state["patch"]["bug_id"]
     270        if bug_id:
     271            log("Updating bug %s" % bug_id)
     272            if self._options.close_bug:
     273                self._tool.bugs.close_bug_as_fixed(bug_id, comment_text)
     274            else:
     275                # FIXME: We should a smart way to figure out if the patch is attached
     276                # to the bug, and if so obsolete it.
     277                self._tool.bugs.post_comment_to_bug(bug_id, comment_text)
     278        else:
     279            log(comment_text)
     280            log("No bug id provided.")
    223281
    224282

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

    r51942 r52129  
    3434
    3535from modules.bugzilla import parse_bug_id
    36 from modules.buildsteps import CommandOptions, BuildSteps, EnsureBuildersAreGreenStep, CleanWorkingDirectoryStep, UpdateStep, ApplyPatchStep, BuildStep, CheckStyleStep, PrepareChangelogStep
     36# FIXME: This list is rediculous.  We need to learn the ways of __all__.
     37from modules.buildsteps import CommandOptions, BuildSteps, EnsureBuildersAreGreenStep, UpdateChangelogsWithReviewerStep, CleanWorkingDirectoryStep, UpdateStep, ApplyPatchStep, BuildStep, CheckStyleStep, RunTestsStep, CommitStep, ClosePatchStep, CloseBugStep, CloseBugForLandDiffStep, PrepareChangelogStep
    3738from modules.changelogs import ChangeLog
    3839from modules.comments import bug_comment_from_commit_text
    3940from modules.executive import ScriptError
    4041from modules.grammar import pluralize
    41 from modules.landingsequence import LandingSequence
    4242from modules.logging import error, log
    4343from modules.multicommandtool import Command
    … …  
    102102    def setup_for_patch_apply(tool, options):
    103103        clean_step = CleanWorkingDirectoryStep(tool, options, allow_local_commits=True)
    104         clean_step.run()
     104        clean_step.run({})
    105105        update_step = UpdateStep(tool, options)
    106         update_step.run()
     106        update_step.run({})
    107107
    108108    @staticmethod
    … …  
    119119
    120120
    121 class LandDiffSequence(LandingSequence):
    122     def run(self):
    123         self.check_builders()
    124         self.build()
    125         self.test()
    126         commit_log = self.commit()
    127         self.close_bug(commit_log)
    128 
    129     def close_bug(self, commit_log):
    130         comment_test = bug_comment_from_commit_text(self._tool.scm(), commit_log)
    131         bug_id = self._patch["bug_id"]
    132         if bug_id:
    133             log("Updating bug %s" % bug_id)
    134             if self._options.close_bug:
    135                 self._tool.bugs.close_bug_as_fixed(bug_id, comment_test)
    136             else:
    137                 # FIXME: We should a smart way to figure out if the patch is attached
    138                 # to the bug, and if so obsolete it.
    139                 self._tool.bugs.post_comment_to_bug(bug_id, comment_test)
    140         else:
    141             log(comment_test)
    142             log("No bug id provided.")
    143 
    144 
    145121class LandDiff(Command):
    146122    name = "land-diff"
    147123    show_in_main_help = True
    148124    def __init__(self):
    149         options = [
    150             make_option("-r", "--reviewer", action="store", type="string", dest="reviewer", help="Update ChangeLogs to say Reviewed by REVIEWER."),
    151         ]
    152         options += BuildSteps.build_options()
    153         options += BuildSteps.land_options()
    154         Command.__init__(self, "Land the current working directory diff and updates the associated bug if any", "[BUGID]", options=options)
    155 
    156     def guess_reviewer_from_bug(self, bugs, bug_id):
    157         patches = bugs.fetch_reviewed_patches_from_bug(bug_id)
    158         if len(patches) != 1:
    159             log("%s on bug %s, cannot infer reviewer." % (pluralize("reviewed patch", len(patches)), bug_id))
    160             return None
    161         patch = patches[0]
    162         reviewer = patch["reviewer"]
    163         log("Guessing \"%s\" as reviewer from attachment %s on bug %s." % (reviewer, patch["id"], bug_id))
    164         return reviewer
    165 
    166     def update_changelogs_with_reviewer(self, reviewer, bug_id, tool):
    167         if not reviewer:
    168             if not bug_id:
    169                 log("No bug id provided and --reviewer= not provided.  Not updating ChangeLogs with reviewer.")
    170                 return
    171             reviewer = self.guess_reviewer_from_bug(tool.bugs, bug_id)
    172 
    173         if not reviewer:
    174             log("Failed to guess reviewer from bug %s and --reviewer= not provided.  Not updating ChangeLogs with reviewer." % bug_id)
    175             return
    176 
    177         for changelog_path in tool.scm().modified_changelogs():
    178             ChangeLog(changelog_path).set_reviewer(reviewer)
     125        self._sequence = StepSequence([
     126            EnsureBuildersAreGreenStep,
     127            UpdateChangelogsWithReviewerStep,
     128            EnsureBuildersAreGreenStep,
     129            BuildStep,
     130            RunTestsStep,
     131            CommitStep,
     132            CloseBugForLandDiffStep,
     133        ])
     134        Command.__init__(self, "Land the current working directory diff and updates the associated bug if any", "[BUGID]", options=self._sequence.options())
    179135
    180136    def execute(self, options, args, tool):
    181137        bug_id = (args and args[0]) or parse_bug_id(tool.scm().create_patch())
    182 
    183         EnsureBuildersAreGreenStep(tool, options).run()
    184 
    185         os.chdir(tool.scm().checkout_root)
    186         self.update_changelogs_with_reviewer(options.reviewer, bug_id, tool)
    187 
    188138        fake_patch = {
    189139            "id": None,
    190             "bug_id": bug_id
     140            "bug_id": bug_id,
    191141        }
    192 
    193         sequence = LandDiffSequence(fake_patch, options, tool)
    194         sequence.run()
     142        state = {"patch": fake_patch}
     143        self._sequence.run_and_handle_errors(tool, options, state)
    195144
    196145
    … …  
    243192        pass
    244193
     194    # FIXME: Add a base class to share this code.
    245195    def _process_patch(self, patch, options, args, tool):
    246         self._sequence.run_and_handle_errors(tool, options, patch)
     196        state = {"patch": patch}
     197        self._sequence.run_and_handle_errors(tool, options, state)
    247198
    248199
    … …  
    265216        pass
    266217
     218    # FIXME: Add a base class to share this code.
    267219    def _process_patch(self, patch, options, args, tool):
    268         self._sequence.run_and_handle_errors(tool, options, patch)
     220        state = {"patch": patch}
     221        self._sequence.run_and_handle_errors(tool, options, state)
    269222
    270223
    271224class AbstractPatchLandingCommand(AbstractPatchProcessingCommand):
    272225    def __init__(self, help_text, args_description):
    273         options = BuildSteps.cleaning_options()
    274         options += BuildSteps.build_options()
    275         options += BuildSteps.land_options()
    276         AbstractPatchProcessingCommand.__init__(self, help_text, args_description, options)
     226        self._sequence = StepSequence([
     227            CleanWorkingDirectoryStep,
     228            UpdateStep,
     229            ApplyPatchStep,
     230            EnsureBuildersAreGreenStep,
     231            BuildStep,
     232            RunTestsStep,
     233            CommitStep,
     234            ClosePatchStep,
     235            CloseBugStep,
     236        ])
     237        AbstractPatchProcessingCommand.__init__(self, help_text, args_description, self._sequence.options())
    277238
    278239    def _prepare_to_process(self, options, args, tool):
    279240        # Check the tree status first so we can fail early.
    280         EnsureBuildersAreGreenStep(tool, options).run()
    281 
     241        EnsureBuildersAreGreenStep(tool, options).run({})
     242
     243    # FIXME: Add a base class to share this code.
    282244    def _process_patch(self, patch, options, args, tool):
    283         sequence = LandingSequence(patch, options, tool)
    284         sequence.run_and_handle_errors()
     245        state = {"patch": patch}
     246        self._sequence.run_and_handle_errors(tool, options, state)
    285247
    286248
    … …  
    329291        # Second, make new ChangeLog entries for this rollout.
    330292        # This could move to prepare-ChangeLog by adding a --revert= option.
    331         PrepareChangelogStep(tool, None).run()
     293        PrepareChangelogStep(tool, None).run({})
    332294        for changelog_path in changelog_paths:
    333295            ChangeLog(changelog_path).update_for_revert(revision)
    … …  
    355317                log("Failed to parse bug number from diff.  No bugs will be updated/reopened after the rollout.")
    356318
    357         CleanWorkingDirectoryStep(tool, options).run()
    358         UpdateStep(tool, options).run()
     319        CleanWorkingDirectoryStep(tool, options).run({})
     320        UpdateStep(tool, options).run({})
    359321        tool.scm().apply_reverse_diff(revision)
    360322        self._create_changelogs_for_revert(tool, revision)

  • trunk/WebKitTools/Scripts/modules/commands/queues.py

    r51895 r52129  
    3636from modules.executive import ScriptError
    3737from modules.grammar import pluralize
    38 from modules.landingsequence import LandingSequence, LandingSequenceErrorHandler
    3938from modules.logging import error, log
    4039from modules.multicommandtool import Command
    4140from modules.patchcollection import PersistentPatchCollection, PersistentPatchCollectionDelegate
    4241from modules.statusbot import StatusBot
     42from modules.stepsequence import StepSequenceErrorHandler
    4343from modules.workqueue import WorkQueue, WorkQueueDelegate
    4444
    … …  
    105105
    106106
    107 class CommitQueue(AbstractQueue, LandingSequenceErrorHandler):
     107class CommitQueue(AbstractQueue, StepSequenceErrorHandler):
    108108    name = "commit-queue"
    109109    def __init__(self):
    … …  
    137137        self.tool.bugs.reject_patch_from_commit_queue(patch["id"], message)
    138138
    139     # LandingSequenceErrorHandler methods
     139    # StepSequenceErrorHandler methods
    140140
    141141    @classmethod
    142     def handle_script_error(cls, tool, patch, script_error):
    143         tool.bugs.reject_patch_from_commit_queue(patch["id"], script_error.message_with_output())
    144 
    145 
    146 class AbstractReviewQueue(AbstractQueue, PersistentPatchCollectionDelegate, LandingSequenceErrorHandler):
     142    def handle_script_error(cls, tool, state, script_error):
     143        tool.bugs.reject_patch_from_commit_queue(state["patch"]["id"], script_error.message_with_output())
     144
     145
     146class AbstractReviewQueue(AbstractQueue, PersistentPatchCollectionDelegate, StepSequenceErrorHandler):
    147147    def __init__(self, options=None):
    148148        AbstractQueue.__init__(self, options)
    … …  
    180180        log(message)
    181181
    182     # LandingSequenceErrorHandler methods
     182    # StepSequenceErrorHandler methods
    183183
    184184    @classmethod
    185     def handle_script_error(cls, tool, patch, script_error):
     185    def handle_script_error(cls, tool, state, script_error):
    186186        log(script_error.message_with_output())
    187187
    … …  
    206206
    207207    @classmethod
    208     def handle_script_error(cls, tool, patch, script_error):
     208    def handle_script_error(cls, tool, state, script_error):
    209209        command = script_error.script_args
    210210        if type(command) is list:
    … …  
    213213        #        have a better API.
    214214        if re.search("check-webkit-style", command):
    215             message = "Attachment %s did not pass %s:\n\n%s" % (patch["id"], cls.name, script_error.message_with_output(output_limit=5*1024))
    216             tool.bugs.post_comment_to_bug(patch["bug_id"], message, cc=cls.watchers)
     215            message = "Attachment %s did not pass %s:\n\n%s" % (state["patch"]["id"], cls.name, script_error.message_with_output(output_limit=5*1024))
     216            tool.bugs.post_comment_to_bug(state["patch"]["bug_id"], message, cc=cls.watchers)

  • trunk/WebKitTools/Scripts/modules/stepsequence.py

    r51956 r52129  
    3434
    3535
     36class StepSequenceErrorHandler():
     37    @classmethod
     38    def handle_script_error(cls, tool, patch, script_error):
     39        raise NotImplementedError, "subclasses must implement"
     40
     41
    3642class StepSequence(object):
    3743    def __init__(self, steps):
    … …  
    4955        return collected_options
    5056
    51     def _run(self, tool, options, patch):
     57    def _run(self, tool, options, state):
    5258        for step in self._steps:
    53             step(tool, options, patch).run()
     59            step(tool, options).run(state)
    5460
    55     def run_and_handle_errors(self, tool, options, patch=None):
     61    def run_and_handle_errors(self, tool, options, state=None):
     62        if not state:
     63            state = {}
    5664        try:
    57             self._run(tool, options, patch)
     65            self._run(tool, options, state)
    5866        except CheckoutNeedsUpdate, e:
    5967            log("Commit failed because the checkout is out of date.  Please update and try again.")
    … …  
    6573            if options.parent_command:
    6674                command = tool.command_by_name(options.parent_command)
    67                 command.handle_script_error(tool, patch, e)
     75                command.handle_script_error(tool, state, e)
    6876            WorkQueue.exit_after_handled_error(e)
Note: See TracChangeset for help on using the changeset viewer.