Remove CL upload logic from Perf Try Job workflow.

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
 import subprocess
+import tempfile
 import urllib2
 
 
 from core import path_util
 
 from telemetry import benchmark
 from telemetry import decorators
 from telemetry.core import discover
 from telemetry.util import command_line
 from telemetry.util import matching
@@ skipping 417 matching lines @@
     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
     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.
-    output = RunGit(['footers', 'HEAD'])
-    if output:
-      raise TrybotError('No local changes found in %s repository.' % repo_path)
+          'Cannot send a try job with a dirty tree.\nPlease commit locally and '
+          'upload your changes to rietveld 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.
 
@@ skipping 28 matching lines @@
     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 _GetChangeList(self):
+    """Gets the codereview URL for the current changes."""
+    temp_file = None
+    json_output = None
+    try:
+      fd, temp_file = tempfile.mkstemp(suffix='.json', prefix='cl')

[Michael Achenbach, 2016/10/08 09:43:29]

nit: Maybe add a slightly longer and more verbose prefix? I see you delete it
below, but if tempfiles are (for whatever reason) not deleted, it would be way
easier to reason where they come from.

[prasadv1, 2016/10/10 18:22:56]

Done.

+      os.close(fd)
+      RunGit(['cl', 'issue', '--json', temp_file],
+              'Failed to run "git cl issue" command.')
+      try:
+        with open(temp_file, 'r') as f:
+          json_output = json.load(f)
+      except (IOError, ValueError):

[Michael Achenbach, 2016/10/08 09:43:29]

Catching this here and setting json_output to None will make json_output.get
fail below. And it would be obscure what the original error was.

Maybe just don't catch the exceptions or recover from them in a more robust way?

[prasadv1, 2016/10/10 18:22:56]

Done.

+        json_output = None
+    finally:
+      try:
+        if temp_file:
+          os.remove(temp_file)
+      except OSError:
+        pass
+
+    # Make sure the local commits are uploaded to rietveld.
+    if not json_output.get('issue'):
+      raise TrybotError(
+          'PLEASE NOTE: The workflow for Perf Try jobs is changed. '
+          'In order to run the perf try job, you must first upload your '
+          'changes to rietveld.')
+    return json_output.get('issue_url')
+
   def _AttemptTryjob(self, options, extra_args):
     """Attempts to run a tryjob from a repo directory.
 
     Args:
       options: Command line arguments to run benchmark.
       extra_args: Extra arugments to run benchmark.
 
     Returns:
      If successful returns 0, otherwise 1.
     """
@@ skipping 12 matching lines @@
       if not repo_info:
         raise TrybotError('Unsupported repository %s' % repo_name)
 
       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.'
+      rietveld_url = self._GetChangeList()
+      print ('\nRunning try job....\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:
+          arguments = [options.benchmark_name] + extra_args
           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."""
-    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, 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.
@@ skipping 10 matching lines @@
     # 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, error_msg_on_fail)
     print 'Perf Try job sent to rietveld for %s platform.' % bot_platform