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

git_cl.py

Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index 09132a1b5be5a61cff2c5e03af977066cd640a7d..f27e04a18ddc6ce233fb8d4f99ee268676e29740 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -363,6 +363,7 @@ def _get_bucket_map(changelist, options, option_parser):
 
     # 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(),
@@ -378,6 +379,18 @@ def _get_bucket_map(changelist, options, option_parser):
   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
@@ -396,58 +409,39 @@ def _get_bucket_map(changelist, options, option_parser):
   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,