Refactoring: Extract helper functions from CMDtry in git_cl.py.

git_cl.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.
 
 # Copyright (C) 2008 Evan Martin <martine@danga.com>
 
 """A git-command for integrating reviews on Rietveld and Gerrit."""
 
 from __future__ import print_function
@@ skipping 321 matching lines @@
     if response.status < 500 or try_count >= 2:
       raise httplib2.HttpLib2Error(content)
 
     # status >= 500 means transient failures.
     logging.debug('Transient errors when %s. Will retry.', operation_name)
     time_sleep(0.5 + 1.5*try_count)
     try_count += 1
   assert False, 'unreachable'
 
 
+def _get_bucket_map(changelist, options, option_parser):
+  """Returns a dict mapping bucket names (or master names) to
+  builders and tests, for triggering try jobs.
+  """
+  if not options.bot:
+    change = changelist.GetChange(
+        changelist.GetCommonAncestorWithUpstream(), None)
+
+    # Get try masters from PRESUBMIT.py files.
+    masters = presubmit_support.DoGetTryMasters(
+        change=change,
+        changed_files=change.LocalPaths(),
+        repository_root=settings.GetRoot(),
+        default_presubmit=None,
+        project=None,
+        verbose=options.verbose,
+        output_stream=sys.stdout)
+
+    if masters:
+      return masters
+
+    # Fall back to deprecated method: get try slaves from PRESUBMIT.py

[tandrii(chromium), 2016/10/23 16:55:55]

we should kill this, it has lived for too long, but since this is refactoring,
can you just add TODO for now?

[qyearsley, 2016/10/24 19:09:52]

Sounds good!

+    # files.
+    options.bot = presubmit_support.DoGetTrySlaves(
+        change=change,
+        changed_files=change.LocalPaths(),
+        repository_root=settings.GetRoot(),
+        default_presubmit=None,
+        project=None,
+        verbose=options.verbose,
+        output_stream=sys.stdout)
+
+    if not options.bot:
+      return {}
+
+  if options.bucket:
+    return {options.bucket: {b: [] for b in options.bot}}
+
+  builders_and_tests = {}
+
+  # TODO(machenbach): The old style command-line options don't support
+  # multiple try masters yet.
+  old_style = filter(lambda x: isinstance(x, basestring), options.bot)
+  new_style = filter(lambda x: isinstance(x, tuple), options.bot)
+
+  for bot in old_style:
+    if ':' in bot:
+      option_parser.error('Specifying testfilter is no longer supported')
+    elif ',' in bot:
+      option_parser.error('Specify one bot per --bot flag')
+    else:
+      builders_and_tests.setdefault(bot, [])
+
+  for bot, tests in new_style:
+    builders_and_tests.setdefault(bot, []).extend(tests)
+
+  if not options.master:
+    # TODO(crbug.com/640740): git cl try should be able to trigger

[tandrii(chromium), 2016/10/23 16:55:55]

lol :) I think styleguide mandates TODO(qyearsley): bugurl ...

that said, i completely agree it's a bug.

[qyearsley, 2016/10/24 19:09:51]

Ah, makes sense; done.

+    # builders on multiple masters if no master is given.
+    options.master, error_message = _get_builder_master(options.bot)
+    if error_message:
+      option_parser.error(
+          'Tryserver master cannot be found because: %s\n'
+          'Please manually specify the tryserver master, e.g. '
+          '"-m tryserver.chromium.linux".' % error_message)

[qyearsley, 2016/10/22 19:14:31]

Note: This block is moved from lines 4810-4815 and the condition is changed to
just `if not options.master` because at this point in the code, given the
conditions above, we can assume that --bot is given and --bucket is not given.

+
+  # Return a master map with one master to be backwards compatible. The
+  # master name defaults to an empty string, which will cause the master
+  # not to be set on rietveld (deprecated).
+  bucket = ''
+  if options.master:
+    # Add the "master." prefix to the master name to obtain the bucket name.
+    bucket = _prefix_master(options.master)
+  return {bucket: builders_and_tests}
+
+
+def _get_builder_master(bot_list):
+  """Fetches a master for the given list of builders.
+
+  Returns a pair (master, error_message), where either master or
+  error_message is None.
+  """
+  map_url = 'https://builders-map.appspot.com/'
+  try:
+    master_map = json.load(urllib2.urlopen(map_url))
+  except urllib2.URLError as e:
+    return None, ('Failed to fetch builder-to-master map from %s. Error: %s.' %
+                  (map_url, e))
+  except ValueError as e:
+    return None, ('Invalid json string from %s. Error: %s.' % (map_url, e))
+  if not master_map:
+    return None, 'Failed to build master map.'
+
+  result_master = ''
+  for bot in bot_list:
+    builder = bot.split(':', 1)[0]
+    master_list = master_map.get(builder, [])
+    if not master_list:
+      return None, ('No matching master for builder %s.' % builder)
+    elif len(master_list) > 1:
+      return None, ('The builder name %s exists in multiple masters %s.' %
+                    (builder, master_list))
+    else:
+      cur_master = master_list[0]
+      if not result_master:
+        result_master = cur_master
+      elif result_master != cur_master:
+        return None, 'The builders do not belong to the same master.'
+  return result_master, None

[qyearsley, 2016/10/22 19:14:31]

This function is moved from below and renamed; in another CL, I'd like to change
this to return a dict mapping masters to builders.

[tandrii(chromium), 2016/10/23 16:55:55]

SGTM. +1 for "_" name.

+
+
 def _trigger_try_jobs(auth_config, changelist, buckets, options,
                       category='git_cl_try', patchset=None):
+  """Sends a request to Buildbucket to trigger try jobs for a changelist.
+
+  Args:
+    auth_config: AuthConfig for Rietveld.
+    changelist: Changelist that the try jobs are associated with.
+    buckets: A nested dict mapping bucket names to builders to tests.
+    options: Command-line options.
+  """
   assert changelist.GetIssue(), 'CL must be uploaded first'
   codereview_url = changelist.GetCodereviewServer()
   assert codereview_url, 'CL must be uploaded first'
   patchset = patchset or changelist.GetMostRecentPatchset()
   assert patchset, 'CL must be uploaded first'
 
   codereview_host = urlparse.urlparse(codereview_url).hostname
   authenticator = auth.get_authenticator_for_host(codereview_host, auth_config)
   http = authenticator.authorize(httplib2.Http())
   http.force_exception_to_status_code = True
@@ skipping 73 matching lines @@
       buildbucket_put_url,
       'PUT',
       body=json.dumps(batch_req_body),
       headers={'Content-Type': 'application/json'}
   )
   print_text.append('To see results here, run:        git cl try-results')
   print_text.append('To see results in browser, run:  git cl web')
   print('\n'.join(print_text))
 
 
+def _trigger_dry_run(changelist):
+  """Triggers a dry run and prints a warning on failure."""
+  try:
+    changelist.SetCQState(_CQState.DRY_RUN)
+    print('scheduled CQ Dry Run on %s' % changelist.GetIssueURL())
+    return 0
+  except KeyboardInterrupt:
+    raise
+  except:
+    print('WARNING: failed to trigger CQ Dry Run.\n'
+          'Either:\n'
+          ' * your project has no CQ\n'
+          ' * you don\'t have permission to trigger Dry Run\n'
+          ' * bug in this code (see stack trace below).\n'
+          'Consider specifying which bots to trigger manually '
+          'or asking your project owners for permissions '
+          'or contacting Chrome Infrastructure team at '
+          'https://www.chromium.org/infra\n\n')
+    # Still raise exception so that stack trace is printed.
+    raise

[qyearsley, 2016/10/22 19:14:31]

This block is extracted from below; potentially, it could be reused in other
places here, or this change could be removed from this CL (and this block could
be put back in CMDtry).

[tandrii(chromium), 2016/10/23 16:55:55]

SGTM, how about making it method of ChangeList itself? 
It'd make sense later to replace SetCQState() in CMDset_commit && CMDUpload to
make use of this function, maybe by refactoring it to be dry-run independent.

[qyearsley, 2016/10/24 19:09:51]

Done, and added TODO note to make use of this function in either place (or use
SetCQState here and change SetCQState to possibly print messages and catch
errors).

+
+
 def fetch_try_jobs(auth_config, changelist, buildbucket_host,
                    patchset=None):
   """Fetches try jobs from buildbucket.
 
   Returns a map from build id to build info as a dictionary.
   """
   assert buildbucket_host
   assert changelist.GetIssue(), 'CL must be uploaded first'
   assert changelist.GetCodereviewServer(), 'CL must be uploaded first'
   patchset = patchset or changelist.GetMostRecentPatchset()
@@ skipping 4229 matching lines @@
   """Fetches the tree status from a json url and returns the message
   with the reason for the tree to be opened or closed."""
   url = settings.GetTreeStatusUrl()
   json_url = urlparse.urljoin(url, '/current?format=json')
   connection = urllib2.urlopen(json_url)
   status = json.loads(connection.read())
   connection.close()
   return status['message']
 
 
-def GetBuilderMaster(bot_list):
-  """For a given builder, fetch the master from AE if available."""
-  map_url = 'https://builders-map.appspot.com/'
-  try:
-    master_map = json.load(urllib2.urlopen(map_url))
-  except urllib2.URLError as e:
-    return None, ('Failed to fetch builder-to-master map from %s. Error: %s.' %
-                  (map_url, e))
-  except ValueError as e:
-    return None, ('Invalid json string from %s. Error: %s.' % (map_url, e))
-  if not master_map:
-    return None, 'Failed to build master map.'
-
-  result_master = ''
-  for bot in bot_list:
-    builder = bot.split(':', 1)[0]
-    master_list = master_map.get(builder, [])
-    if not master_list:
-      return None, ('No matching master for builder %s.' % builder)
-    elif len(master_list) > 1:
-      return None, ('The builder name %s exists in multiple masters %s.' %
-                    (builder, master_list))
-    else:
-      cur_master = master_list[0]
-      if not result_master:
-        result_master = cur_master
-      elif result_master != cur_master:
-        return None, 'The builders do not belong to the same master.'
-  return result_master, None
-
-
 def CMDtree(parser, args):
   """Shows the status of the tree."""
   _, args = parser.parse_args(args)
   status = GetTreeStatus()
   if 'unset' == status:
     print('You must configure your tree status URL by running "git cl config".')
     return 2
 
   print('The tree is %s' % status)
   print()
   print(GetTreeStatusReason())
   if status != 'open':
     return 1
   return 0
 
 
 def CMDtry(parser, args):
-  """Triggers try jobs using CQ dry run or BuildBucket for individual builders.
-  """
+  """Triggers try jobs using either BuildBucket or CQ dry run."""
   group = optparse.OptionGroup(parser, 'Try job options')
   group.add_option(
       '-b', '--bot', action='append',
       help=('IMPORTANT: specify ONE builder per --bot flag. Use it multiple '
             'times to specify multiple builders. ex: '
             '"-b win_rel -b win_layout". See '
             'the try server waterfall for the builders name and the tests '
             'available.'))
   group.add_option(
       '-B', '--bucket', default='',
@@ skipping 54 matching lines @@
   error_message = cl.CannotTriggerTryJobReason()
   if error_message:
     parser.error('Can\'t trigger try jobs: %s')
 
   if not options.name:
     options.name = cl.GetBranch()
 
   if options.bucket and options.master:
     parser.error('Only one of --bucket and --master may be used.')
 
-  if options.bot and not options.master and not options.bucket:
-    options.master, err_msg = GetBuilderMaster(options.bot)
-    if err_msg:
-      parser.error('Tryserver master cannot be found because: %s\n'
-                   'Please manually specify the tryserver master'
-                   ', e.g. "-m tryserver.chromium.linux".' % err_msg)
+  buckets = _get_bucket_map(cl, options, parser)
 
-  def GetMasterMap():
-    # Process --bot.
-    if not options.bot:
-      change = cl.GetChange(cl.GetCommonAncestorWithUpstream(), None)
-
-      # Get try masters from PRESUBMIT.py files.
-      masters = presubmit_support.DoGetTryMasters(
-          change,
-          change.LocalPaths(),
-          settings.GetRoot(),
-          None,
-          None,
-          options.verbose,
-          sys.stdout)
-      if masters:
-        return masters
-
-      # Fall back to deprecated method: get try slaves from PRESUBMIT.py files.
-      options.bot = presubmit_support.DoGetTrySlaves(
-          change,
-          change.LocalPaths(),
-          settings.GetRoot(),
-          None,
-          None,
-          options.verbose,
-          sys.stdout)
-
-    if not options.bot:
-      return {}
-
-    builders_and_tests = {}
-    # TODO(machenbach): The old style command-line options don't support
-    # multiple try masters yet.
-    old_style = filter(lambda x: isinstance(x, basestring), options.bot)
-    new_style = filter(lambda x: isinstance(x, tuple), options.bot)
-
-    for bot in old_style:
-      if ':' in bot:
-        parser.error('Specifying testfilter is no longer supported')
-      elif ',' in bot:
-        parser.error('Specify one bot per --bot flag')
-      else:
-        builders_and_tests.setdefault(bot, [])
-
-    for bot, tests in new_style:
-      builders_and_tests.setdefault(bot, []).extend(tests)
-
-    # Return a master map with one master to be backwards compatible. The
-    # master name defaults to an empty string, which will cause the master
-    # not to be set on rietveld (deprecated).
-    bucket = ''
-    if options.master:
-      # Add the "master." prefix to the master name to obtain the bucket name.
-      bucket = _prefix_master(options.master)
-    return {bucket: builders_and_tests}
-
-  if options.bucket:
-    buckets = {options.bucket: {b: [] for b in options.bot}}
-  else:
-    buckets = GetMasterMap()
-    if not buckets:
-      # Default to triggering Dry Run (see http://crbug.com/625697).
-      if options.verbose:
-        print('git cl try with no bots now defaults to CQ Dry Run.')
-      try:
-        cl.SetCQState(_CQState.DRY_RUN)
-        print('scheduled CQ Dry Run on %s' % cl.GetIssueURL())
-        return 0
-      except KeyboardInterrupt:
-        raise
-      except:
-        print('WARNING: failed to trigger CQ Dry Run.\n'
-              'Either:\n'
-              ' * your project has no CQ\n'
-              ' * you don\'t have permission to trigger Dry Run\n'
-              ' * bug in this code (see stack trace below).\n'
-              'Consider specifying which bots to trigger manually '
-              'or asking your project owners for permissions '
-              'or contacting Chrome Infrastructure team at '
-              'https://www.chromium.org/infra\n\n')
-        # Still raise exception so that stack trace is printed.
-        raise
+  if not buckets:
+    # Default to triggering Dry Run (see http://crbug.com/625697).
+    if options.verbose:
+      print('git cl try with no bots now defaults to CQ Dry Run.')
+    return _trigger_dry_run(cl)
 
   for builders in buckets.itervalues():
     if any('triggered' in b for b in builders):
       print('ERROR You are trying to send a job to a triggered bot. This type '
             'of bot requires an initial job from a parent (usually a builder). '
             'Instead send your job to the parent.\n'
             'Bot list: %s' % builders, file=sys.stderr)
       return 1
 
   patchset = cl.GetMostRecentPatchset()
   if patchset != cl.GetPatchset():
     print('Warning: Codereview server has newer patchsets (%s) than most '
           'recent upload from local checkout (%s). Did a previous upload '
           'fail?\n'
           'By default, git cl try uses the latest patchset from '
           'codereview, continuing to use patchset %s.\n' %
           (patchset, cl.GetPatchset(), patchset))
+
   try:
     _trigger_try_jobs(auth_config, cl, buckets, options, 'git_cl_try',
                       patchset)
   except BuildbucketResponseException as ex:
     print('ERROR: %s' % ex)
     return 1
   return 0
 
 
 def CMDtry_results(parser, args):
@@ skipping 488 matching lines @@
 if __name__ == '__main__':
   # These affect sys.stdout so do it outside of main() to simplify mocks in
   # unit testing.
   fix_encoding.fix_encoding()
   setup_color.init()
   try:
     sys.exit(main(sys.argv[1:]))
   except KeyboardInterrupt:
     sys.stderr.write('interrupted\n')
     sys.exit(1)