Added separate checkout and config file generation to bisect script

build/util/run-bisect-perf-regression.py

+#!/usr/bin/env python
+# Copyright (c) 2013 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.
+
+"""Run Performance Test Bisect Tool
+
+This script is used by a trybot to run the src/tools/bisect-perf-regression.py
+script with the parameters specified in run-bisect-perf-regression.cfg. It will
+check out a copy of the depot in a subdirectory 'bisect' of the working
+directory provided, and run the bisect-perf-regression.py script there.
+
+"""
+
+import errno
+import imp
+import optparse
+import os
+import subprocess
+import sys
+
+GCLIENT_SPEC = """
+solutions = [
+  { "name"        : "src",
+    "url"         : "https://chromium.googlesource.com/chromium/src.git",
+    "deps_file"   : ".DEPS.git",
+    "managed"     : True,
+    "custom_deps" : {
+    },
+    "safesync_url": "",
+  },
+  { "name"        : "src-internal",
+    "url"         : "ssh://simonhatch@gerrit-int.chromium.org:29419/" +

[tonyg, 2013/02/19 18:01:25]

I don't think we want to check in your user id here. Maybe Mike knows how the
existing bots manage a src-internal checkout.

[shatch, 2013/02/19 18:33:51]

Ah darn, forgot about that.

Q for Mike: Do I need to include any particular user id here?

+                    "chrome/src-internal.git",
+    "deps_file"   : ".DEPS.git",
+  },
+]
+"""
+GCLIENT_SPEC = ''.join([l for l in GCLIENT_SPEC.splitlines()])
+
+
+def RunGClient(params):
+  """Runs gclient with the specified parameters.
+
+  Args:
+    params: A list of parameters to pass to gclient.
+
+  Returns:
+    The return code of the call.
+  """
+  cmd = ['gclient'] + params
+  return subprocess.call(cmd)
+

[tonyg, 2013/02/19 18:01:25]

2 line breaks between top level definitions (throughout the file)

[shatch, 2013/02/19 18:33:51]

Done.

+def RunGClientAndCreateConfig():
+  """Runs gclient and creates a config containing both src and src-internal.
+
+  Returns:
+    The return code of the call.
+  """
+  return_code = RunGClient(
+      ['config', '--spec=%s' % GCLIENT_SPEC, '--git-deps'])
+  return return_code
+
+def RunGClientAndSync():
+  """Runs gclient and does a normal sync.
+
+  Returns:
+    The return code of the call.
+  """
+  return RunGClient(['sync'])
+
+def SetupGitDepot():
+  """Sets up the depot for the bisection. The depot will be located in a
+  subdirectory called 'bisect'.
+
+  Returns:
+    True if gclient successfully created the config file and did a sync, False
+    otherwise.
+  """
+  name = 'Setting up Bisection Depot'
+  print '@@@SEED_STEP %s@@@' % name
+  print '@@@STEP_CURSOR %s@@@' % name
+  print '@@@STEP_STARTED@@@'
+
+  passed = False
+
+  if not RunGClientAndCreateConfig():
+    if not RunGClientAndSync():
+      passed = True
+
+  print
+  print '@@@STEP_CLOSED@@@'
+
+  return passed
+
+def LoadConfigFile():
+  """Attempts to load the file 'run-bisect-perf-regression.cfg' as a module
+  and grab the global config dict.
+
+  Returns:
+    The config dict which should be formatted as follows:
+    {'command': string, 'good_revision': string, 'bad_revision': string
+     'metric': string}.
+    Returns None on failure.
+  """
+  try:
+    cfg_file = imp.load_source('config', 'run-bisect-perf-regression.cfg')
+
+    return cfg_file.config
+  except (KeyError, TypeError, IOError):
+    return None
+
+def RunBisectionScript(config):
+  """Attempts to execute src/tools/bisect-perf-regression.py with the parameters
+  passed in.
+
+  Args:
+    config: A dict containing the parameters to pass to the script.
+
+  Returns:
+    0 on success, otherwise 1.
+  """
+
+  cmd = [os.path.join('tools', 'bisect-perf-regression.py'),
+         '-c', config['command'],
+         '-g', config['good_revision'],
+         '-b', config['bad_revision'],
+         '-m', config['metric'],
+         '--debug_ignore_sync',

[tonyg, 2013/02/19 18:01:25]

Is this supposed to be checked in? If so, please add a comment explaining.

[shatch, 2013/02/19 18:33:51]

Done.

+         '--output_buildbot_annotations']
+
+  return_code = subprocess.call(cmd)
+
+  if return_code:
+    print 'Error: bisect-perf-regression.py returned with error %d' %\
+        return_code
+    print
+    return 1
+
+  return 0
+
+def CreateAndChangeToSourceDirectory(working_directory):
+  """Creates a directory 'bisect' as a subdirectory of 'working_directory'.  If
+  the function is successful, the current working directory will change to that
+  of the new 'bisect' directory.
+
+  Returns:
+    True if the directory was successfully created (or already existed).
+  """
+  cwd = os.getcwd()
+  os.chdir(os.path.join(working_directory))

[tonyg, 2013/02/19 18:01:25]

why the join call? Isn't working_directory just a string?

[shatch, 2013/02/19 18:33:51]

Done.

+  try:
+    os.mkdir('bisect')
+  except OSError, e:
+    if e.errno != errno.EEXIST:

[tonyg, 2013/02/19 18:01:25]

To make sure I understand this: the directory is not deleted between runs and we
expect that on the bot it will typically stay there and this script will be able
to avoid creating a checkout from scratch every time? If so, nice :) If not, can
we make that the case so that it will run faster?

[shatch, 2013/02/19 18:33:51]

Yep the intention is that the directory sticks around between runs.

+      return False
+  os.chdir('bisect')
+  return True
+
+def main():
+

[tonyg, 2013/02/19 18:01:25]

nit: extra line break

[shatch, 2013/02/19 18:33:51]

Done.

+  usage = ('%prog [options] [-- chromium-options]\n'
+           'Used by a trybot to run the bisection script using the parameters'
+           ' provided in the run-bisect-perf-regression.cfg file.\n')
+
+  parser = optparse.OptionParser(usage=usage)
+  parser.add_option('-w', '--working_directory',
+                    type='str',
+                    help='Path to the src directory of the git depot to start '
+                    'the bisection.')

[tonyg, 2013/02/19 18:01:25]

This description doesn't seem quite right. It is the path to create the src
directory, but it makes it sound like it should already exist.

[shatch, 2013/02/19 18:33:51]

Done.

+  (opts, args) = parser.parse_args()
+
+  if not opts.working_directory:
+    print 'Error: missing required parameter: --working_directory'
+    print
+    parser.print_help()
+    return 1
+
+  config = LoadConfigFile()
+  if not config:
+    print 'Error: Could not load config file.'
+    print
+    return 1
+
+  if not CreateAndChangeToSourceDirectory(opts.working_directory):
+    print 'Error: Could not create bisect directory.'
+    print
+    return 1
+
+  if not SetupGitDepot():

[tonyg, 2013/02/19 18:01:25]

Did you consider making the git checkout creation an optional part of the main
script instead of in this wrapper script. Then if QA has to run it manually for
some reason, it will be all self contained for them (they don't have chromium
checkouts typically).

[shatch, 2013/02/19 18:33:51]

Yeah I was a bit undecided on whether to put the functionality here or in the
main script. It just felt a little cleaner to separate out the functionality of
grabbing the depot from the actual bisection, but I don't have strong feelings
about it either way.

If QA were to run it, why could they not just run the wrapper with the
appropriate parameters?

[tonyg, 2013/02/19 19:45:57]

The thing about running the wrapper locally is that it is awkward to edit the
config file with the params instead of just passing them on the command line.

[shatch, 2013/02/19 23:01:32]

Done.

+    print 'Error: Failed to grab source.'
+    print
+    return 1
+
+  # The bisect script expects to be in /src
+  os.chdir(os.path.join(os.getcwd(), 'src'))
+
+  return RunBisectionScript(config)
+
+
+if __name__ == '__main__':
+  sys.exit(main())