Bug 53288 - Add webkit-patch roll-chromium-deps
Summary: Add webkit-patch roll-chromium-deps
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-28 01:04 PST by Adam Barth
Modified: 2011-01-28 11:20 PST (History)
6 users (show)

See Also:


Attachments
Patch (41.18 KB, patch)
2011-01-28 01:10 PST, Adam Barth
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-01-28 01:04:45 PST
Add webkit-patch roll-chromium-deps
Comment 1 Adam Barth 2011-01-28 01:10:25 PST
Created attachment 80426 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-01-28 02:01:05 PST
Comment on attachment 80426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80426&action=review

In general this seems fine as a v1.  Thanks.

> Tools/Scripts/webkitpy/common/checkout/api.py:153
> +    def chromium_deps(self):
> +        return DEPS(os.path.join(self._scm.checkout_root, "Source", "WebKit", "chromium", "DEPS"))

This doesn't really seem to belong on Checkout, IMO.  But I guess it's OK.

> Tools/Scripts/webkitpy/tool/mocktool.py:498
> +class MockChromiumDEPS(object):

MockDEPs, no?

> Tools/Scripts/webkitpy/tool/commands/roll_unittest.py:48
> +            self.assert_execute_outputs(RollChromiumDEPS(), [5764])

Just add an expected_exception argument and pass it along to OutputCapture, no?

> Tools/Scripts/webkitpy/tool/steps/preparechangelogfordepsroll.py:40
> +            ChangeLog(changelog_path).update_with_unreviewed_message("Rolled DEPS.\n\n")

In this case seems it should be rolled to latest known good revisoin (maybe even with the link).  Exept that doesn't belong here.  You'd want to pass that in from the command or something, no?  I guess this wants to pull a message off of state?

Yes, that could be v2.

> Tools/Scripts/webkitpy/tool/steps/preparechangelogforrevert_unittest.py:41
> +class UpdateChangeLogsWithReviewerTest(unittest.TestCase):

None of these have reviewers.  Seems this is the wrong name for the class?

> Tools/Scripts/webkitpy/tool/steps/updatechromiumdeps.py:43
> +        new_chromium_revision = state["chromium_revision"]

This needs a comment as to why you use [] instead of .get().

> Tools/Scripts/webkitpy/tool/steps/updatechromiumdeps.py:57
> +        if new_chromium_revision < current_chromium_revision:
> +            log("Current Chromium DEPS revision %s is newer than %s." % (current_chromium_revision, new_chromium_revision))
> +            new_chromium_revision = self._tool.user.prompt("Enter new chromium revision (enter nothing to cancel):\n")
> +            try:
> +                new_chromium_revision = int(new_chromium_revision)
> +            except ValueError, TypeError:
> +                new_chromium_revision = None
> +            if not new_chromium_revision:
> +                error("Unable to update Chromium DEPS")

I would have split this out into some sort of _validate_blah_blah method.
Comment 3 Adam Barth 2011-01-28 02:14:31 PST
Committed r76926: <http://trac.webkit.org/changeset/76926>
Comment 4 WebKit Review Bot 2011-01-28 03:50:23 PST
http://trac.webkit.org/changeset/76926 might have broken Leopard Intel Release (Tests)
Comment 5 Mihai Parparita 2011-01-28 08:43:21 PST
Comment on attachment 80426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80426&action=review

> Tools/Scripts/webkitpy/tool/commands/roll.py:38
> +    steps = [

Given that DEPS rolls can result in a broken build unless other entries in the DEPS file are updated (e.g. http://trac.webkit.org/changeset/76863), I think adding a build step (to make sure that things still build) would be a good idea.
Comment 6 Adam Barth 2011-01-28 11:20:31 PST
> I think adding a build step (to make sure that things still build) would be a good idea.

Will do.