First stab at using Rietveld wrapper object in both gcl.py and git-cl.

Exposes InputApi.rietveld and deprecate host_url.

This is useful since it places authentication at a single place.

BUG=
TEST=

Review URL: http://codereview.chromium.org/6825008

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@81019 0039d316-1c4b-4281-b951-d872f2087c98

gcl.py

@@ -8,7 +8,6 @@ Wrapper script around Rietveld's upload.py that simplifies working with groups
 of files.
 """
 
-import getpass
 import optparse
 import os
 import random
@@ -39,9 +38,10 @@ from scm import SVN
 import fix_encoding
 import gclient_utils
 import presubmit_support
+import rietveld
 import subprocess2
 
-__version__ = '1.2'
+__version__ = '1.2.1'
 
 
 CODEREVIEW_SETTINGS = {
@@ -283,7 +283,7 @@ class ChangeInfo(object):
   _SEPARATOR = "\n-----\n"
 
   def __init__(self, name, issue, patchset, description, files, local_root,
-               rietveld, needs_upload=False):
+               rietveld_url, needs_upload=False):
     self.name = name
     self.issue = int(issue)
     self.patchset = int(patchset)
@@ -297,10 +297,11 @@ class ChangeInfo(object):
     self.patch = None
     self._local_root = local_root
     self.needs_upload = needs_upload
-    self.rietveld = rietveld
+    self.rietveld = rietveld_url
     if not self.rietveld:
       # Set the default value.
       self.rietveld = GetCodeReviewSetting('CODE_REVIEW_SERVER')
+    self._rpc_server = None
 
   def _get_description(self):
     return self._description
@@ -384,6 +385,13 @@ class ChangeInfo(object):
     """Removes the changelist information from disk."""
     os.remove(GetChangelistInfoFile(self.name))
 
+  def RpcServer(self):
+    if not self._rpc_server:
+      if not self.rietveld:
+        ErrorExit(CODEREVIEW_SETTINGS_FILE_NOT_FOUND)
+      self._rpc_server = rietveld.Rietveld(self.rietveld, None, None)
+    return self._rpc_server
+
   def CloseIssue(self):
     """Closes the Rietveld issue for this changelist."""
     # Newer versions of Rietveld require us to pass an XSRF token to POST, so
@@ -420,19 +428,8 @@ class ChangeInfo(object):
 
   def SendToRietveld(self, request_path, timeout=None, **kwargs):
     """Send a POST/GET to Rietveld.  Returns the response body."""
-    if not self.rietveld:
-      ErrorExit(CODEREVIEW_SETTINGS_FILE_NOT_FOUND)
-    def GetUserCredentials():
-      """Prompts the user for a username and password."""
-      email = upload.GetEmail('Email (login for uploading to %s)' %
-          self.rietveld)
-      password = getpass.getpass('Password for %s: ' % email)
-      return email, password
-    rpc_server = upload.HttpRpcServer(self.rietveld,
-                                      GetUserCredentials,
-                                      save_cookies=True)
     try:
-      return rpc_server.Send(request_path, timeout=timeout, **kwargs)
+      return self.RpcServer().Send(request_path, timeout=timeout, **kwargs)
     except urllib2.URLError:
       if timeout is None:
         ErrorExit('Error accessing url %s' % request_path)
@@ -536,8 +533,9 @@ class ChangeInfo(object):
     if not os.path.exists(info_file):
       if fail_on_not_found:
         ErrorExit("Changelist " + changename + " not found.")
-      return ChangeInfo(changename, 0, 0, '', None, local_root, rietveld=None,
-                        needs_upload=False)
+      return ChangeInfo(
+          changename, 0, 0, '', None, local_root, rietveld_url=None,
+          needs_upload=False)
     content = gclient_utils.FileRead(info_file, 'r')
     save = False
     try:
@@ -1222,15 +1220,16 @@ def DoPresubmitChecks(change_info, committing, may_prompt):
                                        change_info.GetFiles(),
                                        change_info.issue,
                                        change_info.patchset)
-  output = presubmit_support.DoPresubmitChecks(change=change,
-                                               committing=committing,
-                                               verbose=False,
-                                               output_stream=sys.stdout,
-                                               input_stream=sys.stdin,
-                                               default_presubmit=root_presubmit,
-                                               may_prompt=may_prompt,
-                                               tbr=False,
-                                               host_url=change_info.rietveld)
+  output = presubmit_support.DoPresubmitChecks(
+      change=change,
+      committing=committing,
+      verbose=False,
+      output_stream=sys.stdout,
+      input_stream=sys.stdin,
+      default_presubmit=root_presubmit,
+      may_prompt=may_prompt,
+      tbr=False,
+      rietveld=change_info.RpcServer())
   if not output.should_continue() and may_prompt:
     # TODO(dpranke): move into DoPresubmitChecks(), unify cmd line args.
     print "\nPresubmit errors, can't continue (use --no_presubmit to bypass)"

git_cl.py

@@ -39,6 +39,7 @@ from third_party import upload
 import breakpad  # pylint: disable=W0611
 import fix_encoding
 import presubmit_support
+import rietveld
 import scm
 import watchlists
 
@@ -463,7 +464,7 @@ or verify this branch is set up to track another (via the --track argument to
     if not self.has_description:
       if self.GetIssue():
         path = '/' + self.GetIssue() + '/description'
-        rpc_server = self._RpcServer()
+        rpc_server = self.RpcServer()
         self.description = rpc_server.Send(path).strip()
       self.has_description = True
     if pretty:
@@ -494,9 +495,9 @@ or verify this branch is set up to track another (via the --track argument to
 
   def GetPatchSetDiff(self, issue):
     # Grab the last patchset of the issue first.
-    data = json.loads(self._RpcServer().Send('/api/%s' % issue))
+    data = json.loads(self.RpcServer().Send('/api/%s' % issue))
     patchset = data['patchsets'][-1]
-    return self._RpcServer().Send(
+    return self.RpcServer().Send(
         '/download/issue%s_%s.diff' % (issue, patchset))
 
   def SetIssue(self, issue):
@@ -511,7 +512,7 @@ or verify this branch is set up to track another (via the --track argument to
     self.has_issue = False
 
   def CloseIssue(self):
-    rpc_server = self._RpcServer()
+    rpc_server = self.RpcServer()
     # Newer versions of Rietveld require us to pass an XSRF token to POST, so
     # we fetch it from the server.  (The version used by Chromium has been
     # modified so the token isn't required when closing an issue.)
@@ -523,14 +524,15 @@ or verify this branch is set up to track another (via the --track argument to
     data = [("description", self.description),
             ("xsrf_token", xsrf_token)]
     ctype, body = upload.EncodeMultipartFormData(data, [])
-    rpc_server.Send('/' + self.GetIssue() + '/close', body, ctype)
+    rpc_server.Send(
+        '/' + self.GetIssue() + '/close', payload=body, content_type=ctype)
 
-  def _RpcServer(self):
+  def RpcServer(self):
     """Returns an upload.RpcServer() to access this review's rietveld instance.
     """
     if not self._rpc_server:
-      server = self.GetRietveldServer()
-      self._rpc_server = upload.GetRpcServer(server, save_cookies=True)
+      self.GetIssue()
+      self._rpc_server = rietveld.Rietveld(self.rietveld_server, None, None)
     return self._rpc_server
 
   def _IssueSetting(self):
@@ -848,7 +850,7 @@ def RunHook(committing, upstream_branch, rietveld_server, tbr, may_prompt,
     output = presubmit_support.DoPresubmitChecks(change, committing,
         verbose=verbose, output_stream=sys.stdout, input_stream=sys.stdin,
         default_presubmit=None, may_prompt=may_prompt, tbr=tbr,
-        host_url=cl.GetRietveldServer())
+        rietveld=cl.RpcServer())
   except presubmit_support.PresubmitFailure, e:
     DieWithError(
         ('%s\nMaybe your depot_tools is out of date?\n'

presubmit_support.py

@@ -6,7 +6,7 @@
 """Enables directory-specific presubmit checks to run at upload and/or commit.
 """
 
-__version__ = '1.6'
+__version__ = '1.6.1'
 
 # TODO(joi) Add caching where appropriate/needed. The API is designed to allow
 # caching (between all different invocations of presubmit scripts for a given
@@ -215,10 +215,8 @@ class InputApi(object):
       r"(|.*[\\\/])\.svn[\\\/].*",
   )
 
-  # TODO(dpranke): Update callers to pass in tbr, host_url, remove
-  # default arguments.
-  def __init__(self, change, presubmit_path, is_committing, tbr, host_url,
-      verbose):
+  def __init__(self, change, presubmit_path, is_committing, tbr,
+      rietveld, verbose):
     """Builds an InputApi object.
 
     Args:
@@ -226,15 +224,18 @@ class InputApi(object):
       presubmit_path: The path to the presubmit script being processed.
       is_committing: True if the change is about to be committed.
       tbr: True if '--tbr' was passed to skip any reviewer/owner checks
-      host_url: scheme, host, and path of rietveld instance
+      rietveld: rietveld client object
     """
     # Version number of the presubmit_support script.
     self.version = [int(x) for x in __version__.split('.')]
     self.change = change
-    self.host_url = host_url
     self.is_committing = is_committing
     self.tbr = tbr
-    self.host_url = host_url or 'http://codereview.chromium.org'
+    self.rietveld = rietveld
+    # TBD
+    self.host_url = 'http://codereview.chromium.org'
+    if self.rietveld:
+      self.host_url = rietveld.url
 
     # We expose various modules and functions as attributes of the input_api
     # so that presubmit scripts don't have to import them.
@@ -932,19 +933,18 @@ def DoGetTrySlaves(changed_files,
 
 
 class PresubmitExecuter(object):
-  def __init__(self, change, committing, tbr, host_url, verbose):
+  def __init__(self, change, committing, tbr, rietveld, verbose):
     """
     Args:
       change: The Change object.
       committing: True if 'gcl commit' is running, False if 'gcl upload' is.
       tbr: True if '--tbr' was passed to skip any reviewer/owner checks
-      host_url: scheme, host, and path of rietveld instance
-          (or None for default)
+      rietveld: rietveld client object.
     """
     self.change = change
     self.committing = committing
     self.tbr = tbr
-    self.host_url = host_url
+    self.rietveld = rietveld
     self.verbose = verbose
 
   def ExecPresubmitScript(self, script_text, presubmit_path):
@@ -965,7 +965,7 @@ class PresubmitExecuter(object):
 
     # Load the presubmit script into context.
     input_api = InputApi(self.change, presubmit_path, self.committing,
-                         self.tbr, self.host_url, self.verbose)
+                         self.tbr, self.rietveld, self.verbose)
     context = {}
     try:
       exec script_text in context
@@ -1000,7 +1000,6 @@ class PresubmitExecuter(object):
     return result
 
 
-# TODO(dpranke): make all callers pass in tbr, host_url?
 def DoPresubmitChecks(change,
                       committing,
                       verbose,
@@ -1008,8 +1007,8 @@ def DoPresubmitChecks(change,
                       input_stream,
                       default_presubmit,
                       may_prompt,
-                      tbr=False,
-                      host_url=None):
+                      tbr,
+                      rietveld):
   """Runs all presubmit checks that apply to the files in the change.
 
   This finds all PRESUBMIT.py files in directories enclosing the files in the
@@ -1028,8 +1027,7 @@ def DoPresubmitChecks(change,
     default_presubmit: A default presubmit script to execute in any case.
     may_prompt: Enable (y/n) questions on warning or error.
     tbr: was --tbr specified to skip any reviewer/owner checks?
-    host_url: scheme, host, and port of host to use for rietveld-related
-        checks
+    rietveld: rietveld object.
 
   Warning:
     If may_prompt is true, output_stream SHOULD be sys.stdout and input_stream
@@ -1050,7 +1048,7 @@ def DoPresubmitChecks(change,
   if not presubmit_files and verbose:
     output.write("Warning, no presubmit.py found.\n")
   results = []
-  executer = PresubmitExecuter(change, committing, tbr, host_url, verbose)
+  executer = PresubmitExecuter(change, committing, tbr, rietveld, verbose)
   if default_presubmit:
     if verbose:
       output.write("Running default presubmit script.\n")
@@ -1200,7 +1198,9 @@ def Main(argv):
         sys.stdout,
         sys.stdin,
         options.default_presubmit,
-        options.may_prompt)
+        options.may_prompt,
+        False,
+        None)
     return not results.should_continue()
   except PresubmitFailure, e:
     print >> sys.stderr, e

rietveld.py

@@ -42,15 +42,16 @@ class Rietveld(object):
   """Accesses rietveld."""
   def __init__(self, url, email, password):
     self.issue = None
-    self.user = email
     self.url = url
-    self._get_creds = lambda: (email, password)
+    if email and password:
+      get_creds = lambda: (email, password)
+      self.rpc_server = upload.HttpRpcServer(
+            self.url,
+            get_creds)
+    else:
+      self.rpc_server = upload.GetRpcServer(url, email)
     self._xsrf_token = None
     self._xsrf_token_time = None
-    self.rpc_server = upload.HttpRpcServer(
-          self.url,
-          self._get_creds,
-          save_cookies=False)
 
   def xsrf_token(self):
     if (not self._xsrf_token_time or
@@ -175,7 +176,8 @@ class Rietveld(object):
         (flag, value)])
 
   def get(self, request_path, **kwargs):
-    return self._send(request_path, payload=None, **kwargs)
+    kwargs.setdefault('payload', None)
+    return self._send(request_path, **kwargs)
 
   def post(self, request_path, data, **kwargs):
     ctype, body = upload.EncodeMultipartFormData(data, [])
@@ -203,3 +205,6 @@ class Rietveld(object):
           raise
       # If reaching this line, loop again. Uses a small backoff.
       time.sleep(1+maxtries*2)
+
+  # DEPRECATED.
+  Send = get

tests/gcl_unittest.py

@@ -91,9 +91,9 @@ class GclUnittest(GclTestsBase):
         'RunShell', 'RunShellWithReturnCode', 'SVN',
         'TryChange', 'UnknownFiles', 'Warn',
         'attrs', 'breakpad', 'defer_attributes', 'fix_encoding',
-        'gclient_utils', 'getpass',
-        'json', 'main', 'need_change', 'need_change_and_args', 'no_args',
-        'optparse', 'os', 'presubmit_support', 'random', 're',
+        'gclient_utils', 'json', 'main', 'need_change', 'need_change_and_args',
+        'no_args', 'optparse', 'os', 'presubmit_support', 'random', 're',
+        'rietveld',
         'string', 'subprocess', 'subprocess2', 'sys', 'tempfile', 'time',
         'upload', 'urllib2',
     ]
@@ -179,7 +179,7 @@ class ChangeInfoUnittest(GclTestsBase):
     members = [
       'CloseIssue', 'Delete', 'Exists', 'GetFiles', 'GetFileNames',
       'GetLocalRoot', 'GetIssueDescription', 'Load', 'MissingTests',
-      'NeedsUpload', 'PrimeLint', 'Save', 'SendToRietveld',
+      'NeedsUpload', 'PrimeLint', 'RpcServer', 'Save', 'SendToRietveld',
       'UpdateRietveldDescription',
       'description', 'issue', 'name',
       'needs_upload', 'patch', 'patchset', 'reviewers', 'rietveld',

tests/presubmit_unittest.py

@@ -418,7 +418,7 @@ class PresubmitUnittest(PresubmitTestsBase):
     change = presubmit.Change('mychange', '\n'.join(description_lines),
                               self.fake_root_dir, files, 0, 0)
     output = presubmit.DoPresubmitChecks(
-        change, False, True, None, input_buf, None, False)
+        change, False, True, None, input_buf, None, False, False, None)
     self.failIf(output.should_continue())
     self.assertEqual(output.getvalue().count('!!'), 2)
     self.assertEqual(output.getvalue().count(
@@ -452,13 +452,13 @@ class PresubmitUnittest(PresubmitTestsBase):
     change = presubmit.Change('mychange', '\n'.join(description_lines),
                               self.fake_root_dir, files, 0, 0)
     output = presubmit.DoPresubmitChecks(
-        change, False, True, None, input_buf, None, True)
+        change, False, True, None, input_buf, None, True, False, None)
     self.failIf(output.should_continue())
     self.assertEqual(output.getvalue().count('??'), 2)
 
     input_buf = StringIO.StringIO('y\n')  # say yes to the warning
     output = presubmit.DoPresubmitChecks(
-        change, False, True, None, input_buf, None, True)
+        change, False, True, None, input_buf, None, True, False, None)
     self.failUnless(output.should_continue())
     self.assertEquals(output.getvalue().count('??'), 2)
     self.assertEqual(output.getvalue().count(
@@ -491,7 +491,7 @@ class PresubmitUnittest(PresubmitTestsBase):
     change = presubmit.Change('mychange', '\n'.join(description_lines),
                               self.fake_root_dir, files, 0, 0)
     output = presubmit.DoPresubmitChecks(change, False, True, None, None,
-        None, False)
+        None, False, False, None)
     self.assertEqual(output.getvalue().count('??'), 2)
     self.assertEqual(output.getvalue().count('XX!!XX'), 2)
     self.assertEqual(output.getvalue().count('(y/N)'), 0)
@@ -528,7 +528,8 @@ def CheckChangeOnCommit(input_api, output_api):
     change = presubmit.Change('mychange', '\n'.join(description_lines),
                               self.fake_root_dir, files, 0, 0)
     output = presubmit.DoPresubmitChecks(
-        change, False, True, None, input_buf, DEFAULT_SCRIPT, False)
+        change, False, True, None, input_buf, DEFAULT_SCRIPT, False, False,
+        None)
     self.failIf(output.should_continue())
     text = ('Running presubmit upload checks ...\n'
             'Warning, no presubmit.py found.\n'
@@ -603,7 +604,8 @@ def CheckChangeOnCommit(input_api, output_api):
         'foo', "Blah Blah\n\nSTORY=http://tracker.com/42\nBUG=boo\n",
         self.fake_root_dir, None, 0, 0)
     self.failUnless(presubmit.DoPresubmitChecks(
-        change, False, True, output, input_buf, DEFAULT_SCRIPT, False))
+        change, False, True, output, input_buf, DEFAULT_SCRIPT, False, False,
+        None))
     self.assertEquals(output.getvalue(),
                       ('Running presubmit upload checks ...\n'
                        'Warning, no presubmit.py found.\n'
@@ -687,7 +689,7 @@ def CheckChangeOnCommit(input_api, output_api):
     presubmit.DoPresubmitChecks(mox.IgnoreArg(), False, False,
                                 mox.IgnoreArg(),
                                 mox.IgnoreArg(),
-                                None, False).AndReturn(output)
+                                None, False, False, None).AndReturn(output)
     self.mox.ReplayAll()
 
     self.assertEquals(
@@ -730,8 +732,8 @@ class InputApiUnittest(PresubmitTestsBase):
       'basename', 'cPickle', 'cStringIO', 'canned_checks', 'change', 'environ',
       'host_url', 'is_committing', 'json', 'marshal', 'os_listdir', 'os_walk',
       'os_path', 'owners_db', 'pickle', 'platform', 'python_executable', 're',
-      'subprocess', 'tbr', 'tempfile', 'time', 'traceback', 'unittest',
-      'urllib2', 'version', 'verbose',
+      'rietveld', 'subprocess', 'tbr', 'tempfile', 'time', 'traceback',
+      'unittest', 'urllib2', 'version', 'verbose',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(
@@ -771,7 +773,7 @@ class InputApiUnittest(PresubmitTestsBase):
     api = presubmit.InputApi(
         self.fake_change,
         presubmit_path='foo/path/PRESUBMIT.py',
-        is_committing=False, tbr=False, host_url=None, verbose=False)
+        is_committing=False, tbr=False, rietveld=None, verbose=False)
     self.assertEquals(api.PresubmitLocalPath(), 'foo/path')
     self.assertEquals(api.change, self.fake_change)
     self.assertEquals(api.host_url, 'http://codereview.chromium.org')
@@ -1008,7 +1010,7 @@ class InputApiUnittest(PresubmitTestsBase):
     presubmit_path = join(self.fake_root_dir, 'isdir', 'PRESUBMIT.py')
     api = presubmit.InputApi(
         change=change, presubmit_path=presubmit_path,
-        is_committing=True, tbr=False, host_url=None, verbose=False)
+        is_committing=True, tbr=False, rietveld=None, verbose=False)
     paths_from_api = api.AbsoluteLocalPaths(include_dirs=True)
     self.assertEqual(len(paths_from_api), 2)
     for absolute_paths in [paths_from_change, paths_from_api]:

third_party/upload.py

@@ -2068,9 +2068,6 @@ def RealMain(argv, data=None):
     The patchset id is None if the base files are not uploaded by this
     script (applies only to SVN checkouts).
   """
-  logging.basicConfig(format=("%(asctime).19s %(levelname)s %(filename)s:"
-                              "%(lineno)s %(message)s "))
-  os.environ['LC_ALL'] = 'C'
   options, args = parser.parse_args(argv[1:])
   global verbosity
   verbosity = options.verbose