2010-04-13 Ojan Vafai <ojan@chromium.org>
authorojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Apr 2010 00:23:39 +0000 (00:23 +0000)
committerojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Apr 2010 00:23:39 +0000 (00:23 +0000)
        Reviewed by David Levin.

        Add experimental prototype Rietveld integration to webkit-patch upload
        https://bugs.webkit.org/show_bug.cgi?id=37418

        This patch adds bare-bones integrtion with Rietveld for code reviews.
        The behavior is hidden behind the --fancy-review command line flag.
        Currently, there's no support for uploading more than one patch per
        issue (which is a nice feature of Rietveld).  The plan is to play with
        this for a bit and see if it's useful.

        Modified from Adam's original patch to autoinstall the rietveld upload script.

        * Scripts/webkitpy/common/config/__init__.py:
        * Scripts/webkitpy/common/net/rietveld.py: Added.
        * Scripts/webkitpy/common/net/rietveld_unitttest.py: Added.
        * Scripts/webkitpy/tool/commands/queues_unittest.py:
        * Scripts/webkitpy/tool/commands/upload.py:
        * Scripts/webkitpy/tool/commands/upload_unittest.py:
        * Scripts/webkitpy/tool/main.py:
        * Scripts/webkitpy/tool/mocktool.py:
        * Scripts/webkitpy/tool/steps/__init__.py:
        * Scripts/webkitpy/tool/steps/options.py:
        * Scripts/webkitpy/tool/steps/postcodereview.py: Added.
        * Scripts/webkitpy/tool/steps/postdiff.py:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@57552 268f45cc-cd09-0410-ab3c-d52691b4dbfc

14 files changed:
WebKitTools/ChangeLog
WebKitTools/Scripts/webkitpy/common/config/__init__.py
WebKitTools/Scripts/webkitpy/common/net/rietveld.py [new file with mode: 0644]
WebKitTools/Scripts/webkitpy/common/net/rietveld_unittest.py [new file with mode: 0644]
WebKitTools/Scripts/webkitpy/thirdparty/__init__.py
WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
WebKitTools/Scripts/webkitpy/tool/commands/upload.py
WebKitTools/Scripts/webkitpy/tool/commands/upload_unittest.py
WebKitTools/Scripts/webkitpy/tool/main.py
WebKitTools/Scripts/webkitpy/tool/mocktool.py
WebKitTools/Scripts/webkitpy/tool/steps/__init__.py
WebKitTools/Scripts/webkitpy/tool/steps/options.py
WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py [new file with mode: 0644]
WebKitTools/Scripts/webkitpy/tool/steps/postdiff.py

index eb1d869..0d51f8e 100644 (file)
@@ -1,3 +1,31 @@
+2010-04-13  Ojan Vafai  <ojan@chromium.org>
+
+        Reviewed by David Levin.
+
+        Add experimental prototype Rietveld integration to webkit-patch upload
+        https://bugs.webkit.org/show_bug.cgi?id=37418
+
+        This patch adds bare-bones integrtion with Rietveld for code reviews.
+        The behavior is hidden behind the --fancy-review command line flag.
+        Currently, there's no support for uploading more than one patch per
+        issue (which is a nice feature of Rietveld).  The plan is to play with
+        this for a bit and see if it's useful.
+
+        Modified from Adam's original patch to autoinstall the rietveld upload script.
+
+        * Scripts/webkitpy/common/config/__init__.py:
+        * Scripts/webkitpy/common/net/rietveld.py: Added.
+        * Scripts/webkitpy/common/net/rietveld_unitttest.py: Added.
+        * Scripts/webkitpy/tool/commands/queues_unittest.py:
+        * Scripts/webkitpy/tool/commands/upload.py:
+        * Scripts/webkitpy/tool/commands/upload_unittest.py:
+        * Scripts/webkitpy/tool/main.py:
+        * Scripts/webkitpy/tool/mocktool.py:
+        * Scripts/webkitpy/tool/steps/__init__.py:
+        * Scripts/webkitpy/tool/steps/options.py:
+        * Scripts/webkitpy/tool/steps/postcodereview.py: Added.
+        * Scripts/webkitpy/tool/steps/postdiff.py:
+
 2010-04-13  Sam Weinig  <sam@webkit.org>
 
         Rubber-stamped by Mark Rowe.
index ef65bee..03f1bc7 100644 (file)
@@ -1 +1,7 @@
 # Required for Python to search this directory for module files
+
+import re
+
+codereview_server_host = "wkrietveld.appspot.com"
+codereview_server_regex = "https?://%s/" % re.sub('\.', '\\.', codereview_server_host)
+codereview_server_url = "https://%s/" % codereview_server_host
diff --git a/WebKitTools/Scripts/webkitpy/common/net/rietveld.py b/WebKitTools/Scripts/webkitpy/common/net/rietveld.py
new file mode 100644 (file)
index 0000000..a9e5b1a
--- /dev/null
@@ -0,0 +1,89 @@
+# Copyright (c) 2010 Google Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+#     * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above
+# copyright notice, this list of conditions and the following disclaimer
+# in the documentation and/or other materials provided with the
+# distribution.
+#     * Neither the name of Google Inc. nor the names of its
+# contributors may be used to endorse or promote products derived from
+# this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+import os
+import re
+import stat
+
+import webkitpy.common.config as config
+from webkitpy.common.system.deprecated_logging import log
+from webkitpy.common.system.executive import ScriptError
+import webkitpy.thirdparty.autoinstalled.rietveld.upload as upload
+
+
+def parse_codereview_issue(message):
+    if not message:
+        return None
+    match = re.search(config.codereview_server_regex +
+                      "(?P<codereview_issue>\d+)",
+                      message)
+    if match:
+        return int(match.group('codereview_issue'))
+
+
+class Rietveld(object):
+    def __init__(self, executive, dryrun=False):
+        self.dryrun = dryrun
+        self._executive = executive
+        self._upload_py = upload.__file__
+        # Chop off the last character so we modify permissions on the py file instead of the pyc.
+        if os.path.splitext(self._upload_py)[1] == ".pyc":
+            self._upload_py = self._upload_py[:-1]
+        os.chmod(self._upload_py, os.stat(self._upload_py).st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
+
+    def url_for_issue(self, codereview_issue):
+        if not codereview_issue:
+            return None
+        return "%s%s" % (config.codereview_server_url, codereview_issue)
+
+    def post(self, message=None, codereview_issue=None, cc=None):
+        if not message:
+            raise ScriptError("Rietveld requires a message.")
+
+        args = [
+            self._upload_py,
+            "--assume_yes",
+            "--server=%s" % config.codereview_server_host,
+            "--message=%s" % message,
+        ]
+        if codereview_issue:
+            args.append("--issue=%s" % codereview_issue)
+        if cc:
+            args.append("--cc=%s" % cc)
+
+        if self.dryrun:
+            log("Would have run %s" % args)
+            return
+
+        output = self._executive.run_and_throw_if_fail(args)
+        match = re.search("Issue created\. URL: " +
+                          config.codereview_server_regex +
+                          "(?P<codereview_issue>\d+)",
+                          output)
+        if match:
+            return int(match.group('codereview_issue'))
diff --git a/WebKitTools/Scripts/webkitpy/common/net/rietveld_unittest.py b/WebKitTools/Scripts/webkitpy/common/net/rietveld_unittest.py
new file mode 100644 (file)
index 0000000..da5e0f5
--- /dev/null
@@ -0,0 +1,39 @@
+# Copyright (c) 2010 Google Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+#     * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above
+# copyright notice, this list of conditions and the following disclaimer
+# in the documentation and/or other materials provided with the
+# distribution.
+#     * Neither the name of Google Inc. nor the names of its
+# contributors may be used to endorse or promote products derived from
+# this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+import unittest
+
+from webkitpy.common.net.rietveld import Rietveld
+from webkitpy.thirdparty.mock import Mock
+
+
+class RietveldTest(unittest.TestCase):
+    def test_url_for_issue(self):
+        rietveld = Rietveld(Mock())
+        self.assertEqual(rietveld.url_for_issue(34223),
+                         "https://codereview.appspot.com/34223")
index 312da53..f1e5334 100644 (file)
@@ -69,6 +69,13 @@ installer.install(url="http://pypi.python.org/packages/source/m/mechanize/mechan
 installer.install(url="http://pypi.python.org/packages/source/p/pep8/pep8-0.5.0.tar.gz#md5=512a818af9979290cd619cce8e9c2e2b",
                   url_subpath="pep8-0.5.0/pep8.py")
 
+
+rietveld_dir = os.path.join(autoinstalled_dir, "rietveld")
+installer = AutoInstaller(target_dir=rietveld_dir)
+installer.install(url="http://webkit-rietveld.googlecode.com/svn/trunk/static/upload.py",
+                  target_name="upload.py")
+
+
 # Since irclib and ircbot are two top-level packages, we need to import
 # them separately.  We group them into an irc package for better
 # organization purposes.
index 88a37f3..0906c70 100644 (file)
@@ -71,6 +71,7 @@ class AbstractQueueTest(CommandsTest):
     def _assert_run_webkit_patch(self, run_args):
         queue = TestQueue()
         tool = MockTool()
+        tool.executive = Mock()
         queue.bind_to_tool(tool)
 
         queue.run_webkit_patch(run_args)
index 6e68fbe..dd0ac13 100644 (file)
@@ -164,6 +164,7 @@ class Post(AbstractPatchUploadingCommand):
     steps = [
         steps.CheckStyle,
         steps.ConfirmDiff,
+        steps.PostCodeReview,
         steps.ObsoletePatches,
         steps.PostDiff,
     ]
@@ -208,6 +209,7 @@ class Upload(AbstractPatchUploadingCommand):
         steps.PrepareChangeLog,
         steps.EditChangeLog,
         steps.ConfirmDiff,
+        steps.PostCodeReview,
         steps.ObsoletePatches,
         steps.PostDiff,
     ]
index 4e987c9..9766e19 100644 (file)
@@ -58,6 +58,7 @@ class UploadCommandsTest(CommandsTest):
         options.description = "MOCK description"
         options.request_commit = False
         options.review = True
+        options.cc = None
         expected_stderr = "Running check-webkit-style\nObsoleting 2 old patches on bug 42\nMOCK add_patch_to_bug: bug_id=42, description=MOCK description, mark_for_review=True, mark_for_commit_queue=False, mark_for_landing=False\n-- Begin comment --\nNone\n-- End comment --\nMOCK: user.open_url: http://example.com/42\n"
         self.assert_execute_outputs(Post(), [42], options=options, expected_stderr=expected_stderr)
 
@@ -77,6 +78,7 @@ class UploadCommandsTest(CommandsTest):
         options.description = "MOCK description"
         options.request_commit = False
         options.review = True
+        options.cc = None
         expected_stderr = "Running check-webkit-style\nObsoleting 2 old patches on bug 42\nMOCK add_patch_to_bug: bug_id=42, description=MOCK description, mark_for_review=True, mark_for_commit_queue=False, mark_for_landing=False\n-- Begin comment --\nNone\n-- End comment --\nMOCK: user.open_url: http://example.com/42\n"
         self.assert_execute_outputs(Upload(), [42], options=options, expected_stderr=expected_stderr)
 
index 629e86f..dd7652d 100755 (executable)
@@ -36,6 +36,7 @@ from webkitpy.common.checkout.api import Checkout
 from webkitpy.common.checkout.scm import detect_scm_system
 from webkitpy.common.net.bugzilla import Bugzilla
 from webkitpy.common.net.buildbot import BuildBot
+from webkitpy.common.net.rietveld import Rietveld
 from webkitpy.common.net.irc.ircproxy import IRCProxy
 from webkitpy.common.system.executive import Executive
 from webkitpy.common.system.user import User
@@ -70,6 +71,7 @@ class WebKitPatch(MultiCommandTool):
         self._scm = None
         self._checkout = None
         self.status_server = StatusServer()
+        self.codereview = Rietveld(self.executive)
 
     def scm(self):
         # Lazily initialize SCM to not error-out before command line parsing (or when running non-scm commands).
@@ -121,6 +123,7 @@ class WebKitPatch(MultiCommandTool):
         if options.dry_run:
             self.scm().dryrun = True
             self.bugs.dryrun = True
+            self.codereview.dryrun = True
         if options.status_host:
             self.status_server.set_host(options.status_host)
         if options.irc_password:
index 89fde6a..6e4b64f 100644 (file)
@@ -33,6 +33,7 @@ from webkitpy.common.config.committers import CommitterList, Reviewer
 from webkitpy.common.checkout.commitinfo import CommitInfo
 from webkitpy.common.checkout.scm import CommitMessage
 from webkitpy.common.net.bugzilla import Bug, Attachment
+from webkitpy.common.net.rietveld import Rietveld
 from webkitpy.thirdparty.mock import Mock
 from webkitpy.common.system.deprecated_logging import log
 
@@ -43,6 +44,7 @@ def _id_to_object_dictionary(*objects):
         dictionary[thing["id"]] = thing
     return dictionary
 
+# Testing
 
 # FIXME: The ids should be 1, 2, 3 instead of crazy numbers.
 
@@ -488,13 +490,18 @@ class MockStatusServer(object):
         return 191
 
 
+class MockExecute(Mock):
+    def run_and_throw_if_fail(self, args, quiet=False):
+        return "MOCK output of child process"
+
+
 class MockTool():
 
     def __init__(self, log_executive=False):
         self.wakeup_event = threading.Event()
         self.bugs = MockBugzilla()
         self.buildbot = MockBuildBot()
-        self.executive = Mock()
+        self.executive = MockExecute()
         if log_executive:
             self.executive.run_and_throw_if_fail = lambda args: log("MOCK run_and_throw_if_fail: %s" % args)
         self._irc = None
@@ -503,6 +510,7 @@ class MockTool():
         self._checkout = MockCheckout()
         self.status_server = MockStatusServer()
         self.irc_password = "MOCK irc password"
+        self.codereview = Rietveld(self.executive)
 
     def scm(self):
         return self._scm
index 8e8ab15..d59cdc5 100644 (file)
@@ -44,6 +44,7 @@ from webkitpy.tool.steps.ensurebuildersaregreen import EnsureBuildersAreGreen
 from webkitpy.tool.steps.ensurelocalcommitifneeded import EnsureLocalCommitIfNeeded
 from webkitpy.tool.steps.obsoletepatches import ObsoletePatches
 from webkitpy.tool.steps.options import Options
+from webkitpy.tool.steps.postcodereview import PostCodeReview
 from webkitpy.tool.steps.postdiff import PostDiff
 from webkitpy.tool.steps.postdiffforcommit import PostDiffForCommit
 from webkitpy.tool.steps.postdiffforrevert import PostDiffForRevert
index 40a6680..7f76f55 100644 (file)
@@ -40,6 +40,7 @@ class Options(object):
     confirm = make_option("--no-confirm", action="store_false", dest="confirm", default=True, help="Skip confirmation steps.")
     description = make_option("-m", "--description", action="store", type="string", dest="description", help="Description string for the attachment (default: \"patch\")")
     email = make_option("--email", action="store", type="string", dest="email", help="Email address to use in ChangeLogs.")
+    fancy_review = make_option("--fancy-review", action="store_true", dest="fancy_review", default=False, help="(Experimental) Upload the patch to Rietveld code review tool.")
     force_clean = make_option("--force-clean", action="store_true", dest="force_clean", default=False, help="Clean working directory before applying patches (removes local changes and commits)")
     local_commit = make_option("--local-commit", action="store_true", dest="local_commit", default=False, help="Make a local commit for each applied patch")
     non_interactive = make_option("--non-interactive", action="store_true", dest="non_interactive", default=False, help="Never prompt the user, fail as fast as possible.")
diff --git a/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py b/WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py
new file mode 100644 (file)
index 0000000..4da5285
--- /dev/null
@@ -0,0 +1,76 @@
+# Copyright (C) 2010 Google Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+#     * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above
+# copyright notice, this list of conditions and the following disclaimer
+# in the documentation and/or other materials provided with the
+# distribution.
+#     * Neither the name of Google Inc. nor the names of its
+# contributors may be used to endorse or promote products derived from
+# this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+from webkitpy.tool.steps.abstractstep import AbstractStep
+from webkitpy.tool.steps.options import Options
+
+
+class PostCodeReview(AbstractStep):
+    @classmethod
+    def options(cls):
+        return [
+            Options.cc,
+            Options.description,
+            Options.fancy_review,
+            Options.review,
+        ]
+
+    def run(self, state):
+        if not self._options.fancy_review:
+            return
+        if not self._options.review:
+            return
+        # FIXME: This will always be None because we don't retrieve the issue
+        #        number from the ChangeLog yet.
+        codereview_issue = state.get("codereview_issue")
+        message = self._options.description
+        if not message:
+            # If we have an issue number, then the message becomes the label
+            # of the new patch. Otherwise, it becomes the title of the whole
+            # issue.
+            if codereview_issue:
+                message = "Updated patch"
+            elif state.get("bug_title"):
+                # This is the common case for the the first "upload" command.
+                message = state.get("bug_title")
+            elif state.get("bug_id"):
+                # This is the common case for the "post" command and
+                # subsequent runs of the "upload" command.  In this case,
+                # I'd rather add the new patch to the existing issue, but
+                # that's not implemented yet.
+                message = "Code review for %s" % self._tool.bugs.bug_url_for_bug_id(state["bug_id"])
+            else:
+                # Unreachable with our current commands, but we might hit
+                # this case if we support bug-less code reviews.
+                message = "Code review"
+        created_issue = self._tool.codereview.post(message=message,
+                                                   codereview_issue=codereview_issue,
+                                                   cc=self._options.cc)
+        if created_issue:
+            # FIXME: Record the issue number in the ChangeLog.
+            state["codereview_issue"] = created_issue
index ef8f4c8..5b28383 100644 (file)
@@ -46,6 +46,10 @@ class PostDiff(AbstractStep):
         diff = self.cached_lookup(state, "diff")
         diff_file = StringIO.StringIO(diff) # add_patch_to_bug expects a file-like object
         description = self._options.description or "Patch"
-        self._tool.bugs.add_patch_to_bug(state["bug_id"], diff_file, description, mark_for_review=self._options.review, mark_for_commit_queue=self._options.request_commit)
+        comment_text = None
+        codereview_issue = state.get("codereview_issue")
+        if codereview_issue:
+            comment_text = "Feel free to provide comments at %s" % self._tool.codereview.url_for_issue(codereview_issue)
+        self._tool.bugs.add_patch_to_bug(state["bug_id"], diff_file, description, comment_text=comment_text, mark_for_review=self._options.review, mark_for_commit_queue=self._options.request_commit)
         if self._options.open_bug:
             self._tool.user.open_url(self._tool.bugs.bug_url_for_bug_id(state["bug_id"]))