Support multiple try masters when sending tries to rietveld.

presubmit_support.py

Index: presubmit_support.py
diff --git a/presubmit_support.py b/presubmit_support.py
index dabfbf0bac2e7bbf253aaa29245fca5378ed58eb..f31afd3d5df175317fd985f0d4b145d42f10ac20 100755
--- a/presubmit_support.py
+++ b/presubmit_support.py
@@ -1007,6 +1007,8 @@ class GetTrySlavesExecuter(object):
   @staticmethod
   def ExecPresubmitScript(script_text, presubmit_path, project, change):
     """Executes GetPreferredTrySlaves() from a single presubmit script.
+    
+    This will soon be deprecated and replaced by GetPreferredTryMasters().
 
     Args:
       script_text: The text of the presubmit script.
@@ -1074,6 +1076,75 @@ class GetTrySlavesExecuter(object):
     return result
 
 
+class GetTryMastersExecuter(object):
+  @staticmethod
+  def ExecPresubmitScript(script_text, presubmit_path, project, change):
+    """Executes GetPreferredTryMasters() from a single presubmit script.
+
+    Args:
+      script_text: The text of the presubmit script.
+      presubmit_path: Project script to run.
+      project: Project name to pass to presubmit script for bot selection.
+
+    Return:
+      A map of try masters to (list of builders or map of builders to set of
+      tests).
+    """
+    context = {}
+    try:
+      exec script_text in context
+    except Exception, e:
+      raise PresubmitFailure('"%s" had an exception.\n%s'
+                             % (presubmit_path, e))
+
+    function_name = 'GetPreferredTryMasters'
+    if function_name not in context:
+      return {}
+    get_preferred_try_masters = context[function_name]
+    function_info = inspect.getargspec(get_preferred_try_masters)

[Paweł Hajdan Jr., 2014/02/27 02:20:24]

Since this is new, can we standardize on one signature of that function?

[Michael Achenbach, 2014/02/27 18:35:23]

Done.

+    if len(function_info[0]) == 1:
+      result = get_preferred_try_masters(project)
+    elif len(function_info[0]) == 2:
+      result = get_preferred_try_masters(project, change)
+    else:
+      result = get_preferred_try_masters()
+
+    def CheckType(item, t):

[Paweł Hajdan Jr., 2014/02/27 02:20:24]

It's not obvious to me how much these type checks in python are actually useful.
I'm fine with them, but please consider removing them.

[Michael Achenbach, 2014/02/27 18:35:23]

If presubmit hooks return the wrong formats, it'd first be detected deeper down
the code and not be so obvious anymore. Would be nice if there were a more
declarative way to express the expected format.

+      if not isinstance(item, t):
+        raise PresubmitFailure(
+            'Expected a %s, got a %s instead: %s' %
+            (str(t), type(item), str(item)))
+
+    def CheckString(item):
+      if not item:
+        raise PresubmitFailure(
+            'PRESUBMIT.py returned empty trymaster/builder string.')
+      CheckType(item, basestring)
+      if item != item.strip():
+        raise PresubmitFailure(
+            'Try master/builder names cannot start/end with whitespace')
+      if ',' in item:
+        raise PresubmitFailure(
+            'Do not use \',\' separated master, builder or test names: %s'
+            % item)
+
+    CheckType(result, dict)
+    for (master, builders) in result.iteritems():
+      CheckString(master)
+      if isinstance(builders, dict):

[Paweł Hajdan Jr., 2014/02/27 02:20:24]

Similarly, since this is new, can we standardize on specifying both builders and
tests? A dummy value for tests can always be specified.

[Michael Achenbach, 2014/02/27 18:35:23]

Done.

+        for (builder, tests) in builders.iteritems():
+          CheckString(builder)
+          CheckType(tests, set)
+      elif isinstance(builders, list):
+        for builder in builders:
+          CheckString(builder)
+      else:
+        raise PresubmitFailure(
+            'Expected a list, got a %s instead: %s' %
+            (type(builders), str(builders)))   
+    return result
+
+
 def DoGetTrySlaves(change,
                    changed_files,
                    repository_root,
@@ -1081,7 +1152,7 @@ def DoGetTrySlaves(change,
                    project,
                    verbose,
                    output_stream):
-  """Get the list of try servers from the presubmit scripts.
+  """Get the list of try servers from the presubmit scripts (deprecated).
 
   Args:
     changed_files: List of modified files.
@@ -1132,6 +1203,87 @@ def DoGetTrySlaves(change,
   return slaves
 
 
+def _ExpandBuilders(builders):
+  """Expands builders in list format to dict format."""
+  if isinstance(builders, list):

[Paweł Hajdan Jr., 2014/02/27 02:20:24]

Why do we need to make a decision based on type here?

[Michael Achenbach, 2014/02/27 18:35:23]

This is removed now, since now only dicts are allowed.

+    return dict((builder, set()) for builder in builders)
+  else:
+    return builders
+
+
+def _MergeMasters(results, masters):
+  """Merges masters into results.

[Paweł Hajdan Jr., 2014/02/27 02:20:24]

It's not obvious to me what this does, could you clarify or ideally simplify it?

I could spend more time trying to understand it, but ideally I shouldn't.

[Michael Achenbach, 2014/02/27 18:35:23]

I tried to simplify it and made it a function. The best would be to look at the
tests and check if they're complete.

+
+  Merges with side effects on the original results dict.
+  """
+  for (master, builders) in masters.iteritems():
+    if master in results:
+      result_builders = results[master]
+      for (builder, tests) in _ExpandBuilders(builders).iteritems():
+        if builder in result_builders:
+          for test in tests:
+            result_builders.add(test)
+        else:
+          result_builders[builder] = tests
+    else:
+      results[master] = _ExpandBuilders(builders)
+  return results
+
+
+def DoGetTryMasters(change,
+                    changed_files,
+                    repository_root,
+                    default_presubmit,
+                    project,
+                    verbose,
+                    output_stream):
+  """Get the list of try masters from the presubmit scripts.
+
+  Args:
+    changed_files: List of modified files.
+    repository_root: The repository root.
+    default_presubmit: A default presubmit script to execute in any case.
+    project: Optional name of a project used in selecting trybots.
+    verbose: Prints debug info.
+    output_stream: A stream to write debug output to.
+
+  Return:
+    Map of try masters to map of builders to set of tests.
+  """
+  presubmit_files = ListRelevantPresubmitFiles(changed_files, repository_root)
+  if not presubmit_files and verbose:
+    output_stream.write("Warning, no presubmit.py found.\n")
+  results = {}
+  executer = GetTryMastersExecuter()
+
+  if default_presubmit:
+    if verbose:
+      output_stream.write("Running default presubmit script.\n")
+    fake_path = os.path.join(repository_root, 'PRESUBMIT.py')
+    results = _MergeMasters(results, executer.ExecPresubmitScript(
+        default_presubmit, fake_path, project, change))
+  for filename in presubmit_files:
+    filename = os.path.abspath(filename)
+    if verbose:
+      output_stream.write("Running %s\n" % filename)
+    # Accept CRLF presubmit script.
+    presubmit_script = gclient_utils.FileRead(filename, 'rU')
+    results = _MergeMasters(results, executer.ExecPresubmitScript(
+        presubmit_script, filename, project, change))
+
+  # Make sets to lists again for later JSON serialization. Add
+  # defaulttests where tests are missing.
+  for (_, builders) in results.iteritems():
+    for builder in builders:
+      builders[builder] = list(builders[builder])
+      if not builders[builder]:
+        builders[builder].append('defaulttests')
+
+  if results and verbose:
+    output_stream.write('%s\n' % str(results))
+  return results
+
+
 class PresubmitExecuter(object):
   def __init__(self, change, committing, rietveld_obj, verbose):
     """