Bug 53288 – Add webkit-patch roll-chromium-deps

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Add webkit-patch roll-chromium-deps

[Adam Barth]

Created attachment 80426 [details]
Patch

[Eric Seidel (no email)]

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.

[Adam Barth]

Committed r76926: <http://trac.webkit.org/changeset/76926>

[WebKit Review Bot]

http://trac.webkit.org/changeset/76926 might have broken Leopard Intel Release (Tests)

[Mihai Parparita]

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.

[Adam Barth]

> I think adding a build step (to make sure that things still build) would be a good idea.

Will do.