DEPS roll script

tools/roll_deps.py

+#!/usr/bin/python
+
+# Copyright 2014 Google Inc.
+#
+# Use of this source code is governed by a BSD-style license that can be

[jcgregorio, 2014/01/03 16:46:20]

Needs a longer description of what this script does and how it is to be used,
can either be here in the comments, or in the usage string.

[hal.canary, 2014/01/03 21:06:15]

Done.

+# found in the LICENSE file.
+
+
+import optparse
+import os
+import random
+import re
+import subprocess
+import shutil
+import sys
+import tempfile
+
+
+GIT = 'git'  # Assume git is in your path.  Change this otherwise.

[borenet, 2014/01/03 18:15:48]

On the bots (and anywhere with depot_tools in the path) this ends up as
'git.bat' on Windows.

[hal.canary, 2014/01/03 21:06:15]

I've made it an option.

+
+
+def find_hash_from_revision(revision, search_depth):
+  '''

[jcgregorio, 2014/01/03 16:46:20]

"""Finds the hash associated with a revision.

Searches through the last N commits to find out the hash that is
associated with the revision number.

Args:
  revision: Integer revision number.
  search_depth: Number of revisions to limit the search to.

Returns:
  Hash as a string.
"""

[hal.canary, 2014/01/03 21:06:15]

Done.

+  Searches through the last N commits to find out the hash that is
+  associated with the revision number.
+  '''

[borenet, 2014/01/03 18:15:48]

Please update your docstrings to follow this format:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen...

Specifically, use double quotes, and document the type and meaning of the
arguments.

[hal.canary, 2014/01/03 21:06:15]

Done.

+  SKIA_URL = 'https://skia.googlesource.com/skia.git'

[borenet, 2014/01/03 18:15:48]

This can go outside, at the module level.  There's probably a nice way to share
this between our checked-in python scripts; we already define it on the buildbot
side, so it's available using buildbot_globals.py, but that requires hitting the
repository to read the checked-in global_variables.json file, so probably not
worth using it here.

[hal.canary, 2014/01/03 23:58:48]

Let's figure out our use-case for modifying the url before fixing the script. 
It's easy enough to change it later.

+  temp_dir = tempfile.mkdtemp(prefix='git_skia_tmp_')
+  devnull = open(os.devnull, "w")

[borenet, 2014/01/03 18:15:48]

Is this just to avoid seeing output from the subprocess?  In Python 3.3, there's
a subprocess.DEVNULL to which you can redirect.  I guess we don't want to see
output from these commands, but this seems messy.

[hal.canary, 2014/01/03 21:06:15]

That's why they introduced subprocess.DEVNULL!

+  try:
+    if 0 != subprocess.call([
+        GIT, 'clone', '--depth=%d' % search_depth, '--single-branch',
+        SKIA_URL, temp_dir], stdout=devnull, stderr=devnull):
+      raise Exception("Failed to grab a copy of Skia.")

[borenet, 2014/01/03 18:15:48]

Why not use subprocess.check_call, which will raise this exception for you?

[hal.canary, 2014/01/03 21:06:15]

Done.

+    for i in xrange(search_depth):
+      commit = 'origin/master~%d' % i
+      proc = subprocess.Popen([
+          GIT, 'log', '-n', '1', '--format=format:%B', commit],
+                  cwd=temp_dir, stdout=subprocess.PIPE,
+                  stderr=devnull)
+      revision_format = 'http://skia.googlecode.com/svn/trunk@%d'
+      revision_regex = re.compile(revision_format % revision)
+      for line in proc.stdout:

[borenet, 2014/01/03 18:15:48]

I think it's a good idea to use proc.wait() before attempting to read its
output, and I think it's safer to get the output using proc.communicate()[0]. 
Is there any reason not to use check_output instead?

[hal.canary, 2014/01/03 21:06:15]

In theory,the output could be very long, but in practice it is probably less
than a kB.  I'll use check_output.

+        if revision_regex.search(line) is not None:
+          proc.stdout.close()
+          return subprocess.check_output([
+              GIT, 'log', '-n', '1', '--format=format:%H',
+              commit], cwd=temp_dir).strip()
+  finally:
+    shutil.rmtree(temp_dir)
+    devnull.close()
+  raise Exception('Failed to find revision.')
+
+
+def checkout_master():
+  '''
+  Stashes current changes if necessary, checkout master, pulls from
+  origin, and returns hash of current revision, plus arguments to be
+  passed to restore_repository to reture to original state.

[borenet, 2014/01/03 18:15:48]

sp: "return"?

[hal.canary, 2014/01/03 21:06:15]

Done.

+  '''
+  # Assumes the current directory is a git repository and master is
+  # tracking an upstream branch.
+  stash = (0 != len(subprocess.check_output([GIT, 'diff', '--shortstat'])))
+  if stash:
+    subprocess.check_call([GIT, 'stash', 'save'])

[borenet, 2014/01/03 18:15:48]

Since this is (mostly) going to be run by bots, I think it's okay to error out
here and not worry about whether local edits need to be stashed.

[hal.canary, 2014/01/03 21:06:15]

This may be run by anyone who wants to force a DEPS roll.

+  old_branch = subprocess.check_output([GIT, 'symbolic-ref', '--short',
+                      'HEAD']).strip()

[borenet, 2014/01/03 18:15:48]

nit: indent 

+  if old_branch != 'master':
+    subprocess.check_call([GIT, 'checkout', 'master'])
+  subprocess.check_call([GIT, 'pull'])
+  master_hash = subprocess.check_output([GIT, 'show-ref', 'HEAD', '--hash'])
+  return master_hash.strip(), (old_branch, stash)

[borenet, 2014/01/03 18:15:48]

I think it's a little confusing to return a tuple with a tuple nested inside it.
 Can we just return a 3-tuple?

[hal.canary, 2014/01/03 21:06:15]

I've refactored.

+
+
+def restore_repository(restore):

[borenet, 2014/01/03 18:15:48]

Please add docstrings for this and the other functions.

[hal.canary, 2014/01/03 21:06:15]

Done.

+  (old_branch, stash) = restore

[borenet, 2014/01/03 18:15:48]

Why not take old_branch and stash as parameters to this function?

[hal.canary, 2014/01/03 23:58:48]

(refactored away)

+  if old_branch != 'master':
+    subprocess.check_call([GIT, 'checkout', old_branch])
+  if stash:
+    subprocess.check_call([GIT, 'stash', 'pop'])
+
+
+def change_skia_deps(revision, hashval, depspath):
+  temp_file = tempfile.NamedTemporaryFile(delete=False,
+                      prefix='skia_DEPS_ROLL_tmp_')

[borenet, 2014/01/03 18:15:48]

nit: indent

+  try:
+    deps_regex_rev = re.compile('"skia_revision": "[0-9]*",')
+    deps_regex_hash = re.compile('"skia_hash": "[0-9a-f]*",')
+
+    deps_regex_rev_repl = '"skia_revision": "{}",'.format(revision)
+    deps_regex_hash_repl = '"skia_hash": "{}",'.format(hashval)

[borenet, 2014/01/03 18:15:48]

Same comment about the format strings as before... I find "%s" syntax easier to
read:

deps_regex_rev_repl = '"skia_revision": "%d",' % revision

[hal.canary, 2014/01/03 21:06:15]

I missed those.  Done.

+
+    with open(depspath, 'r') as f:
+      for line in f:
+        line = deps_regex_rev.sub(deps_regex_rev_repl, line)
+        line = deps_regex_hash.sub(deps_regex_hash_repl, line)
+        temp_file.write(line)

[borenet, 2014/01/03 18:15:48]

Optional: You can entirely avoid using a temporary file by just keeping the DEPS
file contents as a list of lines in memory and writing it back to the DEPS file
at the end.  It's not a big file.

+  finally:
+    temp_file.close()
+  shutil.move(temp_file.name, depspath)
+
+
+def git_branch_add_commit_cl_upload(file_list, message):
+  old_branch = subprocess.check_output([GIT, 'symbolic-ref', '--short',
+                      'HEAD']).strip()

[borenet, 2014/01/03 18:15:48]

nit: indent

+  branch_name = 'tmp_%d' % random.randrange(1<<16)

[borenet, 2014/01/03 18:15:48]

A timestamp would work well here.  Alternatively, you can reserve some branch
name to be used only by this script, like "autogenerated_deps_roll_branch" or
something that we can reasonably expect not to have collisions.

[hal.canary, 2014/01/03 21:06:15]

Done.

+  subprocess.check_call([GIT, 'checkout', '-b', branch_name])
+  args = [GIT, 'add']
+  args.extend(file_list)

[borenet, 2014/01/03 18:15:48]

Since we're on a clean branch, it's probably okay to just run "git add -u" and
not require a file list to be passed around.

[hal.canary, 2014/01/03 21:06:15]

Done.

[hal.canary, 2014/01/03 23:58:48]

Well, that broke the git submodules in some weird undefined way.  Back to the
file_list.

+  subprocess.check_call(args)
+  subprocess.check_call([GIT, 'commit', '-m', message])
+  subprocess.check_call([GIT, 'cl', 'upload'])
+  issue = subprocess.check_output([GIT, 'cl', 'issue']).strip()
+  subprocess.check_call([GIT, 'checkout', old_branch])
+  subprocess.check_call([GIT, 'branch', '-D', branch_name])
+  return issue
+
+
+def add_whitespace_change(file_name):
+  with open(file_name, 'a') as o:
+    o.write('\n')
+
+
+def roll_deps(revision, hashval, chromium_dir):
+  os.chdir(chromium_dir)
+
+  master_hash, restore = checkout_master()
+  message = 'roll skia DEPS to %d' % revision
+  change_skia_deps(revision, hashval, 'DEPS')
+  deps_issue = git_branch_add_commit_cl_upload(['DEPS'], message)
+
+  message = 'whitespace change %s' % master_hash[:8]  # Unique name
+  add_whitespace_change('whitespace.txt')
+  whitespace_issue = git_branch_add_commit_cl_upload(['whitespace.txt'],
+                        message)

[borenet, 2014/01/03 18:15:48]

nit: indent

+  restore_repository(restore)
+  print '\n'
+  print 'DEPS roll:\n  %s\n' % deps_issue
+  print 'Whitespace change:\n  %s\n' % whitespace_issue
+
+
+def find_hash_and_roll_deps(revision, chromium_dir, search_depth):
+  hashval = find_hash_from_revision(revision, search_depth)
+  if hashval is None:
+    raise Exception('failed to find revision')
+
+  print 'revision=@%d hash=%s\n' % (revision, hashval)
+
+  roll_deps(revision, hashval, chromium_dir)
+
+
+def main(args):
+  usage = 'Usage: %prog -c CHROMIUM_PATH -r REVISION'
+
+  # Anyone using this script on a regular basis should set this
+  # environment variable.
+  chromium_repo_path_env = os.environ['CHROMIUM_REPO_PATH'] if (

[jcgregorio, 2014/01/03 16:46:20]

Use:

  chromium_repo_path_env = os.environ.get('CHROMIUM_REPO_PATH')

get() returns None if the key doesn't exist.

[hal.canary, 2014/01/03 21:06:15]

Done.

+    'CHROMIUM_REPO_PATH' in os.environ) else None

[borenet, 2014/01/03 18:15:48]

Python supports defaults in dict retrieval:
chromium_repo_path_env = os.environ.get('CHROMIUM_REPO_PATH', default)

And the default is None, so you can just do:
chromium_repo_path_env = os.environ.get('CHROMIUM_REPO_PATH')

[hal.canary, 2014/01/03 21:06:15]

Done.

+
+  option_parser = optparse.OptionParser(usage=usage)
+  option_parser.add_option(
+    '-c', '--chromium_path', help='Path to Chromium Git repository.',
+    default=chromium_repo_path_env)
+  option_parser.add_option(
+    '-r', '--revision', help='The Skia revision number', type="int")
+  option_parser.add_option(
+    '', '--search_depth', help='How far back to look for the revision',
+    type="int", default=100)
+  options = option_parser.parse_args(args)[0]
+
+  if (not options.revision and not options.chromium_path):
+    option_parser.error('Must specify revision and chromium_path.')
+  if (not options.revision):
+    option_parser.error('Must specify revision.')
+  if (not options.chromium_path):
+    option_parser.error('Must specify chromium_path.')
+
+  find_hash_and_roll_deps(options.revision, options.chromium_path,
+              options.search_depth)

[borenet, 2014/01/03 18:15:48]

nit: indent

[hal.canary, 2014/01/03 21:06:15]

Done.

+
+
+if __name__ == '__main__':
+  main(sys.argv[1:])
+