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..86fe1cf4bd1fbc4987fc3c057a97bbab371d3881 100644
--- a/tools/perf/core/trybot_command.py
+++ b/tools/perf/core/trybot_command.py
@@ -63,6 +63,33 @@ DEFAULT_TRYBOTS = [
 ]
 
 CHROMIUM_SRC_PATH = path_util.GetChromiumSrcDir()
+# Mapping of repo to its root path and git remote URL.
+# Values for 'src' key in the map are related to path to the repo in the
+# DEPS file, These values are to used create the DEPS patch along with patch
+# that is being tried.

[Michael Achenbach, 2016/09/20 10:18:20]

I think we should try to not introduce yet another of these mappings. Future
stakeholders will wonder, "Why doesn't it work for my project?". This should be
generic (or at least configured in one place) and I left comments below, how to
do without REPO_INFO_MAP.

[prasadv, 2016/09/20 19:12:01]

As mentioned in the comments, these mappings are necessary in order to validate
the repos correctly.
'src': Its value is mapped to the repo path in DEPS file. In order to apply the
DEPS revision on the patch we use the DEPS path + DEPS revision as a property to
buildbucket. Without this mapping buildbucket will not know be apple to apply
DEPS revision.

'url': This is the remote URL of the repo which is used to validate whether the
repo we are processing is supported by tryjob. Mostly this is used to avoid
running try jobs for unsupported repos. 

+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',
+    },
+    'catapult': {
+        'src': 'src/third_party/catapult',
+        'url': ('https://chromium.googlesource.com/external/github.com/'
+                'catapult-project/catapult.git')
+    }
+}
 
 assert not set(DEFAULT_TRYBOTS) & set(EXCLUDED_BOTS), (
     'A trybot cannot present in both Default as well as Excluded bots lists.')
@@ -157,7 +184,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):
@@ -292,6 +319,36 @@ 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 the repo path where the patch is created.'
+This argument should only be used if the changes are made outside chromium repo.
+E.g.,
+1) Assume you are running run_benchmarks command from $HOME/cr/src/ direcory:'
+  a) If your changes are in $HOME/cr/src/v8, then --repo_path=v8 or
+     --repo-path=$HOME/cr/src/v8
+  b) If your changes are in $HOME/cr/src/third_party/catapult, then
+     --repo_path=third_party/catapult or
+     --repo_path = $HOME/cr/src/third_party/catapult'
+  c) If your changes are not relative to src/ e.g. you created changes in some
+     other direcotry say $HOME/mydir/v8/v8/, then the
+     --repo_path=$HOME/mydir/v8/v8
+2) Assume you are running run_benchmarks command not relatvie to src i.e.,
+   you are running from $HOME/mydir/ direcory:'
+   a) If your changes are in $HOME/cr/src/v8, then --repo-path=$HOME/cr/src/v8
+   b) If your changes are in $HOME/cr/src/third_party/catapult, then
+      --repo_path=$HOME/cr/src/third_party/catapult'
+  c) If your changes are in $HOME/mydir/v8/v8/, then the
+     --repo_path=$HOME/mydir/v8/v8 or --repo_path=v8/v8"""),
+        metavar='<repo path>')
+    parser.add_argument(
+        '--deps_revision', type=str, default=None,
+        help=('specify DEPS revision to modify DEPS entry in Chromium to a '
+              'certain pushed revision.\n'
+              'This revision overrides value in DEPS on TOT Chromium for the '
+              'repo specified in --repo_path.\nIt is applied for both with and '
+              'wihout patch.'),
+        metavar='<deps revision>')
 
   def Run(self, options, extra_args=None):
     """Sends a tryjob to a perf trybot.
@@ -303,12 +360,8 @@ class Trybot(command_line.ArgParseCommand):
     if extra_args is None:
       extra_args = []
     self._InitializeBuilderNames(options.trybot)
-    try:
-      self._AttemptTryjob(CHROMIUM_SRC_PATH, options, extra_args)
-    except TrybotError, error:
-      print error
-      return 1
-    return 0
+
+    return self._AttemptTryjob(options, extra_args)
 
   def _GetPerfConfig(self, bot_platform, arguments):
     """Generates the perf config for try job.
@@ -403,31 +456,107 @@ class Trybot(command_line.ArgParseCommand):
 
     return (repo_name, branch_name)
 
-  def _AttemptTryjob(self, repo_path, options, extra_args):
+  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.
+    """
+    # Check if there is any upstream branch associated with current working
+    # branch, Once the upstream branch is found i.e., then validates the
+    # remote URL and then returns the HEAD revision from the remote branch.
+    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.')
+
+    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, 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, 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)
-      except TrybotError, err:
-        print err
+    Returns:
+     If successful returns 0, otherwise 1.
+    """
+    original_workdir = os.getcwd()
+    repo_path = os.path.abspath(options.repo_path)
+    try:
+      # Check the existence of repo path.
+      if not os.path.exists(repo_path):
+        raise TrybotError('Repository path "%s" does not exist, please check '
+                          'the value of <repo_path> argument.' % repo_path)
+      # Change to the repo directory.
+      os.chdir(repo_path)
+      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)
+
+      deps_override = None
+      if repo_name != 'src':
+        if not options.deps_revision:

[Michael Achenbach, 2016/09/20 10:18:20]

Why is this feature limited to non-src repos? I find this feature orthogonal to
allowing deps tryjobs. Against which revision are src CLs tried?

I'd be fine with trying against a default like HEAD or DEPS revision of the
repo, still allowing manual override with deps_revision.

A feature that calculates the locally used upstream revision is neat, but I
don't understand why it is tied to this CL and not already present for src
tryjobs.

[Michael Achenbach, 2016/09/22 07:44:47]

ok - could you comment also on this remark?

[prasadv, 2016/09/22 17:50:31]

src CLs are always tried against TOT chromium revisions, therefore we don't
override DEPS revisions for src.
Moreover we want to override deps revision only for DEPs repo try jobs because
the TOT chromium revision might not be pinned to the locally used upstream
revision for the CL.

[Michael Achenbach, 2016/09/22 20:46:29]

Why not always run against ToT of the deps repo? Any CL under developent is
usually supposed to work with ToT of the respective repo. If devs want something
else they can manually override.

And it's never guaranteed anyway that a locally used version of a deps repo will
fit to Chromium ToT.

One reason I've heard from developers, why they'd want to perf test against an
older revision and not ToT, is to keep their results comparable (from patch set
to patch set). But that use case would apply to Chromium as the main project as
well.

+          options.deps_revision = self._GetBaseGitHashForRepo(
+              branch_name, repo_info.get('url'))

[Michael Achenbach, 2016/09/20 10:18:20]

You could get the remote repo url from the local repo itself, not needing a
static mapping.

[prasadv, 2016/09/20 19:12:00]

Agreed, but we want to validate the remote URL to check if it is supported by
perf try job.

+        deps_override = {repo_info.get('src'): options.deps_revision}

[Michael Achenbach, 2016/09/20 10:18:20]

You shouldn't need to specify the name of the deps here if the project has set
up the patch_project setting in codereview.settings file. The mapping of
patch_project name to deps name already exists for many repos, e.g.:
https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/gclient/con...

Bot_update automatically updates the HEAD of the listed repositories when
checking out chromium and with project set to one of the listed projects. It
should be easy to override HEAD on the recipe side to something else.

[prasadv, 2016/09/20 19:12:00]

Actually running the tests and building the binary happens on two separate bots.
When processing the try job the try bot post a build request to the builder
along with the patch.
And the builder expects the dep_override along with name of the deps.

+
+      arguments = [options.benchmark_name] + extra_args
+
+      rietveld_url = self._UploadPatchToRietveld(repo_name, options)

[Michael Achenbach, 2016/09/20 10:18:20]

This code here has nothing to do with the arguments calculation above. Maybe
change the order of steps and group things that are clearly dependent on each
other? Or do you want to make sure to fail early?

Furthermore, why is this script at all concerned with uploading to rietveld? IMO
this script should rather deal with already uploaded CLs.

[prasadv, 2016/09/20 19:12:01]

Perf try job uses git cl try to run the try job on the patch by uploading it to
the rietveld.
This will work for both the case i.e., already uploaded CL and a not uploaded
CL(the try job will upload it).

+      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, deps_override)
+        # Even if git cl try throws TrybotError exception for any platform,
+        # keep sending try jobs to other platforms.
+        except TrybotError, err:
+          print err
+    except TrybotError, error:
+      print error
+      return 1
+    finally:
+      # Restore to original working directory.
+      os.chdir(original_workdir)
+    return 0
 
   def _UploadPatchToRietveld(self, repo_name, options):
     """Uploads the patch to rietveld and returns rietveld URL."""
@@ -443,12 +572,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,8 +591,14 @@ 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)])
 
+    error_msg_on_fail = 'Could not try CL for %s' % bot_platform
+    # Add deps overrides to git try --properties arg.
+    if deps_override:
+      git_try_command.extend([
+          '-p', 'deps_revision_overrides=%s' % json.dumps(deps_override)])
+      error_msg_on_fail += ' with DEPS override (%s)' % 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)
+    RunGit(git_try_command, error_msg_on_fail)
     print 'Perf Try job sent to rietveld for %s platform.' % bot_platform