Extend Perf Try Job support to other chromium repos.

tools/perf/core/trybot_command.py

 # Copyright 2015 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.
 
 import argparse
 import json
 import logging
 import os
 import platform
 import re
@@ skipping 45 matching lines @@
 
 # Default try bot to use incase builbot is unreachable.
 DEFAULT_TRYBOTS = [
     'linux_perf_bisect',
     'mac_10_11_perf_bisect',
     'winx64_10_perf_bisect',
     'android_s5_perf_bisect',
 ]
 
 CHROMIUM_SRC_PATH = path_util.GetChromiumSrcDir()
+# Mapping of repo to its root path and git remote URL.
+REPO_INFO_MAP = {
+    'src': {
+        'src': 'src',
+        'url': 'https://chromium.googlesource.com/chromium/src.git',
+    },
+    'v8': {
+        'src': 'src/v8',
+        'url': 'https://chromium.googlesource.com/v8/v8.git',
+    },
+    'skia': {
+        'src': 'src/third_party/skia',
+        'url': 'https://chromium.googlesource.com/skia.git',
+    },
+    'angle': {
+        'src': 'src/third_party/angle',
+        'url': 'https://chromium.googlesource.com/angle/angle.git',
+    }
+}

[sullivan, 2016/09/08 03:22:59]

Any reason not to add catapult here as well?

[prasadv, 2016/09/08 19:03:32]

I added support for catapult too.

 
 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 '(ERROR) Perf Try Job: %s' % self.args[0]
 
@@ skipping 74 matching lines @@
   """
   proc = subprocess.Popen(
       [_GIT_CMD] + cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
   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
-
+  return output.strip()
 
 class Trybot(command_line.ArgParseCommand):
   """Run telemetry perf benchmark on trybot."""
 
   usage = 'botname benchmark_name [<benchmark run options>]'
   _builders = None
 
   def __init__(self):
     self._builder_names = None
 
@@ skipping 113 matching lines @@
     parser.add_argument(
         'trybot', choices=available_bots,
         help=('specify which bots to run telemetry benchmarks on. '
               ' Allowed values are:\n' + '\n'.join(available_bots)),
         metavar='<trybot name>')
     parser.add_argument(
         'benchmark_name', type=str,
         help=('specify which benchmark to run. To see all available benchmarks,'
               ' run `run_benchmark list`'),
         metavar='<benchmark name>')
+    parser.add_argument(
+        '--repo_path', type=str, default=CHROMIUM_SRC_PATH,
+        help='specify which repo path to use for perf try job\n',
+        metavar='<repo path>')

[sullivan, 2016/09/08 03:22:59]

This is supposed to be specified relative to cwd? For example, if you're in src/
directory, you would say 'v8' or 'third_party/skia'? And if you're in v8
directory, it would be '.'?

That's kind of confusing, especially since IIRC telemetry supports running from
a wide variety of cwds. I think it would be clearer for users to specify 'v8',
'skia', 'angle' always, with None for chromium. Especially since you already
have to store the lookup info in REPO_INFO_MAP anyway.

[prasadv, 2016/09/08 19:03:32]

No, this is the complete path to the directory where the patch exist.
As discussed with v8 about normal workflow for v8 development, they create
separate directory (not in src/) for normal development. Therefore we don't want
to assume the patches are in repos relative to src/ directory.

[sullivan, 2016/09/08 20:43:57]

I think the documentation needs to be clearer, especially since you've got
different user groups with different assumptions: v8 team would be running this
script with a directory not relative to src/, but with the way the catapult
documentation talks about developing in Chrome, catapult developers likely
wouldn't know you could run this with a path that isn't third_party/catapult.

[prasadv, 2016/09/08 23:44:36]

Please note that the path info in REPO_INFO_MAP is not the actual path to the
patches. They are are related to path to the repo in the
# DEPS file, These values are used create the DEPS patch along with patch that
is being tried.
I also added detailed explanation about --repo_path in command help.

+    parser.add_argument(
+        '--deps_revision', type=str, default=None,
+        help='specify DEPS revision to be used by Chromium.\n',

[sullivan, 2016/09/08 03:22:59]

Would be great to clarify what this argument does a little in the help text:

* When would you want to specify a DEPS revision?
* I assume DEPS revision changes the value in DEPS for the repo specified in
--repo_path?
* Does this apply to with patch, without patch, or both?

[prasadv, 2016/09/08 19:03:32]

This is used when we want to modify DEPS entry in Chromium to a certain pushed
revision. If this is not specified then the tool will determine the DEPS
revision on top of which the changes are made.
Yes, this changes the DEPS revision for the repo in TOT chromium.
This is applied while process both (with and without patch).

+        metavar='<deps revision>')
 
   def Run(self, options, extra_args=None):
     """Sends a tryjob to a perf trybot.
 
     This creates a branch, telemetry-tryjob, switches to that branch, edits
     the bisect config, commits it, uploads the CL to rietveld, and runs a
     tryjob on the given bot.
     """
     if extra_args is None:
       extra_args = []
     self._InitializeBuilderNames(options.trybot)
+
+    original_workdir = os.getcwd()
+    repo_path = os.path.abspath(options.repo_path)
     try:
-      self._AttemptTryjob(CHROMIUM_SRC_PATH, options, extra_args)
+      # Check the existence of repo path.
+      if not os.path.exists(repo_path):
+        raise TrybotError('Repository path "%s" does not exists, please check '

[sullivan, 2016/09/08 03:22:59]

s/does not exists/does not exist/

[prasadv, 2016/09/08 19:03:33]

Done.

+                          'the value of <repo_path> argument.' % repo_path)
+      # Change to the repo directory.
+      os.chdir(repo_path)

[sullivan, 2016/09/08 03:22:59]

Why the need for os.chdir here?

[prasadv, 2016/09/08 19:03:32]

As mentioned in the previous comments, repo_path is the path to the directory
where the patch exists. For dependency repo's workflow it is not always true
that they do development under src/ directory.

[sullivan, 2016/09/08 20:43:56]

So it looks like _AttemptTryJob requires you to be in repo_path and also takes
repo_path as an argument. Shouldn't the try, os.chdir, finally all be in
_AttemptTryJob?

[prasadv, 2016/09/08 23:44:36]

Done.

+      self._AttemptTryjob(repo_path, options, extra_args)
     except TrybotError, error:
       print error
       return 1
+    finally:
+      # Restore to original working directory.
+      os.chdir(original_workdir)
     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:
@@ skipping 75 matching lines @@
           '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.
     output = RunGit(['footers', 'HEAD'])
     if output:
       raise TrybotError('No local changes found in %s repository.' % repo_path)
 
     return (repo_name, branch_name)
 
+  def _GetBaseGitHashForRepo(self, branch_name, git_url):
+    """Gets the base revision for the repo on which local changes are made.
+
+    Finds the upstream of the current branch that it is set to and gets
+    the HEAD revision from upstream. This also checks if the remote URL on
+    the upstream is supported by Perf Try job.
+
+    Args:
+      branch_name: Current working branch name.
+      git_url: Remote URL of the repo.
+
+    Returns:
+      Git hash of the HEAD revision from the upstream branch.
+
+    Raises:
+      TrybotError: This exception is raised when a GIT command fails or if the
+      remote URL of the repo found is not supported.
+    """
+    while not self._IsRepoSupported(branch_name, git_url):
+      branch_name = RunGit(
+          ['rev-parse', '--abbrev-ref', '%s@{upstream}' % branch_name],
+          'Failed to get upstream branch name.')

[sullivan, 2016/09/08 03:22:59]

Isn't this an infinite loop?

[prasadv, 2016/09/08 19:03:33]

I don't think so, because every time we enter the loop branch_name is set to the
upstream of current branch.
This loop will break and raise TrybotError exception when a branch upstream is
not to the branch properly.

[sullivan, 2016/09/08 20:43:57]

I think adding a high-level comment to explain what the loop does would help
here.

[prasadv, 2016/09/08 23:44:36]

Done.

+
+    return RunGit(
+        ['rev-parse', '%s@{upstream}' % branch_name],
+        'Failed to get base revision hash on upstream.')
+
+  def _IsRepoSupported(self, current_branch, repo_git_url):
+    cur_remote = RunGit(
+        ['config', 'branch.%s.remote'% current_branch],
+        'Failed to get branch.%s.remote from git config' % current_branch)
+    cur_remote = cur_remote.strip()
+    if cur_remote == '.':
+      return False
+    cur_remote_url = RunGit(
+        ['config', 'remote.%s.url' % cur_remote],
+        'Failed to get remote.%s.url from git config' % cur_remote)
+    if cur_remote_url.lower() == repo_git_url:
+      return True
+    raise TrybotError('URL %s on remote %s is not recognized on branch.'% (
+        cur_remote_url, cur_remote))
+
   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)
 
+    repo_info = REPO_INFO_MAP.get(repo_name, None)
+    if not repo_info:
+      raise TrybotError('Unsupported repository %s' % repo_name)

[sullivan, 2016/09/08 03:22:59]

Here the error would be clearer if users specified repo_name instead of
repo_path.

+
+    deps_override = None
+    if repo_name != 'src':
+      if not options.deps_revision:
+        options.deps_revision = self._GetBaseGitHashForRepo(
+            branch_name, repo_info.get('url'))
+      deps_override = {repo_info.get('src'): options.deps_revision}
+
     arguments = [options.benchmark_name] + extra_args
 
     rietveld_url = self._UploadPatchToRietveld(repo_name, options)
     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)
+        self._RunTryJob(bot_platform, arguments, deps_override)
       except TrybotError, err:
         print err
 
   def _UploadPatchToRietveld(self, repo_name, options):
     """Uploads the patch to rietveld and returns rietveld URL."""
     output = RunGit(['cl', 'upload', '-f', '--bypass-hooks', '-m',
                      ('CL for %s perf tryjob to run %s benchmark '
                       'on %s platform(s)' % (
                           repo_name, options.benchmark_name, options.trybot))],
                     '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):
+  def _RunTryJob(self, bot_platform, arguments, deps_override):
     """Executes perf try job with benchmark test properties.
 
     Args:
       bot_platform: Name of the platform to be generated.
       arguments: Command line arguments.
+      deps_override: DEPS revision if needs to be overridden.
 
     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)])
 
+    # Add deps overrides to git try --properties arg.
+    if deps_override:
+      git_try_command.extend([
+          '-p', 'deps_revision_overrides=%s' % json.dumps(deps_override)])
     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