Use git cl try --properties option to send test arguments instead of run-perf-test.cfg

tools/perf/core/trybot_command.py

Index: tools/perf/core/trybot_command.py
diff --git a/tools/perf/core/trybot_command.py b/tools/perf/core/trybot_command.py
index 7d97fb52bb5b21ea088649783e34716cd7766bd4..e375354a3bafe9bfc78fc82b5775a42ee7f9d1f8 100644
--- a/tools/perf/core/trybot_command.py
+++ b/tools/perf/core/trybot_command.py
@@ -3,13 +3,14 @@
 # found in the LICENSE file.
 
 import argparse
-import os
+import json
 import logging
+import os
 import platform
 import re
 import subprocess
 import urllib2
-import json
+
 
 from core import path_util
 
@@ -20,9 +21,6 @@ from telemetry.util import command_line
 from telemetry.util import matching
 
 
-CHROMIUM_CONFIG_FILENAME = 'tools/run-perf-test.cfg'
-BLINK_CONFIG_FILENAME = 'Tools/run-perf-test.cfg'
-SUCCESS, NO_CHANGES, ERROR = range(3)
 # Unsupported Perf bisect bots.
 EXCLUDED_BOTS = {
     'win_xp_perf_bisect',  # Goma issues: crbug.com/330900
@@ -57,20 +55,23 @@ INCLUDE_BOTS = [
 ]
 
 # Default try bot to use incase builbot is unreachable.
-DEFAULT_TRYBOTS =  [
+DEFAULT_TRYBOTS = [
     'linux_perf_bisect',
     'mac_10_11_perf_bisect',
     'winx64_10_perf_bisect',
     'android_s5_perf_bisect',
 ]
 
-assert not set(DEFAULT_TRYBOTS) & set(EXCLUDED_BOTS),  ( 'A trybot cannot '
-    'present in both Default as well as Excluded bots lists.')
+CHROMIUM_SRC_PATH = path_util.GetChromiumSrcDir()
+
+assert not set(DEFAULT_TRYBOTS) & set(EXCLUDED_BOTS), (
+    'A trybot cannot present in both Default as well as Excluded bots lists.')
+
 
 class TrybotError(Exception):
 
   def __str__(self):
-    return '%s\nError running tryjob.' % self.args[0]
+    return '(ERROR) Perf Try Job: %s' % self.args[0]
 
 
 def _GetTrybotList(builders):
@@ -89,7 +90,7 @@ def _GetBotPlatformFromTrybotName(trybot_name):
 
 
 def _GetBuilderNames(trybot_name, builders):
-  """ Return platform and its available bot name as dictionary."""
+  """Return platform and its available bot name as dictionary."""
   os_names = ['linux', 'android', 'mac', 'win']
   if 'all' not in trybot_name:
     bot = ['%s_perf_bisect' % trybot_name.replace('-', '_')]
@@ -125,22 +126,42 @@ def _GetBuilderNames(trybot_name, builders):
   return platform_and_bots
 
 
-def _RunProcess(cmd):
-  logging.debug('Running process: "%s"', ' '.join(cmd))
-  proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-  out, err = proc.communicate()
-  returncode = proc.poll()
-  return (returncode, out, err)
+_GIT_CMD = 'git'
 
 
-_GIT_CMD = 'git'
 if platform.system() == 'Windows':
   # On windows, the git command is installed as 'git.bat'
   _GIT_CMD = 'git.bat'
 
 
+def RunGit(cmd, msg_on_error='', ignore_return_code=False):
+  """Runs the GIT command with the given arguments.

[eakuefner, 2016/08/30 17:33:49]

nit: s/GIT/git/

[prasadv, 2016/08/30 18:49:40]

Done.

+
+  Args:
+    cmd: GIT command arguments.
+    msg_on_error: Message to be displayed on GIT command error.
+    ignore_return_code: Ignores the return code for GIT command

[eakuefner, 2016/08/30 17:33:49]

nit: end with period?

[prasadv, 2016/08/30 18:49:40]

Done.

+
+  Returns:
+    The output of the GIT command as string.
+
+  Raises:
+    TrybotError: This exception is raised when GIT command fails.
+  """
+  proc = subprocess.Popen(
+      [_GIT_CMD] + cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)

[eakuefner, 2016/08/30 17:33:50]

Can you just write [_GIT_CMD, cmd]?

[prasadv, 2016/08/30 18:49:40]

cmd is a list of arguments for git command and I think subprocess.Popen expects
all tis argurmenst as list of strings.

+  output, err = proc.communicate()
+  returncode = proc.poll()
+  if returncode:
+    if ignore_return_code:
+      return None
+    raise TrybotError('%s. \n%s' % (msg_on_error, err))
+
+  return output
+
+
 class Trybot(command_line.ArgParseCommand):
-  """ Run telemetry perf benchmark on trybot """
+  """Run telemetry perf benchmark on trybot."""
 
   usage = 'botname benchmark_name [<benchmark run options>]'
   _builders = None
@@ -159,10 +180,10 @@ class Trybot(command_line.ArgParseCommand):
       # In case of any kind of exception, allow tryjobs to use default trybots.
       # Possible exception are ssl.SSLError, urllib2.URLError,
       # socket.timeout, socket.error.
-      except Exception:
+      except Exception:  # pylint: disable=broad-except
         # Incase of any exception return default trybots.
         print ('WARNING: Unable to reach builbot to retrieve trybot '
-            'information, tryjob will use default trybots.')
+               'information, tryjob will use default trybots.')
         cls._builders = DEFAULT_TRYBOTS
       else:
         builders = json.loads(f.read()).keys()
@@ -189,9 +210,9 @@ class Trybot(command_line.ArgParseCommand):
       if arg == '--browser' or arg.startswith('--browser='):
         parser.error('--browser=... is not allowed when running trybot.')
     all_benchmarks = discover.DiscoverClasses(
-      start_dir=path_util.GetPerfBenchmarksDir(),
-      top_level_dir=path_util.GetPerfDir(),
-      base_class=benchmark.Benchmark).values()
+        start_dir=path_util.GetPerfBenchmarksDir(),
+        top_level_dir=path_util.GetPerfDir(),
+        base_class=benchmark.Benchmark).values()
     all_benchmark_names = [b.Name() for b in all_benchmarks]
     all_benchmarks_by_names = {b.Name(): b for b in all_benchmarks}
     benchmark_class = all_benchmarks_by_names.get(options.benchmark_name, None)
@@ -199,9 +220,9 @@ class Trybot(command_line.ArgParseCommand):
       possible_benchmark_names = matching.GetMostLikelyMatchedObject(
           all_benchmark_names, options.benchmark_name)
       parser.error(
-         'No benchmark named "%s". Do you mean any of those benchmarks '
-         'below?\n%s' %
-         (options.benchmark_name, '\n'.join(possible_benchmark_names)))
+          'No benchmark named "%s". Do you mean any of those benchmarks '
+          'below?\n%s' % (
+              options.benchmark_name, '\n'.join(possible_benchmark_names)))
     is_benchmark_disabled, reason = cls.IsBenchmarkDisabledOnTrybotPlatform(
         benchmark_class, options.trybot)
     also_run_disabled_option = '--also-run-disabled-tests'
@@ -211,7 +232,7 @@ class Trybot(command_line.ArgParseCommand):
 
   @classmethod
   def IsBenchmarkDisabledOnTrybotPlatform(cls, benchmark_class, trybot_name):
-    """ Return whether benchmark will be disabled on trybot platform.
+    """Return whether benchmark will be disabled on trybot platform.
 
     Note that we cannot tell with certainty whether the benchmark will be
     disabled on the trybot platform since the disable logic in ShouldDisable()
@@ -282,89 +303,19 @@ class Trybot(command_line.ArgParseCommand):
     if extra_args is None:
       extra_args = []
     self._InitializeBuilderNames(options.trybot)
-
-    arguments = [options.benchmark_name] + extra_args
-
-    # First check if there are chromium changes to upload.
-    status = self._AttemptTryjob(CHROMIUM_CONFIG_FILENAME, arguments)
-    if status not in [SUCCESS, ERROR]:
-      # If we got here, there are no chromium changes to upload. Try blink.
-      os.chdir('third_party/WebKit/')
-      status = self._AttemptTryjob(BLINK_CONFIG_FILENAME, arguments)
-      os.chdir('../..')
-      if status not in [SUCCESS, ERROR]:
-        logging.error('No local changes found in chromium or blink trees. '
-                      'browser=%s argument sends local changes to the '
-                      'perf trybot(s): %s.', options.trybot,
-                      self._builder_names.values())
-        return 1
-    return 0
-
-  def _UpdateConfigAndRunTryjob(self, bot_platform, cfg_file_path, arguments):
-    """Updates perf config file, uploads changes and excutes perf try job.
-
-    Args:
-      bot_platform: Name of the platform to be generated.
-      cfg_file_path: Perf config file path.
-
-    Returns:
-      (result, msg) where result is one of:
-          SUCCESS if a tryjob was sent
-          NO_CHANGES if there was nothing to try,
-          ERROR if a tryjob was attempted but an error encountered
-          and msg is an error message if an error was encountered, or rietveld
-          url if success, otherwise throws TrybotError exception.
-    """
-    config = self._GetPerfConfig(bot_platform, arguments)
-    config_to_write = 'config = %s' % json.dumps(
-        config, sort_keys=True, indent=2, separators=(',', ': '))
-
     try:
-      with open(cfg_file_path, 'r') as config_file:
-        if config_to_write == config_file.read():
-          return NO_CHANGES, ''
-    except IOError:
-      msg = 'Cannot find %s. Please run from src dir.' % cfg_file_path
-      return (ERROR, msg)
-
-    with open(cfg_file_path, 'w') as config_file:
-      config_file.write(config_to_write)
-    # Commit the config changes locally.
-    returncode, out, err = _RunProcess(
-        [_GIT_CMD, 'commit', '-a', '-m', 'bisect config: %s' % bot_platform])
-    if returncode:
-      raise TrybotError('Could not commit bisect config change for %s,'
-                        ' error %s' % (bot_platform, err))
-    # Upload the CL to rietveld and run a try job.
-    returncode, out, err = _RunProcess([
-        _GIT_CMD, 'cl', 'upload', '-f', '--bypass-hooks', '-m',
-        'CL for perf tryjob on %s' % bot_platform
-    ])
-    if returncode:
-      raise TrybotError('Could not upload to rietveld for %s, error %s' %
-                        (bot_platform, err))
-
-    match = re.search(r'https://codereview.chromium.org/[\d]+', out)
-    if not match:
-      raise TrybotError('Could not upload CL to rietveld for %s! Output %s' %
-                        (bot_platform, out))
-    rietveld_url = match.group(0)
-    # Generate git try command for available bots.
-    git_try_command = [_GIT_CMD, 'cl', 'try', '-m', 'tryserver.chromium.perf']
-    for bot in self._builder_names[bot_platform]:
-      git_try_command.extend(['-b', bot])
-    returncode, out, err = _RunProcess(git_try_command)
-    if returncode:
-      raise TrybotError('Could not try CL for %s, error %s' %
-                        (bot_platform, err))
-
-    return (SUCCESS, rietveld_url)
+      self._AttemptTryjob(CHROMIUM_SRC_PATH, options, extra_args)
+    except TrybotError, error:
+      print error

[nednguyen, 2016/08/29 23:15:22]

Why print error then return 1? Wouldn't not catching the exception will give the
same effect with the clearer stack trace?

[prasadv, 2016/08/30 18:49:40]

Most of the TryjobError exceptions are raised when something goes wrong with git
commands in the tool. The reason to print the error is to display more meaning
error message in this exception is raised.

+      return 1
+    return 0
 
   def _GetPerfConfig(self, bot_platform, arguments):
     """Generates the perf config for try job.
 
     Args:
       bot_platform: Name of the platform to be generated.
+      arguments: Command line arguments.
 
     Returns:
       A dictionary with perf config parameters.
@@ -374,7 +325,7 @@ class Trybot(command_line.ArgParseCommand):
 
     # Always set verbose logging for later debugging
     if '-v' not in arguments and '--verbose' not in arguments:
-        arguments.append('--verbose')
+      arguments.append('--verbose')
 
     # Generate the command line for the perf trybots
     target_arch = 'ia32'
@@ -384,17 +335,15 @@ class Trybot(command_line.ArgParseCommand):
           'Trybot does not suport --chrome-root option set directly '
           'through command line since it may contain references to your local '
           'directory')
-    if bot_platform in ['win', 'win-x64']:
-      arguments.insert(0, 'python tools\\perf\\run_benchmark')
-    else:
-      arguments.insert(0, './tools/perf/run_benchmark')
 
+    arguments.insert(0, 'src/tools/perf/run_benchmark')
     if bot_platform == 'android':
       arguments.insert(1, '--browser=android-chromium')
     elif any('x64' in bot for bot in self._builder_names[bot_platform]):
       arguments.insert(1, '--browser=release_x64')
       target_arch = 'x64'
     else:
+
       arguments.insert(1, '--browser=release')
 
     command = ' '.join(arguments)
@@ -407,100 +356,111 @@ class Trybot(command_line.ArgParseCommand):
         'target_arch': target_arch,
     }
 
-  def _AttemptTryjob(self, cfg_file_path, arguments):
-    """Attempts to run a tryjob from the current directory.
-
-    This is run once for chromium, and if it returns NO_CHANGES, once for blink.
+  def _GetRepoAndBranchName(self, repo_path):
+    """Gets the repository name and working branch name.
 
     Args:
-      cfg_file_path: Path to the config file for the try job.
+      repo_path: Path to the repository.
 
     Returns:
-      Returns SUCCESS if a tryjob was sent, NO_CHANGES if there was nothing to
-      try, ERROR if a tryjob was attempted but an error encountered.
+      Repository name and branch name as tuple.
+
+    Raises:
+      TrybotError: This exception is raised for the following cases,

[eakuefner, 2016/08/30 17:33:49]

nit: cases:

[prasadv, 2016/08/30 18:49:40]

Done.

+        1. Try job is for non-git repository or in invalid branch.
+        2. Un-committed changes in the current branch.
+        3. No local commits in the current branch.
     """
-    source_repo = 'chromium'
-    if cfg_file_path == BLINK_CONFIG_FILENAME:
-      source_repo = 'blink'
-
-    # TODO(prasadv): This method is quite long, we should consider refactor
-    # this by extracting to helper methods.
-    returncode, original_branchname, err = _RunProcess(
-        [_GIT_CMD, 'rev-parse', '--abbrev-ref', 'HEAD'])
-    if returncode:
-      msg = 'Must be in a git repository to send changes to trybots.'
-      if err:
-        msg += '\nGit error: %s' % err
-      logging.error(msg)
-      return ERROR
-
-    original_branchname = original_branchname.strip()
+    # If command runs successfully, then the output will be repo root path.
+    # and current branch name.
+    output = RunGit(['rev-parse', '--abbrev-ref', '--show-toplevel', 'HEAD'],
+                    ('%s is not a git repository, must be in a git repository '
+                     'to send changes to trybots' % os.getcwd()))
+
+    repo_info = output.split()
+    # Assuming the base directory name is same as repo project name set in
+    # codereviews.settings file.
+    repo_name = os.path.basename(repo_info[0]).strip()
+    branch_name = repo_info[1].strip()
+
+    if branch_name == 'HEAD':
+      raise TrybotError('Not on a valid branch, looks like branch '
+                        'is dettached. [branch:%s]' % branch_name)
 
     # Check if the tree is dirty: make sure the index is up to date and then
     # run diff-index
-    _RunProcess([_GIT_CMD, 'update-index', '--refresh', '-q'])
-    returncode, out, err = _RunProcess([_GIT_CMD, 'diff-index', 'HEAD'])
-    if out:
-      logging.error(
-          'Cannot send a try job with a dirty tree. Commit locally first.')
-      return ERROR
+    RunGit(['update-index', '--refresh', '-q'], ignore_return_code=True)
+    output = RunGit(['diff-index', 'HEAD'])
+    if output:
+      raise TrybotError(
+          'Cannot send a try job with a dirty tree. Please commit '
+          'your changes locally first in %s repository.' % repo_path)
 
     # Make sure the tree does have local commits.
-    returncode, out, err = _RunProcess(
-        [_GIT_CMD, 'log', 'origin/master..HEAD'])
-    if not out:
-      return NO_CHANGES
-
-    # Create/check out the telemetry-tryjob branch, and edit the configs
-    # for the tryjob there.
-    returncode, out, err = _RunProcess(
-        [_GIT_CMD, 'checkout', '-b', 'telemetry-tryjob'])
-    if returncode:
-      logging.error('Error creating branch telemetry-tryjob. '
-                    'Please delete it if it exists.\n%s', err)
-      return ERROR
-    try:
-      returncode, out, err = _RunProcess(
-          [_GIT_CMD, 'branch', '--set-upstream-to', 'origin/master'])
-      if returncode:
-        logging.error('Error in git branch --set-upstream-to: %s', err)
-        return ERROR
-      for bot_platform in self._builder_names:
-        if not self._builder_names[bot_platform]:
-          logging.warning('No builder is found for %s', bot_platform)
-          continue
-        try:
-          results, output = self._UpdateConfigAndRunTryjob(
-              bot_platform, cfg_file_path, arguments)
-          if results == ERROR:
-            logging.error(output)
-            return ERROR
-          elif results == NO_CHANGES:
-            print ('Skip the try job run on %s because it has been tried in '
-                   'previous try job run. ' % bot_platform)
-          else:
-            print ('Uploaded %s try job to rietveld for %s platform. '
-                   'View progress at %s' % (source_repo, bot_platform, output))
-        except TrybotError, err:
-          print err
-          logging.error(err)
-    finally:
-      # Checkout original branch and delete telemetry-tryjob branch.
-      # TODO(prasadv): This finally block could be extracted out to be a
-      # separate function called _CleanupBranch.
-      returncode, out, err = _RunProcess(
-          [_GIT_CMD, 'checkout', original_branchname])
-      if returncode:
-        logging.error('Could not check out %s. Please check it out and '
-                      'manually delete the telemetry-tryjob branch. '
-                      ': %s', original_branchname, err)
-        return ERROR  # pylint: disable=lost-exception
-      logging.info('Checked out original branch: %s', original_branchname)
-      returncode, out, err = _RunProcess(
-          [_GIT_CMD, 'branch', '-D', 'telemetry-tryjob'])
-      if returncode:
-        logging.error('Could not delete telemetry-tryjob branch. '
-                      'Please delete it manually: %s', err)
-        return ERROR  # pylint: disable=lost-exception
-      logging.info('Deleted temp branch: telemetry-tryjob')
-    return SUCCESS
+    output = RunGit(['footers', 'HEAD'])
+    if output:
+      raise TrybotError('No local changes found in %s repository.' % repo_path)
+
+    return (repo_name, branch_name)
+
+  def _AttemptTryjob(self, repo_path, options, extra_args):
+    """Attempts to run a tryjob from a repo directory.
+
+    Args:
+      repo_path: Path to the repository.
+      options: Command line arguments to run benchmark.
+      extra_args: Extra arugments to run benchmark.
+    """
+    repo_name, branch_name = self._GetRepoAndBranchName(repo_path)
+
+    arguments = [options.benchmark_name] + extra_args
+
+    rietveld_url = self._UploadPatchToRietveld(repo_name)
+    print ('\nUploaded try job to rietveld.\nview progress here %s.'
+           '\n\tRepo Name: %s\n\tPath: %s\n\tBranch: %s' % (
+               rietveld_url, repo_name, repo_path, branch_name))
+
+    for bot_platform in self._builder_names:
+      if not self._builder_names[bot_platform]:
+        logging.warning('No builder is found for %s', bot_platform)
+        continue
+      try:
+        self._RunTryJob(bot_platform, arguments)
+      except TrybotError, err:
+        print err
+
+  def _UploadPatchToRietveld(self, repo_name):
+    """Uploads the patch to rietveld and returns rietveld URL."""
+    output = RunGit(['cl', 'upload', '-f', '--bypass-hooks', '-m',
+                     'CL for %s perf tryjob' % repo_name],

[nednguyen, 2016/08/29 23:15:22]

nits: would be nice to include benchmark name & bots in the message. 

Also would the title be the same as the message or do we need '-t' option as
well?

[eakuefner, 2016/08/30 17:33:50]

The title should be the same as the first/only line of the message for new CLs.

[prasadv, 2016/08/30 18:49:40]

Yes, CL title will be the first line of the message.
This raises a new question:
Before this change we used to create a new branch to upload and run the try job.
The rietveld issue created for tryjob was specific to the new branch. But with
this change the rietveld issue is created in users working branch.
So if the user choose to send his/her patch for review in their working branch,
they are required to change the title of the issue or reset the issue number and
upload the patch again.
I'm not sure whether this is acceptable to user.

[nednguyen, 2016/08/30 18:55:23]

It seems not very ideal that this overwrites existing CL of user. Can we do a
"git cl issue 0" before do "git cl upload"?

+                    'Could not upload to rietveld for %s' % repo_name)
+
+    match = re.search(r'https://codereview.chromium.org/[\d]+', output)
+    if not match:
+      raise TrybotError('Could not upload CL to rietveld for %s! Output %s' %
+                        (repo_name, output))
+    return match.group(0)
+
+  def _RunTryJob(self, bot_platform, arguments):
+    """Executes perf try job with benchmark test properties.
+
+    Args:
+      bot_platform: Name of the platform to be generated.
+      arguments: Command line arguments.
+
+    Raises:
+      TrybotError: When trybot fails to upload CL or run git try.
+    """
+    config = self._GetPerfConfig(bot_platform, arguments)
+
+    # Generate git try command for available bots.
+    git_try_command = ['cl', 'try', '-m', 'tryserver.chromium.perf']
+
+     # Add Perf Test config to git try --properties arg.
+    git_try_command.extend(['-p', 'perf_try_config=%s' % json.dumps(config)])
+
+    for bot in self._builder_names[bot_platform]:
+      git_try_command.extend(['-b', bot])
+
+    RunGit(git_try_command, 'Could not try CL for %s' % bot_platform)
+    print 'Perf Try job sent to rietveld for %s platform.' % bot_platform