Extend Perf Try Job support to other chromium repos.

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 a53a5a357f5ee7e5da1f9feaa5dfa079ba031215..11bc923c819c493a536614cc6e183927c804913f 100644
--- a/tools/perf/core/trybot_command.py
+++ b/tools/perf/core/trybot_command.py
@@ -63,6 +63,25 @@ DEFAULT_TRYBOTS = [
 ]
 
 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.')
@@ -157,8 +176,7 @@ def RunGit(cmd, msg_on_error='', ignore_return_code=False):
       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."""
@@ -292,6 +310,14 @@ class Trybot(command_line.ArgParseCommand):
         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.
@@ -303,11 +329,23 @@ class Trybot(command_line.ArgParseCommand):
     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):
@@ -403,6 +441,48 @@ class Trybot(command_line.ArgParseCommand):
 
     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.
 
@@ -413,6 +493,17 @@ class Trybot(command_line.ArgParseCommand):
     """
     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)
@@ -425,7 +516,7 @@ class Trybot(command_line.ArgParseCommand):
         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
 
@@ -443,12 +534,13 @@ class Trybot(command_line.ArgParseCommand):
                         (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.
@@ -461,6 +553,10 @@ class Trybot(command_line.ArgParseCommand):
      # 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])