git cl try: support multiple bots from different masters without specifying master.

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 345 matching lines @@
         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
     # files.
+    # TODO(qyearsley): Remove this.
     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}}
 
+  if not options.master:
+    master_map, error_message = _get_master_map_for_builders(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)
+    buckets = {}
+    for master, builders in master_map.iteritems():
+      buckets[_prefix_master(master)] = {b: [] for b in builders}

[Michael Achenbach, 2016/10/26 08:36:40]

How about inlining this in the _get_master_map_for_builders as the raw
master_map value isn't used for anything else?

Below you could use _prefix_master(master) as key in master_map and e.g.
directly write:

master_map.setdefault(_prefix_master(masters[0]), {})[builder] = []

Then you could just return master_map here.

[qyearsley, 2016/10/26 16:08:47]

Makes sense; done

+    return buckets
+
   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(qyearsley): crbug.com/640740
-    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)
-
-  # 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)
+  # 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.
-  """
+def _get_master_map_for_builders(builders):
+  """Returns a map of masters to builders for the given builders."""
   map_url = 'https://builders-map.appspot.com/'
   try:
-    master_map = json.load(urllib2.urlopen(map_url))
+    builders_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:
+  if not builders_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:
+  master_map = {}
+  for builder in builders:
+    builder = builder.split(':', 1)[0]
+    masters = builders_map.get(builder, [])
+    if not masters:
       return None, ('No matching master for builder %s.' % builder)
-    elif len(master_list) > 1:
+    elif len(masters) > 1:
       return None, ('The builder name %s exists in multiple masters %s.' %
-                    (builder, master_list))
+                    (builder, masters))
     else:

[Michael Achenbach, 2016/10/26 08:36:40]

nit: No else after returns required.

The code below could be written (or see other suggestion above):

master_map.setdefault(masters[0], []).append(builder)

[qyearsley, 2016/10/26 16:08:47]

Done; also, renamed "master_map" to "bucket_map" to reflect that the keys are
now bucket names (with the "master." prefix).

-      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
+      master = masters[0]
+      master_map.setdefault(master, [])
+      master_map[master].append(builder)
+
+  return master_map, None
 
 
 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.
@@ skipping 4949 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)