trychange.py: Gerrit protocol

trychange.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.
 
 """Client-side script to send a try job to the try server. It communicates to
 the try server by either writting to a svn/git repository or by directly
 connecting to the server by HTTP.
 """
 
 import contextlib
 import datetime
 import errno
 import getpass
 import itertools
 import json
 import logging
 import optparse
 import os
 import posixpath
 import re
 import shutil
 import sys
 import tempfile
 import urllib
 import urllib2
+import urlparse
 
 import breakpad  # pylint: disable=W0611
 
+import fix_encoding
 import gcl
-import fix_encoding
 import gclient_utils
+import gerrit_util
 import scm
 import subprocess2
 
 
 __version__ = '1.2'
 
 
 # Constants
 HELP_STRING = "Sorry, Tryserver is not available."
 USAGE = r"""%prog [options]
@@ skipping 45 matching lines @@
       DieWithError(
           'Command "%s" failed.\n%s' % (
             ' '.join(args), error_message or e.stdout or ''))
     return e.stdout
 
 
 def RunGit(args, **kwargs):
   """Returns stdout."""
   return RunCommand(['git'] + args, **kwargs)
 
+class Error(Exception):
+  """An error during a try job submission.
 
-class InvalidScript(Exception):
+  For this error, trychange.py does not display stack trace, only message
+  """
+
+class InvalidScript(Error):
   def __str__(self):
     return self.args[0] + '\n' + HELP_STRING
 
 
-class NoTryServerAccess(Exception):
+class NoTryServerAccess(Error):
   def __str__(self):
     return self.args[0] + '\n' + HELP_STRING
 
-
 def Escape(name):
   """Escapes characters that could interfere with the file system or try job
   parsing.
   """
   return re.sub(r'[^\w#-]', '_', name)
 
 
 class SCM(object):
   """Simplistic base class to implement one function: ProcessOptions."""
   def __init__(self, options, path, file_list):
@@ skipping 42 matching lines @@
     """Set default settings based on the gcl-style settings from the repository.
 
     The settings in the self.options object will only be set if no previous
     value exists (i.e. command line flags to the try command will override the
     settings in codereview.settings).
     """
     settings = {
       'port': self.GetCodeReviewSetting('TRYSERVER_HTTP_PORT'),
       'host': self.GetCodeReviewSetting('TRYSERVER_HTTP_HOST'),
       'svn_repo': self.GetCodeReviewSetting('TRYSERVER_SVN_URL'),
+      'gerrit_url': self.GetCodeReviewSetting('TRYSERVER_GERRIT_URL'),
       'git_repo': self.GetCodeReviewSetting('TRYSERVER_GIT_URL'),
       'project': self.GetCodeReviewSetting('TRYSERVER_PROJECT'),
       # Primarily for revision=auto
       'revision': self.GetCodeReviewSetting('TRYSERVER_REVISION'),
       'root': self.GetCodeReviewSetting('TRYSERVER_ROOT'),
       'patchlevel': self.GetCodeReviewSetting('TRYSERVER_PATCHLEVEL'),
     }
     logging.info('\n'.join(['%s: %s' % (k, v)
                             for (k, v) in settings.iteritems() if v]))
     for (k, v) in settings.iteritems():
@@ skipping 518 matching lines @@
           # Fetch, reset, update branch file again.
           patch_git('fetch', 'origin')
           patch_git('reset', '--hard', 'origin/master')
           update_branch()
     except subprocess2.CalledProcessError, e:
       # Restore state.
       patch_git('checkout', 'master')
       patch_git('reset', '--hard', 'origin/master')
       raise
 
+def _SendChangeGerrit(bot_spec, options):
+  """Posts a try job to a Gerrit change.
+
+  Reads Change-Id from the HEAD commit, resolves the current revision, checks
+  that local revision matches the uploaded one, posts a try job in form of a
+  message, sets Tryjob label to 1.
+
+  Gerrit message format: starts with !tryjob, optionally followed by a tryjob
+  definition in JSON format:
+      buildNames: list of strings specifying build names.
+  """
+
+  logging.info('Sending by Gerrit')
+  if not options.gerrit_url:
+    raise NoTryServerAccess('Please use --gerrit_url option to specify the '
+                            'Gerrit instance url to connect to')
+  gerrit_host = urlparse.urlparse(options.gerrit_url).hostname
+  logging.debug('Gerrit host: %s' % gerrit_host)
+
+  def get_change_id():

[Vadim Sh., 2014/04/30 00:40:48]

looks like this file is using GetChangeId naming style

+    """Finds Change-ID of the HEAD commit."""
+    CHANGE_ID_RGX = '^Change-Id: (I[a-f0-9]{10,})'
+    comment = scm.GIT.Capture(['log', '-1', '--format=%b'], cwd=os.getcwd())
+    change_id_match = re.search(CHANGE_ID_RGX, comment, re.I | re.M)
+    if not change_id_match:
+      raise Error('Change-Id was not found in the HEAD commit.')

[Vadim Sh., 2014/04/30 00:40:48]

It would be helpful to explain what to do in that case. For example explain that
this try mechanism works only Gerrit projects that have commit hooks in place. 

+    change_id = change_id_match.group(1)
+    assert change_id and change_id.startswith('I')

[Vadim Sh., 2014/04/30 00:40:48]

I think this assert is redundant since your regexp is pretty strict already.

+    return change_id
+
+  def format_message():
+    # Build job definition.
+    job_def = {}
+    builderNames = [builder for builder, _ in bot_spec]
+    if builderNames:
+      job_def['builderNames'] = builderNames
+
+    # Format message.
+    msg = '!tryjob'
+    if job_def:
+      msg = '%s %s' % (msg, json.dumps(job_def))

[Vadim Sh., 2014/04/30 00:40:48]

json.dumps(job_def, sort_keys=True)

+    return msg

[Vadim Sh., 2014/04/30 00:40:48]

Does Gerrit append '\n' for you here? I think it's better to explicitly append
it, since your regexp on buildbot side expects it.

[nodir, 2014/04/30 18:57:27]

The regexp expects EOL or EOF  after job definition. I've tested that:

re.search('!tryjob(.*)$', 'aaa\n!tryjob xxx', re.I).group() == '!tryjob xxx'

+
+  head_sha = scm.GIT.Capture(['log', '-1', '--format=%H'],  cwd=os.getcwd())
+
+  change_id = get_change_id()

[Vadim Sh., 2014/04/30 00:40:48]

I think it would be cleaner to pass |head_sha| to get_change_id() and extract
Change-Id from |head_sha| commit, rather then requery the history again.

[nodir, 2014/04/30 18:57:27]

Done.

+  assert change_id

[Vadim Sh., 2014/04/30 00:40:48]

I think this is redundant too..

+
+  # Check that the uploaded revision matches the local one.
+  changes = gerrit_util.GetChangeCurrentRevision(gerrit_host, change_id)
+  assert len(changes) in (0, 1), 'Multiple changes with id %s' % change_id

[Vadim Sh., 2014/04/30 00:40:48]

hm.. len(changes) <= 1?

[nodir, 2014/04/30 18:57:27]

Wanted to be more explicit, it's probably looks weird

+  if not changes:
+    raise Error('A change %s was not found on the server. Was it uploaded?' %
+                change_id)
+  logging.debug('Found Gerrit change: %s' % changes[0])
+  if changes[0]['current_revision'] != head_sha:
+    raise Error('Some changes were made since last upload to Gerrit, '

[Vadim Sh., 2014/04/30 00:40:48]

I'd rephrase it somehow. Maybe "Please reupload your latest changes to Gerrit"
or something like that. English is hard :(

You should also probably check that index and working directory are clean? I.e.
both 'git diff' and 'git diff --cached' return empty strings.

[nodir, 2014/04/30 18:57:27]

Modified the string a bit.
This is done by other part of trychange.py

+                'so stopping.')
+
+  # Post a try job.
+  message = format_message()
+  logging.info('Posting gerrit message: %s' % message)
+  if not options.dry_run:
+    # Post a message and set TryJob=1 label

[Vadim Sh., 2014/04/30 00:40:48]

nit: append '.' at the end of a sentence :)

+    gerrit_util.SetReview(gerrit_host, change_id, msg=message,
+                          labels={'Tryjob': 1})

[Vadim Sh., 2014/04/30 00:40:48]

What happens here if project doesn't have Tryjob label configured?

[nodir, 2014/04/30 18:57:27]

It will report:

Bad Request: label "Tryjb" is not a configured label

+
 
 def PrintSuccess(bot_spec, options):
   if not options.dry_run:
     text = 'Patch \'%s\' sent to try server' % options.name
     if bot_spec:
       text += ': %s' % ', '.join(
           '%s:%s' % (b[0], ','.join(b[1])) for b in bot_spec)
     print(text)
 
 
@@ skipping 194 matching lines @@
   group.add_option("--use_git",
                    action="store_const",
                    const=_SendChangeGit,
                    dest="send_patch",
                    help="Use GIT to talk to the try server")
   group.add_option("-G", "--git_repo",
                    metavar="GIT_URL",
                    help="GIT url to use to write the changes in; --use_git is "
                         "implied when using --git_repo")
   parser.add_option_group(group)
+
+  group = optparse.OptionGroup(parser, "Access the try server with Gerrit")
+  group.add_option("--use_gerrit",
+                   action="store_const",
+                   const=_SendChangeGerrit,
+                   dest="send_patch",
+                   help="Use Gerrit to talk to the try server")
+  group.add_option("--gerrit_url",
+                   metavar="GERRIT_URL",
+                   help="Gerrit url to post a tryjob to; --use_gerrit is "
+                        "implied when using --gerrit_url")
+  parser.add_option_group(group)
+
   return parser
 
 
 def TryChange(argv,
               change,
               swallow_exception,
               prog=None,
               extra_epilog=None):
   """
   Args:
@@ skipping 93 matching lines @@
                           os.path.join(current_vcs.checkout_root, item),
                           None)
       if checkout.checkout_root in [c.checkout_root for c in checkouts]:
         parser.error('Specified the root %s two times.' %
                      checkout.checkout_root)
       checkouts.append(checkout)
 
     can_http = options.port and options.host
     can_svn = options.svn_repo
     can_git = options.git_repo
+    can_gerrit = options.gerrit_url
+    can_anything = can_http or can_svn or can_git or can_gerrit

[Vadim Sh., 2014/04/30 00:40:48]

I'm far from english expert, but shouldn't it be "can_something" ? :)

[nodir, 2014/04/30 18:57:27]

Done

     # If there was no transport selected yet, now we must have enough data to
     # select one.
-    if not options.send_patch and not (can_http or can_svn or can_git):
+    if not options.send_patch and not can_anything:
       parser.error('Please specify an access method.')
 
     # Convert options.diff into the content of the diff.
     if options.url:
       if options.files:
         parser.error('You cannot specify files and --url at the same time.')
       options.diff = urllib2.urlopen(options.url).read()
     elif options.diff:
       if options.files:
         parser.error('You cannot specify files and --diff at the same time.')
@@ skipping 66 matching lines @@
 
     # Determine sending protocol
     if options.send_patch:
       # If forced.
       senders = [options.send_patch]
     else:
       # Try sending patch using avaialble protocols
       all_senders = [
         (_SendChangeHTTP, can_http),
         (_SendChangeSVN, can_svn),
-        (_SendChangeGit, can_git)
+        (_SendChangeGerrit, can_gerrit),
+        (_SendChangeGit, can_git),
       ]
       senders = [sender for sender, can in all_senders if can]
 
     # Send the patch.
     for sender in senders:
       try:
         sender(bot_spec, options)
         PrintSuccess(bot_spec, options)
         return 0
       except NoTryServerAccess:
         is_last = sender == senders[-1]
         if is_last:
           raise
     assert False, "Unreachable code"
-  except (InvalidScript, NoTryServerAccess), e:
+  except Error, e:
     if swallow_exception:
       return 1
     print >> sys.stderr, e
     return 1
   except (gclient_utils.Error, subprocess2.CalledProcessError), e:
     print >> sys.stderr, e
     return 1
   return 0
 
 
 if __name__ == "__main__":
   fix_encoding.fix_encoding()
   sys.exit(TryChange(None, None, False))