Make the try slave selection client side.

BUG=23071
TEST=none
Review URL: http://codereview.chromium.org/248029

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@28155 0039d316-1c4b-4281-b951-d872f2087c98

git-try

@@ -72,8 +72,9 @@ def GetSubRepWorkingDir(sub_rep_path):
 
 def GetMungedDiff(branch, prefix, sub_rep_path):
   """Get the diff we'll send to the try server.  We munge paths to match svn.
-     We add the prefix that the try bot is expecting. If sub_rep_path is 
-     provided, diff will be calculated in the sub repository."""
+     We add the prefix that the try bot is expecting. If sub_rep_path is
+     provided, diff will be calculated in the sub repository.
+     We also return the list of files in this diff, without munged paths."""
   # Make the following changes:
   # - Prepend "src/" (or some other prefix) to paths as svn is expecting
   # - In the case of added files, replace /dev/null with the path to the file
@@ -96,7 +97,7 @@ def GetMungedDiff(branch, prefix, sub_rep_path):
     # Add the right prefix
     command.extend(['--src-prefix=' + sub_rep_path])
     command.extend(['--dst-prefix=' + sub_rep_path])
-    
+
   command.extend([branch, 'HEAD'])
 
   # Run diff tree
@@ -113,23 +114,28 @@ def GetMungedDiff(branch, prefix, sub_rep_path):
 
   # Add root prefix
   output = []
+  file_set = set()
   for line in diff:
     if line.startswith('--- ') or line.startswith('+++ '):
-      line = line[0:4] + os.path.join(prefix,line[4:])
+      filename = line[4:]
+      line = line[0:4] + os.path.join(prefix, filename)
+      file_set.add(filename.rstrip('\r\n'))
     output.append(line)
 
   munged_diff = ''.join(output)
   if len(munged_diff.strip()) == 0:
     raise Exception("Patch was empty, did you give the right remote branch?")
 
-  return munged_diff
+  return (munged_diff, list(file_set))
 
 def OneRepositoryDiff(diff_file, patch_names, branch, prefix, sub_rep_path):
   """Computes a diff for one git repository at a given path against a given
      branch. Writes the diff into diff_file and appends a name to the
-     patch_names list."""
-  
-  diff = GetMungedDiff(branch, prefix, sub_rep_path)
+     patch_names list.
+
+     Returns the list of files in the diff."""
+
+  (diff, file_list) = GetMungedDiff(branch, prefix, sub_rep_path)
 
   # Write the diff out
   diff_file.write(diff)
@@ -137,6 +143,7 @@ def OneRepositoryDiff(diff_file, patch_names, branch, prefix, sub_rep_path):
   # Add patch name to list of patches
   patch_name = GetPatchName(GetSubRepWorkingDir(sub_rep_path))
   patch_names.extend([patch_name])
+  return file_list
 
 
 def ValidEmail(email):
@@ -150,11 +157,9 @@ def GetEmail():
   return email
 
 
-def TryChange(args):
+def TryChange(args, file_list):
   """Put a patch on the try server."""
-  root_dir = Backquote(['git', 'rev-parse', '--show-cdup'])
-  trychange.checkout_root = os.path.abspath(root_dir)
-  trychange.TryChange(args, None, False)
+  trychange.TryChange(args, file_list, False)
 
 
 if __name__ == '__main__':
@@ -169,12 +174,12 @@ if __name__ == '__main__':
   parser.add_option("-r", "--revision",
                     help="Specify the SVN base revision to use")
   parser.add_option("--root", default="src", metavar="PATH",
-                    help="Specify the root prefix that is appended to paths "
+                    help="Specify the root prefix that is prepended to paths "
                          "in the patch")
   parser.add_option("--dry_run", action="store_true",
                     help="Print the diff but don't send it to the try bots")
-  parser.add_option("--sub_rep", nargs=2, action="append", default=[], 
-                    metavar="PATH BRANCH", 
+  parser.add_option("--sub_rep", nargs=2, action="append", default=[],
+                    metavar="PATH BRANCH",
                     help="Specify a path to a git sub-repository and a branch "
                          "to diff with in order to simultanously try changes "
                          "in multiple git repositories. Option may be "
@@ -182,7 +187,7 @@ if __name__ == '__main__':
   parser.add_option("--webkit", metavar="BRANCH",
                     help="Specify webkit branch. Syntactic sugar for "
                          "--sub_rep third_party/WebKit/ <branch>")
-                         
+
   (options, args) = parser.parse_args(sys.argv)
 
   if options.webkit:
@@ -195,17 +200,18 @@ if __name__ == '__main__':
 
   # Dump all diffs into one diff file.
   diff_file = tempfile.NamedTemporaryFile()
-  
+
   # Calculate diff for main git repository.
-  OneRepositoryDiff(diff_file, patch_names, branch, options.root, None)
-  
+  file_list = OneRepositoryDiff(diff_file, patch_names, branch, options.root,
+                                None)
+
   # Calculate diff for each extra git repository.
   for path_and_branch in options.sub_rep:
-    OneRepositoryDiff(diff_file,
-                      patch_names,
-                      path_and_branch[1],
-                      options.root,
-                      path_and_branch[0]) 
+    file_list.extend(OneRepositoryDiff(diff_file,
+                                      patch_names,
+                                      path_and_branch[1],
+                                      options.root,
+                                      path_and_branch[0]))
   # Make diff file ready for reading.
   diff_file.flush()
 
@@ -217,7 +223,7 @@ if __name__ == '__main__':
       '-u', user,
       '-e', email,
       '-n', '-'.join(patch_names),
-      '--diff', diff_file.name, 
+      '--diff', diff_file.name,
   ]
 
   # Send to try server via HTTP if we can parse the config, otherwise
@@ -257,4 +263,4 @@ if __name__ == '__main__':
     exit(0)
 
   print sendmsg
-  TryChange(args=args)
+  TryChange(args, file_list)

presubmit_support.py

@@ -6,7 +6,7 @@
 """Enables directory-specific presubmit checks to run at upload and/or commit.
 """
 
-__version__ = '1.3.2'
+__version__ = '1.3.3'
 
 # TODO(joi) Add caching where appropriate/needed. The API is designed to allow
 # caching (between all different invocations of presubmit scripts for a given
@@ -744,6 +744,78 @@ def ListRelevantPresubmitFiles(files, root):
   return filter(lambda x: os.path.isfile(x), entries)
 
 
+class GetTrySlavesExecuter(object):
+  def ExecPresubmitScript(self, script_text):
+    """Executes GetPreferredTrySlaves() from a single presubmit script.
+
+    Args:
+      script_text: The text of the presubmit script.
+
+    Return:
+      A list of try slaves.
+    """
+    context = {}
+    exec script_text in context
+
+    function_name = 'GetPreferredTrySlaves'
+    if function_name in context:
+      result = eval(function_name + '()', context)
+      if not isinstance(result, types.ListType):
+        raise exceptions.RuntimeError(
+            'Presubmit functions must return a list, got a %s instead: %s' %
+            (type(result), str(result)))
+      for item in result:
+        if not isinstance(item, basestring):
+          raise exceptions.RuntimeError('All try slaves names must be strings.')
+        if item != item.strip():
+          raise exceptions.RuntimeError('Try slave names cannot start/end'
+                                        'with whitespace')
+    else:
+      result = []
+    return result
+
+
+def DoGetTrySlaves(changed_files,
+                   repository_root,
+                   default_presubmit,
+                   verbose,
+                   output_stream):
+  """Get the list of try servers 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.
+    verbose: Prints debug info.
+    output_stream: A stream to write debug output to.
+
+  Return:
+    List of try slaves
+  """
+  presubmit_files = ListRelevantPresubmitFiles(changed_files, repository_root)
+  if not presubmit_files and verbose:
+    output_stream.write("Warning, no presubmit.py found.\n")
+  results = []
+  executer = GetTrySlavesExecuter()
+  if default_presubmit:
+    if verbose:
+      output_stream.write("Running default presubmit script.\n")
+    results += executer.ExecPresubmitScript(default_presubmit)
+  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 = gcl.ReadFile(filename, 'rU')
+    results += executer.ExecPresubmitScript(presubmit_script)
+
+  slaves = list(set(results))
+  if slaves and verbose:
+    output_stream.write(', '.join(slaves))
+    output_stream.write('\n')
+  return slaves
+
+
 class PresubmitExecuter(object):
   def __init__(self, change, committing):
     """

tests/presubmit_unittest.py

@@ -31,6 +31,10 @@ def CheckChangeOnUpload(input_api, output_api):
   else:
     return ()
 """
+  presubmit_tryslave = """
+def GetPreferredTrySlaves():
+  return %s
+"""
 
   def setUp(self):
     super_mox.SuperMoxTestBase.setUp(self)
@@ -73,8 +77,8 @@ class PresubmitUnittest(PresubmitTestsBase):
   def testMembersChanged(self):
     self.mox.ReplayAll()
     members = [
-      'AffectedFile', 'Change', 'DoPresubmitChecks', 'GitChange',
-      'GitAffectedFile', 'InputApi',
+      'AffectedFile', 'Change', 'DoGetTrySlaves', 'DoPresubmitChecks',
+      'GetTrySlavesExecuter', 'GitChange', 'GitAffectedFile', 'InputApi',
       'ListRelevantPresubmitFiles', 'Main', 'NotImplementedException',
       'OutputApi', 'ParseFiles', 'PresubmitExecuter', 'ScanSubDirs',
       'SvnAffectedFile', 'SvnChange',
@@ -484,6 +488,60 @@ def CheckChangeOnCommit(input_api, output_api):
                        '** Presubmit Messages **\n'
                        'http://tracker.com/42\n\n'))
 
+  def testGetTrySlavesExecuter(self):
+    self.mox.ReplayAll()
+
+    executer = presubmit.GetTrySlavesExecuter()
+    self.assertEqual([], executer.ExecPresubmitScript(''))
+    self.assertEqual([], executer.ExecPresubmitScript('def foo():\n  return\n'))
+
+    # bad results
+    starts_with_space_result = ['  starts_with_space']
+    not_list_result1 = "'foo'"
+    not_list_result2 = "('a', 'tuple')"
+    for result in starts_with_space_result, not_list_result1, not_list_result2:
+      self.assertRaises(exceptions.RuntimeError,
+                        executer.ExecPresubmitScript,
+                        self.presubmit_tryslave % result)
+
+    # good results
+    expected_result = ['1', '2', '3']
+    empty_result = []
+    space_in_name_result = ['foo bar', '1\t2 3']
+    for result in expected_result, empty_result, space_in_name_result:
+      self.assertEqual(result,
+                       executer.ExecPresubmitScript(self.presubmit_tryslave %
+                                                    str(result)))
+
+  def testDoGetTrySlaves(self):
+    join = presubmit.os.path.join
+    filename = 'foo.cc'
+    filename_linux = join('linux_only', 'penguin.cc')
+    root_presubmit = join(self.fake_root_dir, 'PRESUBMIT.py')
+    linux_presubmit = join(self.fake_root_dir, 'linux_only', 'PRESUBMIT.py')
+
+    presubmit.os.path.isfile(root_presubmit).AndReturn(True)
+    presubmit.gcl.ReadFile(root_presubmit, 'rU').AndReturn(
+        self.presubmit_tryslave % '["win"]')
+
+    presubmit.os.path.isfile(root_presubmit).AndReturn(True)
+    presubmit.os.path.isfile(linux_presubmit).AndReturn(True)
+    presubmit.gcl.ReadFile(root_presubmit, 'rU').AndReturn(
+        self.presubmit_tryslave % '["win"]')
+    presubmit.gcl.ReadFile(linux_presubmit, 'rU').AndReturn(
+        self.presubmit_tryslave % '["linux"]')
+    self.mox.ReplayAll()
+
+    output = StringIO.StringIO()
+    self.assertEqual(['win'],
+                     presubmit.DoGetTrySlaves([filename], self.fake_root_dir,
+                                              None, False, output))
+    output = StringIO.StringIO()
+    self.assertEqual(['win', 'linux'],
+                     presubmit.DoGetTrySlaves([filename, filename_linux],
+                                              self.fake_root_dir, None, False,
+                                              output))
+
   def testMain(self):
     self.mox.StubOutWithMock(presubmit, 'DoPresubmitChecks')
     self.mox.StubOutWithMock(presubmit, 'ParseFiles')

tests/trychange_unittest.py

@@ -5,11 +5,14 @@
 
 """Unit tests for trychange.py."""
 
+import optparse
 import unittest
 
 # Local imports
+import gcl
 import super_mox
 import trychange
+import upload
 from super_mox import mox
 
 
@@ -27,8 +30,8 @@ class TryChangeUnittest(TryChangeTestsBase):
       'HELP_STRING', 'InvalidScript', 'NoTryServerAccess', 'PathDifference',
       'RunCommand', 'SCM', 'SVN', 'TryChange', 'USAGE',
       'datetime', 'gcl', 'gclient', 'gclient_scm', 'getpass', 'logging',
-      'optparse', 'os', 'shutil', 'socket', 'sys', 'tempfile', 'traceback',
-      'upload', 'urllib', 'subprocess',
+      'optparse', 'os', 'presubmit_support', 'shutil', 'socket', 'subprocess',
+      'sys', 'tempfile', 'traceback', 'upload', 'urllib',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(trychange, members)
@@ -36,23 +39,62 @@ class TryChangeUnittest(TryChangeTestsBase):
 
 class SVNUnittest(TryChangeTestsBase):
   """trychange.SVN tests."""
+  def setUp(self):
+    self.fake_root = '/fake_root'
+    self.expected_files = ['foo.txt', 'bar.txt']
+    change_info = gcl.ChangeInfo('test_change', 0, 0, 'desc',
+                                 [('M', f) for f in self.expected_files],
+                                 self.fake_root)
+    self.svn = trychange.SVN(None)
+    self.svn.change_info = change_info
+    super_mox.SuperMoxTestBase.setUp(self)
+
   def testMembersChanged(self):
     members = [
-      'GenerateDiff', 'ProcessOptions', 'options'
+      'GenerateDiff', 'GetFileNames', 'GetLocalRoot', 'ProcessOptions',
+      'options'
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(trychange.SVN(None), members)
 
+  def testGetFileNames(self):
+    self.mox.ReplayAll()
+    self.assertEqual(self.svn.GetFileNames(), self.expected_files)
+
+  def testGetLocalRoot(self):
+    self.mox.ReplayAll()
+    self.assertEqual(self.svn.GetLocalRoot(), self.fake_root)
+
 
 class GITUnittest(TryChangeTestsBase):
   """trychange.GIT tests."""
+  def setUp(self):
+    self.fake_root = '/fake_root'
+    self.expected_files = ['foo.txt', 'bar.txt']
+    options = optparse.Values()
+    options.files = self.expected_files
+    self.git = trychange.GIT(options)
+    super_mox.SuperMoxTestBase.setUp(self)
+
   def testMembersChanged(self):
     members = [
-      'GenerateDiff', 'GetEmail', 'GetPatchName', 'ProcessOptions', 'options'
+      'GenerateDiff', 'GetEmail', 'GetFileNames', 'GetLocalRoot',
+      'GetPatchName', 'ProcessOptions', 'options'
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(trychange.GIT(None), members)
 
+  def testGetFileNames(self):
+    self.mox.ReplayAll()
+    self.assertEqual(self.git.GetFileNames(), self.expected_files)
+
+  def testGetLocalRoot(self):
+    self.mox.StubOutWithMock(upload, 'RunShell')
+    upload.RunShell(['git', 'rev-parse', '--show-cdup']).AndReturn(
+        self.fake_root)
+    self.mox.ReplayAll()
+    self.assertEqual(self.git.GetLocalRoot(), self.fake_root)
+
 
 if __name__ == '__main__':
   unittest.main()

trychange.py

@@ -24,6 +24,7 @@ import urllib
 import gcl
 import gclient
 import gclient_scm
+import presubmit_support
 import upload
 
 __version__ = '1.1.1'
@@ -195,6 +196,14 @@ class SVN(SCM):
     os.chdir(previous_cwd)
     return "".join(diff)
 
+  def GetFileNames(self):
+    """Return the list of files in the diff."""
+    return self.change_info.GetFileNames()
+
+  def GetLocalRoot(self):
+    """Return the path of the repository root."""
+    return self.change_info.GetLocalRoot()
+
   def ProcessOptions(self):
     if not self.options.diff:
       # Generate the diff with svn and write it to the submit queue path.  The
@@ -205,6 +214,8 @@ class SVN(SCM):
       prefix = PathDifference(source_root, gcl.GetRepositoryRoot())
       adjusted_paths = [os.path.join(prefix, x) for x in self.options.files]
       self.options.diff = self.GenerateDiff(adjusted_paths, root=source_root)
+      self.change_info = gcl.LoadChangelistInfoForMultiple(self.options.name,
+          gcl.GetRepositoryRoot(), True, True)
 
 
 class GIT(SCM):
@@ -225,6 +236,16 @@ class GIT(SCM):
     # TODO: check for errors here?
     return upload.RunShell(['git', 'config', 'user.email']).strip()
 
+  def GetFileNames(self):
+    """Return the list of files in the diff."""
+    return self.options.files
+
+  def GetLocalRoot(self):
+    """Return the path of the repository root."""
+    # TODO: check for errors here?
+    root = upload.RunShell(['git', 'rev-parse', '--show-cdup']).strip()
+    return os.path.abspath(root)
+
   def GetPatchName(self):
     """Construct a name for this patch."""
     # TODO: perhaps include the hash of the current commit, to distinguish
@@ -410,6 +431,12 @@ def TryChange(argv,
               file_list,
               swallow_exception,
               prog=None):
+  """
+  Args:
+    argv: Arguments and options.
+    file_list: Default value to pass to --file.
+    swallow_exception: Whether we raise or swallow exceptions.
+  """
   default_settings = GetTryServerSettings()
   transport_functions = { 'http': _SendChangeHTTP, 'svn': _SendChangeSVN }
   default_transport = transport_functions.get(
@@ -542,9 +569,19 @@ def TryChange(argv,
       if not options.diff:
         raise
 
+    # Get try slaves from PRESUBMIT.py files if not specified.
+    if not options.bot:
+      root_presubmit = gcl.GetCachedFile('PRESUBMIT.py', use_root=True)
+      options.bot = presubmit_support.DoGetTrySlaves(options.scm.GetFileNames(),
+                                                     options.scm.GetLocalRoot(),
+                                                     root_presubmit,
+                                                     False,
+                                                     sys.stdout)
+
     # Send the patch.
     patch_name = options.send_patch(options)
-    print 'Patch \'%s\' sent to try server.' % patch_name
+    print 'Patch \'%s\' sent to try server: %s' % (patch_name,
+                                                   ', '.join(options.bot))
     if patch_name == 'Unnamed':
       print "Note: use --name NAME to change the try's name."
   except (InvalidScript, NoTryServerAccess), e: