| Summary: | bugzilla-tool should automate rollouts | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> |
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | commit-queue, ddkilzer, evan, levin |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Mac | ||
| OS: | OS X 10.5 | ||
| Bug Depends on: | 28193 | ||
| Bug Blocks: | |||
| Attachments: | |||
|
Description
Eric Seidel (no email)
2009-06-25 01:22:15 PDT
Once we have a way to do automated rollbacks, then we could eventually consider adding a web interface to do so. :) Never have a red tree again! :) Created attachment 32173 [details]
Work in progress
Created attachment 32174 [details]
Working, but not ready for primetime
Created attachment 32974 [details]
Updated patch for rollout support
---
4 files changed, 189 insertions(+), 51 deletions(-)
Created attachment 32976 [details]
Begone ye tabs!
---
4 files changed, 189 insertions(+), 51 deletions(-)
Comment on attachment 32976 [details] Begone ye tabs! Just a few things to address. I'm most concerned about the issue I mentioned in update_changelog_for_revert. Two general notes: 1. It was pointed out to me that python style is to have two blank lines between top level functions: "Separate top-level function and class definitions with two blank lines." 2. Many comments don't end with periods. Please add them. > diff --git a/WebKitTools/Scripts/bugzilla-tool b/WebKitTools/Scripts/bugzilla-tool > +def update_changelog_for_revert(changelog_path, revision): > + removing_boilerplate = False > + # inplace=1 creates a backup file and re-directs stdout to the file > + for line in fileinput.FileInput(changelog_path, inplace=1): > + if re.search('Reviewed by', line): I'm confused. How does this stop after this boilerplate change log that was just created? It looks like it goes through the whole changelog removing all descriptions along the way. > + print line.replace("Reviewed by NOBODY (OOPS!)", "No review, rolling out %s" % revision), I like to prefix revisions with an r. Consider: ..."No review, rolling out r%s"... > +class RolloutCommit(Command): ... > + def create_changelogs_for_revert(scm, revision): ... > + # This could move to prepare-ChangeLog by adding a --revert= option That might be easier. I think the current code has a bug as I noted above. > diff --git a/WebKitTools/Scripts/modules/scm.py b/WebKitTools/Scripts/modules/scm.py > + @classmethod > + def git_commit_from_svn_revision(cls, revision): > + git_log_output = cls.run_command(['git', 'svn', 'log', '-r', revision, '--oneline', '--show-commit']) > + # Example: r45396 | b4954e7 | 2009-06-30 Antonio Gomes <antonio.gomes@openbossa.org> > + matched_commit = re.search("^r(\d+) \| (\w+) \| .*$", git_log_output, re.MULTILINE) This is ok, but I always find code more readable when it uses named groups. btw, ignore this comment:
> + # This could move to prepare-ChangeLog by adding a --revert= option
That might be easier. I think the current code has a bug as I noted above.
as I realized I was completely wrong about it being easier and I rather like it in these files :)
+ git_log_output = cls.run_command(['git', 'svn', 'log', '-r', revision, '--oneline', '--show-commit']) + # Example: r45396 | b4954e7 | 2009-06-30 Antonio Gomes <antonio.gomes@openbossa.org> + matched_commit = re.search("^r(\d+) \| (\w+) \| .*$", git_log_output, re.MULTILINE) git svn find-rev r45396 Created attachment 33930 [details]
Updated, just needs testing with more rollouts
---
4 files changed, 186 insertions(+), 53 deletions(-)
Some testing code for later:
def _parse_revision_to_commit_mappings(self, stderr):
revision_to_commit_mapping = {}
for line in stderr.split_lines():
match = line.match("^(?P<revision>r\d+) = (?P<commit_id>\w+)$")
if match:
revision_to_commit_mapping[match.group('revision')] = match.group('commit_id')
return revision_to_commit_mapping
Created attachment 34639 [details]
Updated to ToT
---
4 files changed, 123 insertions(+), 3 deletions(-)
Created attachment 35147 [details]
Update rollout support
---
7 files changed, 373 insertions(+), 49 deletions(-)
Created attachment 39377 [details]
Patch v1
Created attachment 39379 [details]
Works well enough for others to start to use
Created attachment 39381 [details]
Make ChangeLog less pessimistic
Comment on attachment 39381 [details]
Make ChangeLog less pessimistic
Would be nice to have some unittests for this change... We also discussed some alternate approaches if the ChangeLog conflicts are too problematic.
Created attachment 39397 [details]
Add a bunch of testing code for abarth
Created attachment 39398 [details]
Final patch with a bunch of extra testing
Note that I uploaded a diff from the reviewed patch which just contains the new testing changes as "Add a bunch of testing code for abarth". The final patch (including all the added tests) is up for review again. Created attachment 39400 [details]
Document all the extra testing in the ChangeLog
Comment on attachment 39400 [details]
Document all the extra testing in the ChangeLog
Thanks for the tests.
Comment on attachment 39400 [details]
Document all the extra testing in the ChangeLog
Rejecting patch 39400 from commit-queue.
This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment on attachment 39400 [details] Document all the extra testing in the ChangeLog Bitten by bug 28845. :( media/video-source-error.html -> timed out Comment on attachment 39400 [details] Document all the extra testing in the ChangeLog Clearing flags on attachment: 39400 Committed r48303: <http://trac.webkit.org/changeset/48303> All reviewed patches have been landed. Closing bug. Committed r48304: <http://trac.webkit.org/changeset/48304> |