Commit af575f47 authored by abarth@webkit.org's avatar abarth@webkit.org
Browse files

2010-09-14 Adam Barth <abarth@webkit.org>

        Reviewed by Eric Seidel.

        commit-queue is slow during the day
        https://bugs.webkit.org/show_bug.cgi?id=45780

        Thanks to the new logging, we've noticed that checkout-is-out-of-date
        errors in the first pass of landing don't retry the land.  Instead,
        they're treated as failures and cause the commit-queue to do two more
        builds before really trying to land the patch.  Worse, in the second
        build, we can get bitten by a flaky test.

        This patch takes a slightly different approach to the commit-queue's
        main control logic.  We now use a separate subprocess for building and
        testing and for landing.  This means we should very rarely see the
        checkout-is-out-of-date error, and when we do see it, we should retry
        more quickly.  If my understanding is correct, this should be a big
        speed win for the commit-queue.

        * Scripts/webkitpy/tool/commands/download.py:
        * Scripts/webkitpy/tool/commands/queues.py:
        * Scripts/webkitpy/tool/commands/queues_unittest.py:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@67495 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 099bc16b
2010-09-14 Adam Barth <abarth@webkit.org>
Reviewed by Eric Seidel.
commit-queue is slow during the day
https://bugs.webkit.org/show_bug.cgi?id=45780
Thanks to the new logging, we've noticed that checkout-is-out-of-date
errors in the first pass of landing don't retry the land. Instead,
they're treated as failures and cause the commit-queue to do two more
builds before really trying to land the patch. Worse, in the second
build, we can get bitten by a flaky test.
This patch takes a slightly different approach to the commit-queue's
main control logic. We now use a separate subprocess for building and
testing and for landing. This means we should very rarely see the
checkout-is-out-of-date error, and when we do see it, we should retry
more quickly. If my understanding is correct, this should be a big
speed win for the commit-queue.
* Scripts/webkitpy/tool/commands/download.py:
* Scripts/webkitpy/tool/commands/queues.py:
* Scripts/webkitpy/tool/commands/queues_unittest.py:
2010-09-14 Tony Chang <tony@chromium.org>
Reviewed by Dimitri Glazkov.
......
......@@ -194,6 +194,19 @@ class BuildAttachment(AbstractPatchSequencingCommand, ProcessAttachmentsMixin):
]
class BuildAndTestAttachment(AbstractPatchSequencingCommand, ProcessAttachmentsMixin):
name = "build-and-test-attachment"
help_text = "Apply, build, and test patches from bugzilla"
argument_names = "ATTACHMENT_ID [ATTACHMENT_IDS]"
main_steps = [
steps.CleanWorkingDirectory,
steps.Update,
steps.ApplyPatch,
steps.Build,
steps.RunTests,
]
class PostAttachmentToRietveld(AbstractPatchSequencingCommand, ProcessAttachmentsMixin):
name = "post-attachment-to-rietveld"
help_text = "Uploads a bugzilla attachment to rietveld"
......
......@@ -199,7 +199,7 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
self.log_progress([patch.id() for patch in patches])
return patches[0]
def _can_build_and_test(self):
def _can_build_and_test_without_patch(self):
try:
self.run_webkit_patch([
"build-and-test",
......@@ -212,7 +212,7 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
"--quiet"])
except ScriptError, e:
failure_log = self._log_from_script_error_for_upload(e)
self._update_status("Unable to successfully do a clean build and test", results_file=failure_log)
self._update_status("Unable to build and test without patch", results_file=failure_log)
return False
return True
......@@ -221,16 +221,16 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
self._update_status("Landing %s" % patch_text, patch)
return True
def _land(self, patch, first_run=False):
def _build_and_test_patch(self, patch, first_run=False):
try:
args = [
"land-attachment",
"build-and-test-attachment",
"--force-clean",
"--build",
"--non-interactive",
"--ignore-builders",
"--build-style=both",
"--quiet",
"--parent-command=commit-queue",
patch.id()
]
# We don't bother to run tests for rollouts as that makes them too slow.
......@@ -248,31 +248,50 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler):
# built and tested.
args.append("--no-update")
self.run_webkit_patch(args)
self._did_pass(patch)
return True
except ScriptError, e:
failure_log = self._log_from_script_error_for_upload(e)
self._update_status("Unable to land patch", patch=patch, results_file=failure_log)
self._update_status("Unable to build and test patch", patch=patch, results_file=failure_log)
if first_run:
return False
self._did_fail(patch)
raise
def _land(self, patch):
try:
args = [
"land-attachment",
"--force-clean",
"--non-interactive",
"--ignore-builders",
"--quiet",
"--parent-command=commit-queue",
patch.id(),
]
self.run_webkit_patch(args)
self._did_pass(patch)
except ScriptError, e:
failure_log = self._log_from_script_error_for_upload(e)
self._update_status("Unable to land patch", patch=patch, results_file=failure_log)
self._did_fail(patch)
raise
def process_work_item(self, patch):
self._cc_watchers(patch.bug_id())
if not self._land(patch, first_run=True):
self._update_status("Doing a clean build as a sanity check", patch)
# The patch failed to land, but the bots were green. It's possible
# that the bots were behind. To check that case, we try to build and
# test ourselves.
if not self._can_build_and_test():
if not self._build_and_test_patch(patch, first_run=True):
self._update_status("Building and testing without the patch as a sanity check", patch)
# The patch failed to build and test. It's possible that the
# tree is busted. To check that case, we try to build and test
# without the patch.
if not self._can_build_and_test_without_patch():
return False
self._update_status("Clean build succeeded, trying patch again", patch)
self._update_status("Build and test succeeded, trying again with patch", patch)
# Hum, looks like the patch is actually bad. Of course, we could
# have been bitten by a flaky test the first time around. We try
# to land again. If it fails a second time, we're pretty sure its
# a bad test and re can reject it outright.
self._land(patch)
# to build and test again. If it fails a second time, we're pretty
# sure its a bad test and re can reject it outright.
self._build_and_test_patch(patch)
self._land(patch)
return True
def handle_unexpected_error(self, patch, message):
......
......@@ -183,7 +183,7 @@ MOCK setting flag 'commit-queue' to '-' on attachment '128' with comment 'Reject
MOCK: update_work_items: commit-queue [106, 197]
2 patches in commit-queue [106, 197]
""",
"process_work_item" : "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 1234, '--test']\nMOCK: update_status: commit-queue Pass\n",
"process_work_item" : "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build-and-test-attachment', '--force-clean', '--build', '--non-interactive', '--build-style=both', '--quiet', '--parent-command=commit-queue', 1234, '--test']\nMOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--non-interactive', '--ignore-builders', '--quiet', '--parent-command=commit-queue', 1234]\nMOCK: update_status: commit-queue Pass\n",
"handle_unexpected_error" : "MOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'Mock error message'\n",
"handle_script_error": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\n",
}
......@@ -207,7 +207,7 @@ MOCK setting flag 'commit-queue' to '-' on attachment '128' with comment 'Reject
MOCK: update_work_items: commit-queue [106, 197]
2 patches in commit-queue [106, 197]
""",
"process_work_item": "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 76543]\nMOCK: update_status: commit-queue Pass\n",
"process_work_item": "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build-and-test-attachment', '--force-clean', '--build', '--non-interactive', '--build-style=both', '--quiet', '--parent-command=commit-queue', 76543]\nMOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--non-interactive', '--ignore-builders', '--quiet', '--parent-command=commit-queue', 76543]\nMOCK: update_status: commit-queue Pass\n",
"handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '76543' with comment 'Rejecting patch 76543 from commit-queue.' and additional comment 'Mock error message'\n",
"handle_script_error": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\n",
}
......@@ -218,7 +218,7 @@ MOCK: update_work_items: commit-queue [106, 197]
tool = MockTool()
tool.executive = Mock()
queue.bind_to_tool(tool)
self.assertTrue(queue._can_build_and_test())
self.assertTrue(queue._can_build_and_test_without_patch())
expected_run_args = ["echo", "--status-host=example.com", "build-and-test", "--force-clean", "--build", "--test", "--non-interactive", "--no-update", "--build-style=both", "--quiet"]
tool.executive.run_and_throw_if_fail.assert_called_with(expected_run_args)
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment