Changeset 53593 in webkit


Ignore:
Timestamp:
Jan 20, 2010 7:09:44 PM (14 years ago)
Author:
eric@webkit.org
Message:

2010-01-20 Eric Seidel <​eric@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.
Location:
trunk/WebKitTools
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebKitTools/ChangeLog

    r53592 r53593  
     12010-01-20  Eric Seidel  <eric@webkit.org>
     2
     3        Reviewed by Adam Barth.
     4
     5        webkit-commit-queue status page is confusing
     6        https://bugs.webkit.org/show_bug.cgi?id=33496
     7
     8        This should improve the status page by removing more Fail messages.
     9        To do this, I re-factored the CommitQueue and the AbstractReviewQueues
     10        to behave more like one another.  This meant moving where the failure reporting was done.
     11        Previously the AbstractReviewQueue always used the parent process to report the error,
     12        while CommitQueue used the subprocess when possible, and the parent only reported errors
     13        that we didn't know how to handle (bugs in the commit-queue itself).
     14        Now the AbstractReviewQueue follow's the commit-queue's model.  This got rid of a try-block
     15        in both implementations and required teaching handle_script_error in each to post Fail messages
     16        to the status server instead of calling exit(1).
     17
     18        This will also make the style-queue share more bug posting logic with other queues:
     19        https://bugs.webkit.org/show_bug.cgi?id=33871
     20
     21        * Scripts/webkitpy/commands/early_warning_system.py:
     22         - Don't exit(1) as that will cause the calling queue to also report Fail to the status server.
     23           Implementors of handle_script_error are expected to update the status server if needed, but only exit if the error could not be handled.
     24           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.
     25           _update_status_for_script_error knows how to post the Fail message to the status server.
     26         - Teach _update_status_for_script_error how to post Fail messages to the status server.
     27        * Scripts/webkitpy/commands/queues.py:
     28         - Remove the try block from process_work_item since the caller already has one.
     29         - Only CC watchers on failure to cut down on commit-queue generated mail.
     30         - handle_unexpected_error needs to mark _did_fail now that the try block is gone from process_work_item.
     31         - Abstract _format_script_error_output_for_bug to share code between all queues.
     32         - 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.
     33         - Rename _can_build_and_test to _current_checkout_builds_and_passes_tests to better explain what revision it's testing.
     34         - Move logging out of _can_build_and_test and make the logs explain what revision we're testing.
     35         - handle_script_error now posts Fail instead of the try block in process_work_item handling it.
     36        * Scripts/webkitpy/queueengine.py:
     37         - QueueEngine is no longer used just by the commit-queue, update the logging to say "processing" instead of landing.
     38        * Scripts/webkitpy/scm.py:
     39         - Add new checkout_revision function.
     40        * Scripts/webkitpy/scm_unittest.py:
     41         - Test our new checkout_revision function.
     42
    1432010-01-20  Fumitoshi Ukai  <ukai@chromium.org>
    244

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

    r53570 r53593  
    7171
    7272    @classmethod
    73     def handle_script_error(cls, tool, state, script_error):
    74         is_svn_apply = script_error.command_name() == "svn-apply"
    75         status_id = cls._update_status_for_script_error(tool, state, script_error, is_error=is_svn_apply)
    76         if is_svn_apply:
    77             QueueEngine.exit_after_handled_error(script_error)
     73    def _report_patch_failure(cls, tool, patch, script_error):
    7874        results_link = tool.status_server.results_url_for_status(status_id)
    79         message = "Attachment %s did not build on %s:\nBuild output: %s" % (state["patch"].id(), cls.port_name, results_link)
    80         tool.bugs.post_comment_to_bug(state["patch"].bug_id(), message, cc=cls.watchers)
    81         exit(1)
     75        message = "Attachment %s did not build on %s:\nBuild output: %s" % (patch.id(), cls.port_name, results_link)
     76        tool.bugs.post_comment_to_bug(patch.bug_id(), message, cc=cls.watchers)
    8277
    8378

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

    r53570 r53593  
    107107        raise NotImplementedError, "subclasses must implement"
    108108
     109    # Subclasses are expected to post the message somewhere it can be read by a human.
     110    # They should also prevent the queue from processing this patch again.
    109111    def handle_unexpected_error(self, work_item, message):
    110112        raise NotImplementedError, "subclasses must implement"
    … …  
    126128
    127129    @classmethod
    128     def _update_status_for_script_error(cls, tool, state, script_error, is_error=False):
     130    def _update_status_for_script_error(cls, tool, state, script_error, patch_has_failed_this_queue=True):
    129131        message = script_error.message
    130         if is_error:
     132        # Error: turns the status bubble purple and means that the error was such that we can't tell if the patch fails or not.
     133        if not patch_has_failed_this_queue:
    131134            message = "Error: %s" % message
    132         return tool.status_server.update_status(cls.name, message, state["patch"], StringIO(script_error.output))
     135        status_id = tool.status_server.update_status(cls.name, message, state["patch"], StringIO(script_error.output))
     136        # In the case we know that this patch actually failed the queue, we make a second status entry (mostly because the server doesn't
     137        # understand messages prefixed with "Fail:" to mean failures, and thus would leave the status bubble yellow instead of red).
     138        # FIXME: Fail vs. Error should really be stored on a separate field from the message on the server.
     139        if patch_has_failed_this_queue:
     140            tool.status_server.update_status(cls.name, cls._fail_status)
     141        return status_id # Return the status id to the original message with all the script output.
     142
     143    @classmethod
     144    def _format_script_error_output_for_bug(tool, status_id, script_error):
     145        if not script_error.output:
     146            return script_error.message_with_output()
     147        results_link = tool.status_server.results_url_for_status(status_id)
     148        return "%s\nFull output: %s" % (script_error.message_with_output(), results_link)
    133149
    134150
    … …  
    160176        return patches[0]
    161177
    162     def _can_build_and_test(self):
     178    def _current_checkout_builds_and_passes_tests(self):
    163179        try:
    164180            self.run_webkit_patch(["build-and-test", "--force-clean", "--non-interactive", "--build-style=both", "--quiet"])
    165181        except ScriptError, e:
    166             self._update_status("Unabled to successfully build and test", None)
    167182            return False
    168183        return True
    … …  
    179194        if not self._builders_are_green():
    180195            return False
    181         if not self._can_build_and_test():
     196        current_revision = self.tool.scm().checkout_revision()
     197        self._update_status("Building and testing r%s before landing patch" % current_revision, patch)
     198        if not self._current_checkout_builds_and_passes_tests():
     199            self._update_status("Failed to build and test r%s, will try landing later" % current_revision, patch)
    182200            return False
    183201        if not self._builders_are_green():
    … …  
    186204        return True
    187205
     206    # FIXME: This should be unified with AbstractReviewQueue's implementation.  (Mostly _review_patch just needs a rename.)
    188207    def process_work_item(self, patch):
    189         try:
    190             self._cc_watchers(patch.bug_id())
    191             # We pass --no-update here because we've already validated
    192             # that the current revision actually builds and passes the tests.
    193             # If we update, we risk moving to a revision that doesn't!
    194             self.run_webkit_patch(["land-attachment", "--force-clean", "--non-interactive", "--no-update", "--parent-command=commit-queue", "--build-style=both", "--quiet", patch.id()])
    195             self._did_pass(patch)
    196         except ScriptError, e:
    197             self._did_fail(patch)
    198             raise e
     208        # We pass --no-update here because we've already validated
     209        # that the current revision actually builds and passes the tests.
     210        # If we update, we risk moving to a revision that doesn't!
     211        self.run_webkit_patch(["land-attachment", "--force-clean", "--non-interactive", "--no-update", "--parent-command=commit-queue", "--build-style=both", "--quiet", patch.id()])
     212        self._did_pass(patch)
    199213
    200214    def handle_unexpected_error(self, patch, message):
    201         self.committer_validator.reject_patch_from_commit_queue(patch.id(), message)
     215        self._did_fail(patch)
     216        self._cc_watchers(patch.bug_id())
     217        self.committer_validator.reject_patch_from_commit_queue(patch.id(), message) # Should instead pass cls.watchers as a CC list here.
    202218
    203219    # StepSequenceErrorHandler methods
    204220
    205     @staticmethod
    206     def _error_message_for_bug(tool, status_id, script_error):
    207         if not script_error.output:
    208             return script_error.message_with_output()
    209         results_link = tool.status_server.results_url_for_status(status_id)
    210         return "%s\nFull output: %s" % (script_error.message_with_output(), results_link)
    211 
    212221    @classmethod
    213222    def handle_script_error(cls, tool, state, script_error):
    214         status_id = cls._update_status_for_script_error(tool, state, script_error)
    215         validator = CommitterValidator(tool.bugs)
    216         validator.reject_patch_from_commit_queue(state["patch"].id(), cls._error_message_for_bug(tool, status_id, script_error))
    217 
     223        status_id = cls._update_status_for_script_error(tool, state, script_error, patch_has_failed_this_queue=True)
     224        script_error_output = cls._format_script_error_output_for_bug(tool, status_id, script_error)
     225        self._cc_watchers(patch.bug_id())
     226        CommitterValidator(tool.bugs).reject_patch_from_commit_queue(state["patch"].id(), script_error_output) # Should instead pass cls.watchers as a CC list here.
    218227
    219228class AbstractReviewQueue(AbstractQueue, PersistentPatchCollectionDelegate, StepSequenceErrorHandler):
    … …  
    251260
    252261    def process_work_item(self, patch):
    253         try:
    254             self._review_patch(patch)
    255             self._did_pass(patch)
    256         except ScriptError, e:
    257             if e.exit_code != QueueEngine.handled_error_code:
    258                 self._did_fail(patch)
    259             raise e
     262        self._review_patch(patch)
     263        self._did_pass(patch)
    260264
    261265    def handle_unexpected_error(self, patch, message):
    262         log(message)
     266        log(message) # Unit tests expect us to log, we could eventually remove this since post_comment_to_bug will log as well.
     267        self._did_fail(patch)
     268        self.tool.bugs.post_comment_to_bug(patch.bug_id(), message, cc=self.watchers)
     269
     270    @classmethod
     271    def _report_patch_failure(cls, tool, patch, script_error):
     272        raise NotImplementedError, "subclasses must implement"
     273
     274    @classmethod
     275    def handle_script_error(cls, tool, state, script_error):
     276        patch_has_failed = script_error.command_name() != "svn-apply" # svn-apply failures do not necessarily mean the patch would fail to build.
     277        status_id = cls._update_status_for_script_error(tool, state, script_error, patch_has_failed_this_queue=patch_has_failed)
     278        if patch_has_failed:
     279            return # No need to update the bug for svn-apply failures.
     280        cls._report_patch_failure(tool, patch, script_error)
    263281
    264282    # StepSequenceErrorHandler methods
    … …  
    282300
    283301    @classmethod
    284     def handle_script_error(cls, tool, state, script_error):
    285         is_svn_apply = script_error.command_name() == "svn-apply"
    286         status_id = cls._update_status_for_script_error(tool, state, script_error, is_error=is_svn_apply)
    287         if is_svn_apply:
    288             QueueEngine.exit_after_handled_error(script_error)
    289         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))
    290         tool.bugs.post_comment_to_bug(state["patch"].bug_id(), message, cc=cls.watchers)
    291         exit(1)
     302    def _style_error_message_for_bug(cls, patch_id, script_error_output):
     303        check_style_command = "check-webkit-style"
     304        message_header = "Attachment %s did not pass %s:" % (patch_id, cls.name)
     305        message_footer = "If any of these errors are false positives, please file a bug against %s." % check_style_command
     306        return "%s\n\n%s\n\n%s" % (message_header, script_error_output, message_footer)
     307
     308    @classmethod
     309    def _report_patch_failure(cls, tool, patch, script_error):
     310        script_error_output = self._format_script_error_output_for_bug(tool, status_id, script_error)
     311        bug_message = cls._style_error_message_for_bug(patch.id(), script_error_output)
     312        tool.bugs.post_comment_to_bug(patch.bug_id(), message, cc=cls.watchers)

  • trunk/WebKitTools/Scripts/webkitpy/queueengine.py

    r53570 r53593  
    108108                    if e.exit_code == self.handled_error_code:
    109109                        continue
    110                     message = "Unexpected failure when landing patch!  Please file a bug against webkit-patch.\n%s" % e.message_with_output()
     110                    tool_name = "webkit-patch" # QueueEngine has no way to get to tool.name() at current.
     111                    message_header = "Unexpected failure while processing patch!  Please file a bug against %s." % tool_name
     112                    message = "%s\n\n%s" % (message_header, e.message_with_output())
    111113                    self._delegate.handle_unexpected_error(work_item, message)
    112114            except KeyboardInterrupt, e:

  • trunk/WebKitTools/Scripts/webkitpy/scm.py

    r53570 r53593  
    234234        raise NotImplementedError, "subclasses must implement"
    235235
     236    def checkout_revision(self):
     237        raise NotImplementedError, "subclasses must implement"
     238
    236239    # Subclasses must indicate if they support local commits,
    237240    # but the SCM baseclass will only call local_commits methods when this is true.
    … …  
    363366        return self.svn_commit_log('BASE')
    364367
     368    def checkout_revision(self):
     369        return int(self.value_from_svn_info(self.checkout_root, 'Revision'))
     370
    365371# All git-specific logic should go here.
    366372class Git(SCM):
    … …  
    460466    def last_svn_commit_log(self):
    461467        return run_command(['git', 'svn', 'log', '--limit=1'])
     468
     469    def checkout_revision(self):
     470        return int(run_command(["git", "svn", "find-rev", "HEAD"]).rstrip())
    462471
    463472    # Git-specific methods:

  • trunk/WebKitTools/Scripts/webkitpy/scm_unittest.py

    r53570 r53593  
    205205        self.assertTrue(re.search('test2', self.scm.diff_for_revision(2)))
    206206
     207    def _shared_test_checkout_revision(self):
     208        self.assertEqual(self.scm.checkout_revision(), 4)
     209
    207210    def _shared_test_svn_apply_git_patch(self):
    208211        self._setup_webkittools_scripts_symlink(self.scm)
    … …  
    466469        self._shared_test_diff_for_revision()
    467470
     471    def test_checkout_revision(self):
     472        self._shared_test_checkout_revision()
     473
    468474    def test_svn_apply_git_patch(self):
    469475        self._shared_test_svn_apply_git_patch()
    … …  
    557563    def test_diff_for_revision(self):
    558564        self._shared_test_diff_for_revision()
     565
     566    def test_checkout_revision(self):
     567        self._shared_test_checkout_revision()
    559568
    560569    def test_svn_apply_git_patch(self):
Note: See TracChangeset for help on using the changeset viewer.