Reland Implement owners check in presubmit for Gerrit.

presubmit_support.py

 #!/usr/bin/env python
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Enables directory-specific presubmit checks to run at upload and/or commit.
 """
 
 __version__ = '1.8.0'
 
@@ skipping 179 matching lines @@
 
 
 # Top level object so multiprocessing can pickle
 # Public access through OutputApi object.
 class _MailTextResult(_PresubmitResult):
   """A warning that should be included in the review request email."""
   def __init__(self, *args, **kwargs):
     super(_MailTextResult, self).__init__()
     raise NotImplementedError()
 
+class GerritAccessor(object):
+  """Limited Gerrit functionality for canned presubmit checks to work.
+
+  To avoid excessive Gerrit calls, caches the results.
+  """
+
+  def __init__(self, host):
+    self.host = host
+    self.cache = {}
+
+  def _FetchChangeDetail(self, issue):
+    # Separate function to be easily mocked in tests.
+    return gerrit_util.GetChangeDetail(
+        self.host, str(issue),
+        ['ALL_REVISIONS', 'DETAILED_LABELS'])
+
+  def GetChangeInfo(self, issue, force=False):
+    assert issue
+    cache_key = int(issue)
+    if force or cache_key not in self.cache:
+      self.cache[cache_key] = self._FetchChangeDetail(issue)
+    return self.cache[cache_key]
+
+  def GetChangeDescription(self, issue, patchset=None, force=False):

[Michael Achenbach, 2016/04/28 07:17:30]

Who's using force=True?

[tandrii(chromium), 2016/04/28 19:31:50]

nobody any more! Removed.

+    """If patchset is none, fetches current patchset."""
+    self.GetChangeInfo(issue, force=force)  # Ensure details are in cache.

[Michael Achenbach, 2016/04/28 07:17:30]

Why not:
info = self.GetChangeInfo(issue, force=force)

delete next line and s/self.cache[cache_key]/info/g below

[tandrii(chromium), 2016/04/28 19:31:50]

good question. I thought modifying cache explicitly, rather than implicitly, is
better. But, your suggestion + comment is even better, i think.

+    cache_key = int(issue)
+
+    if patchset:

[Michael Achenbach, 2016/04/28 07:17:30]

I assume gerrit patchset numbers are not starting with 0.

[tandrii(chromium), 2016/04/28 19:31:51]

yes, they don't.

+      for rev, rev_info in self.cache[cache_key]['revisions'].iteritems():
+        if str(rev_info['_number']) == str(patchset):
+          break
+      else:
+        raise Exception('patchset %s doesn\'t exist in issue %s' % (
+            patchset, issue))
+    else:
+      rev = self.cache[cache_key]['current_revision']
+      rev_info = self.cache[cache_key]['revisions'][rev]

[Michael Achenbach, 2016/04/28 07:17:30]

Can you explain of what each of the revisions is comprised? Above it looks like
a rev, rev_info tuple. Now you just assign to rev_info.

[tandrii(chromium), 2016/04/28 19:31:50]

rev_info is dict; rev, rev_info is a tuple, but that's from iteritems().
actual example is in test. Should a comment here be useful?

[Michael Achenbach, 2016/04/29 08:10:18]

No, just me being stupid. Got it now.

+
+    if 'real_description' not in rev_info:
+      rev_info['real_description'] = (
+          gerrit_util.GetChangeDescriptionFromGitiles(
+              rev_info['fetch']['http']['url'], rev))
+    return rev_info['real_description']
+
+  def GetChangeOwner(self, issue):
+    return self.GetChangeInfo(issue)['owner']['email']
+
+  def GetChangeReviewers(self, issue, approving_only=True):
+    # Gerrit has 'approved' sub-section, but it only lists 1 approver.
+    # So, if we look only for approvers, we have to look at all anyway.
+    # Also, assume LGTM means Code-Review label == 2. Other configurations
+    # aren't supported.
+    return [r['email']
+            for r in self.GetChangeInfo(issue)['labels']['Code-Review']['all']

[Michael Achenbach, 2016/04/28 07:17:30]

Do you have a reference to a place that explains how the change info is built
up? Otherwise I can only rubber stamp this.

OK - later I looked at the test which explains it somewhat. So never mind.

[tandrii(chromium), 2016/04/28 19:31:50]

https://gerrit-review.googlesource.com/Documentation/rest-api.html is the 
sorta-reference, but it's not clear from it how 'labels' section looks like.
Still, it's better than rietveld API :)

+            if not approving_only or '2' == str(r.get('value', 0))]
+
 
 class OutputApi(object):
   """An instance of OutputApi gets passed to presubmit scripts so that they
   can output various types of results.
   """
   PresubmitResult = _PresubmitResult
   PresubmitAddReviewers = _PresubmitAddReviewers
   PresubmitError = _PresubmitError
   PresubmitPromptWarning = _PresubmitPromptWarning
   PresubmitNotifyResult = _PresubmitNotifyResult
@@ skipping 48 matching lines @@
       r".*\b[A-Z0-9_]{2,}$",
       # SCM (can happen in dual SCM configuration). (Slightly over aggressive)
       r"(|.*[\\\/])\.git[\\\/].*",
       r"(|.*[\\\/])\.svn[\\\/].*",
       # There is no point in processing a patch file.
       r".+\.diff$",
       r".+\.patch$",
   )
 
   def __init__(self, change, presubmit_path, is_committing,
-      rietveld_obj, verbose, dry_run=None):
+      rietveld_obj, verbose, dry_run=None, gerrit_obj=None):
     """Builds an InputApi object.
 
     Args:
       change: A presubmit.Change object.
       presubmit_path: The path to the presubmit script being processed.
       is_committing: True if the change is about to be committed.
       rietveld_obj: rietveld.Rietveld client object

[Michael Achenbach, 2016/04/28 07:17:30]

nit: Add to docu for consistency, though the rietveld line is not saying much...

[tandrii(chromium), 2016/04/28 19:31:51]

Done.

     """
     # Version number of the presubmit_support script.
     self.version = [int(x) for x in __version__.split('.')]
     self.change = change
     self.is_committing = is_committing
     self.rietveld = rietveld_obj
     self.dry_run = dry_run
+    self.gerrit = gerrit_obj
     # TBD
     self.host_url = 'http://codereview.chromium.org'
     if self.rietveld:
       self.host_url = self.rietveld.url
 
     # We expose various modules and functions as attributes of the input_api
     # so that presubmit scripts don't have to import them.
     self.basename = os.path.basename
     self.cPickle = cPickle
     self.cpplint = cpplint
@@ skipping 1049 matching lines @@
     output_stream.write('** Post Upload Hook Messages **\n')
   for result in results:
     result.handle(output_stream)
     output_stream.write('\n')
 
   return results
 
 
 class PresubmitExecuter(object):
   def __init__(self, change, committing, rietveld_obj, verbose,
-               dry_run=None):
+      dry_run=None, gerrit_obj=None):

[Michael Achenbach, 2016/04/28 07:17:30]

nit: Maybe keep ( alignment? Or move all into a new line.

[tandrii(chromium), 2016/04/28 19:31:51]

Done.

     """
     Args:
       change: The Change object.
       committing: True if 'gcl commit' is running, False if 'gcl upload' is.
       rietveld_obj: rietveld.Rietveld client object.

[Michael Achenbach, 2016/04/28 07:17:30]

nit: Add to docu for consistency, though the rietveld line is not saying much...

[tandrii(chromium), 2016/04/28 19:31:51]

Done.

     """
     self.change = change
     self.committing = committing
     self.rietveld = rietveld_obj
+    self.gerrit = gerrit_obj
     self.verbose = verbose
     self.dry_run = dry_run
 
   def ExecPresubmitScript(self, script_text, presubmit_path):
     """Executes a single presubmit script.
 
     Args:
       script_text: The text of the presubmit script.
       presubmit_path: The path to the presubmit file (this will be reported via
         input_api.PresubmitLocalPath()).
 
     Return:
       A list of result objects, empty if no problems.
     """
 
     # Change to the presubmit file's directory to support local imports.
     main_path = os.getcwd()
     os.chdir(os.path.dirname(presubmit_path))
 
     # Load the presubmit script into context.
     input_api = InputApi(self.change, presubmit_path, self.committing,
                          self.rietveld, self.verbose,
-                         dry_run=self.dry_run)
+                         self.dry_run, self.gerrit)
     context = {}
     try:
       exec script_text in context
     except Exception, e:
       raise PresubmitFailure('"%s" had an exception.\n%s' % (presubmit_path, e))
 
     # These function names must change if we make substantial changes to
     # the presubmit API that are not backwards compatible.
     if self.committing:
       function_name = 'CheckChangeOnCommit'
@@ skipping 22 matching lines @@
 
 
 def DoPresubmitChecks(change,
                       committing,
                       verbose,
                       output_stream,
                       input_stream,
                       default_presubmit,
                       may_prompt,
                       rietveld_obj,
-                      dry_run=None):
+                      dry_run=None,
+                      gerrit_obj=None):

[Michael Achenbach, 2016/04/28 07:17:30]

Just some order nit: The dry_run feature is quite new, right? Maybe you can
sneak in gerrit_obj next to rietveld_obj here without breaking the world?

[tandrii(chromium), 2016/04/28 19:31:50]

Done.

   """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
   change (up to the repository root) and calls the relevant entrypoint function
   depending on whether the change is being committed or uploaded.
 
   Prints errors, warnings and notifications.  Prompts the user for warnings
   when needed.
 
   Args:
     change: The Change object.
     committing: True if 'gcl commit' is running, False if 'gcl upload' is.
     verbose: Prints debug info.
     output_stream: A stream to write output from presubmit tests to.
     input_stream: A stream to read input from the user.
     default_presubmit: A default presubmit script to execute in any case.
     may_prompt: Enable (y/n) questions on warning or error.
     rietveld_obj: rietveld.Rietveld object.
     dry_run: if true, some Checks will be skipped.
+    gerrit_obj: provides basic Gerrit codereview functionality.
 
   Warning:
     If may_prompt is true, output_stream SHOULD be sys.stdout and input_stream
     SHOULD be sys.stdin.
 
   Return:
     A PresubmitOutput object. Use output.should_continue() to figure out
     if there were errors or warnings and the caller should abort.
   """
   old_environ = os.environ
   try:
     # Make sure python subprocesses won't generate .pyc files.
     os.environ = os.environ.copy()
     os.environ['PYTHONDONTWRITEBYTECODE'] = '1'
 
     output = PresubmitOutput(input_stream, output_stream)
     if committing:
       output.write("Running presubmit commit checks ...\n")
     else:
       output.write("Running presubmit upload checks ...\n")
     start_time = time.time()
     presubmit_files = ListRelevantPresubmitFiles(
         change.AbsoluteLocalPaths(True), change.RepositoryRoot())
     if not presubmit_files and verbose:
       output.write("Warning, no PRESUBMIT.py found.\n")
     results = []
     executer = PresubmitExecuter(change, committing, rietveld_obj, verbose,
-                                 dry_run=dry_run)
+                                 dry_run, gerrit_obj)
     if default_presubmit:
       if verbose:
         output.write("Running default presubmit script.\n")
       fake_path = os.path.join(change.RepositoryRoot(), 'PRESUBMIT.py')
       results += executer.ExecPresubmitScript(default_presubmit, fake_path)
     for filename in presubmit_files:
       filename = os.path.abspath(filename)
       if verbose:
         output.write("Running %s\n" % filename)
       # Accept CRLF presubmit script.
@@ skipping 204 matching lines @@
                  "can be passed to this program.")
   if options.rietveld_email_file:
     with open(options.rietveld_email_file, "rb") as f:
       options.rietveld_email = f.read().strip()
 
   change_class, files = load_files(options, args)
   if not change_class:
     parser.error('For unversioned directory, <files> is not optional.')
   logging.info('Found %d file(s).' % len(files))
 
-  rietveld_obj = None
+  rietveld_obj, gerrit_obj = None, None
+
   if options.rietveld_url:
     # The empty password is permitted: '' is not None.
     if options.rietveld_private_key_file:
       rietveld_obj = rietveld.JwtOAuth2Rietveld(
         options.rietveld_url,
         options.rietveld_email,
         options.rietveld_private_key_file)
     else:
       rietveld_obj = rietveld.CachingRietveld(
         options.rietveld_url,
         auth_config,
         options.rietveld_email)
     if options.rietveld_fetch:
       assert options.issue
       props = rietveld_obj.get_issue_properties(options.issue, False)
       options.author = props['owner_email']
       options.description = props['description']
       logging.info('Got author: "%s"', options.author)
       logging.info('Got description: """\n%s\n"""', options.description)
 
   if options.gerrit_url and options.gerrit_fetch:
+    assert options.issue and options.patchset
     rietveld_obj = None

[Michael Achenbach, 2016/04/28 07:17:29]

So, a hybrid is not supported? :)

E.g. a CL that's uploaded to rietveld and gerrit and presubmit kicks in for
both?

Never mind this comment.

[tandrii(chromium), 2016/04/28 19:31:50]

Well, just call it twice with two sets of args :)
too late :D

-    assert options.issue and options.patchset
-    props = gerrit_util.GetChangeDetail(
-        urlparse.urlparse(options.gerrit_url).netloc, str(options.issue),
-        ['ALL_REVISIONS'])
-    options.author = props['owner']['email']
-    for rev, rev_info in props['revisions'].iteritems():
-      if str(rev_info['_number']) == str(options.patchset):
-        options.description = gerrit_util.GetChangeDescriptionFromGitiles(
-            rev_info['fetch']['http']['url'], rev)
-        break
-    else:
-      print >> sys.stderr, ('Patchset %d was not found in Gerrit issue %d' %
-                            options.patchset, options.issue)
-      return 2
+    gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc)
+    options.author = gerrit_obj.GetChangeOwner(options.issue)
+    options.description = gerrit_obj.GetChangeDescription(options.patchset)
     logging.info('Got author: "%s"', options.author)
     logging.info('Got description: """\n%s\n"""', options.description)
 
   try:
     with canned_check_filter(options.skip_canned):
       results = DoPresubmitChecks(
           change_class(options.name,
                        options.description,
                        options.root,
                        files,
                        options.issue,
                        options.patchset,
                        options.author,
                        upstream=options.upstream),
           options.commit,
           options.verbose,
           sys.stdout,
           sys.stdin,
           options.default_presubmit,
           options.may_prompt,
           rietveld_obj,
-          options.dry_run)
+          options.dry_run,
+          gerrit_obj)
     return not results.should_continue()
   except NonexistantCannedCheckFilter, e:
     print >> sys.stderr, (
       'Attempted to skip nonexistent canned presubmit check: %s' % e.message)
     return 2
   except PresubmitFailure, e:
     print >> sys.stderr, e
     print >> sys.stderr, 'Maybe your depot_tools is out of date?'
     print >> sys.stderr, 'If all fails, contact maruel@'
     return 2
 
 
 if __name__ == '__main__':
   fix_encoding.fix_encoding()
   try:
     sys.exit(main())
   except KeyboardInterrupt:
     sys.stderr.write('interrupted\n')
     sys.exit(1)