DEPS roll script

tools/roll_deps.py

+#!/usr/bin/python2
+
+# Copyright 2014 Google Inc.
+#
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+
+"""
+Skia's Chromium DEPS roll script
+
+This script:
+- searches through the last N Skia git commits to find out the hash that is
+  associated with the SVN revision number.
+- creates a new branch in the Chromium tree, modifies the DEPS file to
+  point at the given Skia commit, commits, uploads to Rietveld, and
+  deletes the local copy of the branch.
+- creates a whitespace-only commit and uploads that to to Rietveld.
+- returns the Chromium tree to its previous state.
+
+Usage:
+  %prog -c CHROMIUM_PATH -r REVISION [OPTIONAL_OPTIONS]"""
+
+
+import optparse
+import os
+import re
+import subprocess
+import shutil
+import sys
+import tempfile
+
+
+def test_git(git):
+    """Test to see if the git executable can be run.
+
+    Args:
+        git: git executable.
+    Raises:
+        OSError on failure
+
+    """
+    with open(os.devnull, "w") as devnull:
+        subprocess.call([git, '--version'], stdout=devnull)
+
+
+def strip_output(*args, **kwargs):
+    """Wrap subprocess.check_output and str.strip()
+
+    Pass the given arguments into subprocess.check_output() and return
+    the results, after stripping any excess whitespace.
+
+    Returns:
+        a string without leading or trailing whitespace.

[borenet, 2014/01/06 14:06:46]

This may be obvious, but I might include "output of the process" somewhere here.

[hal.canary, 2014/01/06 18:27:57]

Done.

+    """
+    return str(subprocess.check_output(*args, **kwargs)).strip()
+
+
+def find_hash_from_revision(revision, search_depth, git):
+    """Finds the hash associated with a revision.
+
+    Searches through the last search_depth commits to find out the hash
+    that is associated with the SVN revision number.
+
+    Args:
+        revision: (int) SVN revision number.
+        search_depth: (int) Number of revisions to limit the search to.
+        git: git executable.
+
+    Returns:
+        Hash as a string.
+
+    Raises an exception on failure
+    """
+    skia_url = 'https://skia.googlesource.com/skia.git'

[borenet, 2014/01/06 14:06:46]

Would prefer that this be in a global variable.

[hal.canary, 2014/01/06 18:27:57]

Done.

+    temp_dir = tempfile.mkdtemp(prefix='git_skia_tmp_')
+    devnull = open(os.devnull, "w")
+    revision_format = 'http://skia.googlecode.com/svn/trunk@%d'

[borenet, 2014/01/06 14:06:46]

Ditto here.

[hal.canary, 2014/01/06 18:27:57]

Done.

+    revision_regex = re.compile(revision_format % revision)
+    try:
+        subprocess.check_call(
+            [git, 'clone', '--depth=%d' % search_depth, '--single-branch',
+             skia_url, temp_dir], stdout=devnull, stderr=devnull)

[borenet, 2014/01/06 14:06:46]

This isn't as expensive as it used to be, but maybe it would be worth giving the
option to supply a path to an existing git checkout.  Once we're fully switched
to git, this shouldn't be necessary at all, since this script will always be
sitting in a git checkout.

[hal.canary, 2014/01/06 18:27:57]

Done.

+        for i in xrange(search_depth):
+            commit = 'origin/master~%d' % i
+            output = subprocess.check_output(
+                [git, 'log', '-n', '1', '--format=format:%B', commit],
+                cwd=temp_dir, stderr=devnull)
+            if revision_regex.search(output):
+                return strip_output(
+                    [git, 'log', '-n', '1', '--format=format:%H', commit],
+                    cwd=temp_dir)
+    finally:
+        shutil.rmtree(temp_dir)
+        devnull.close()
+    raise Exception('Failed to find revision.')
+
+
+def fetch_origin(git):
+    """Call git fetch
+
+    Updates origin/master (via git fetch).  Leaves local tree alone.
+    Assumes current directory is a git repository.
+
+    Args:
+        git: git executable.
+    Returns:
+        the commit hash of origin/master
+    """
+    with open(os.devnull, "w") as devnull:
+        subprocess.check_call(
+            [git, 'fetch', 'origin'], stdout=devnull, stderr=devnull)
+    return strip_output([git, 'show-ref', 'origin/master', '--hash'])
+
+
+class GitBranchCLUpload(object):

[borenet, 2014/01/06 14:06:46]

This is a really elegant way of handling this.  +1

[hal.canary, 2014/01/06 18:27:57]

Thanks!

+    """
+    This class allows one to create a new branch in a repository based
+    off of origin/master, make changes to the tree inside the
+    with-block, upload that new branch to Rietveld, restore the original
+    tree state, and delete the local copy of the new branch.
+
+    See roll_deps() for an example of use.
+
+    Constructor Args:
+        message: the commit message.
+        file_list: list of files to pass to `git add`.
+        git: git executable.
+        set_brach_name: if not None, the name of the branch to use.
+            If None, then use a temporary branch that will be deleted.
+    """
+    # (Too few public methods) pylint: disable=I0011,R0903
+    def __init__(self, message, file_list, git, set_branch_name):

[borenet, 2014/01/06 14:06:46]

I'm not a big fan of having to pass the file_list in advance of actually
modifying the files.  I think it might be nice to add a modify(file) or
stage_for_commit(file) function which appends a file to file_list, so that the
list can be built on the fly.  

Alternatively, we could assume that everything on this branch which has changed
should be committed and (in lieu of doing "git add -u") we could "git add" the
changes between "git status" performed in __enter__ and __exit__.  If this is
intractable for some reason, a file_list is okay.

+        self._message = message
+        self._file_list = file_list
+        self._git = git
+        self._issue = None
+        self._branch_name = set_branch_name
+        self._stash = None
+        self._original_branch = None
+
+    def __enter__(self):
+        diff = subprocess.check_output([self._git, 'diff', '--shortstat'])
+        self._stash = (0 != len(diff))
+        if self._stash:
+            subprocess.check_call([self._git, 'stash', 'save'])
+        try:
+            self._original_branch = strip_output(
+                [self._git, 'symbolic-ref', '--short', 'HEAD'])
+        except (subprocess.CalledProcessError,):
+            self._original_branch = strip_output(
+                [self._git, 'rev-parse', 'HEAD'])
+
+        if not self._branch_name:
+            self._branch_name = 'autogenerated_deps_roll_branch'

[borenet, 2014/01/06 14:06:46]

Please put this in a default_branch_name variable or something similar.  And can
this default be set in the constructor?

[hal.canary, 2014/01/06 18:27:57]

Done.

+
+        try:
+            subprocess.check_call(
+                [self._git, 'checkout', '-b',
+                 self._branch_name, 'origin/master'])
+        except (subprocess.CalledProcessError,):
+            # Branch already exists.
+            subprocess.check_call([self._git, 'checkout', 'master'])
+            subprocess.check_call(
+                [self._git, 'branch', '-D', self._branch_name])
+            subprocess.check_call(
+                [self._git, 'checkout', '-b',
+                 self._branch_name, 'origin/master'])
+
+
+    def __exit__(self, etype, value, traceback):
+        for filename in self._file_list:
+            subprocess.check_call([self._git, 'add', filename])
+        subprocess.check_call([self._git, 'commit', '-m', self._message])

[borenet, 2014/01/06 14:06:46]

self._message probably needs to be double quoted and escaped, right?

[hal.canary, 2014/01/06 18:27:57]

Nope.  execvp doesn't require that.

+
+        environ = os.environ.copy()
+        if sys.platform != 'win32':
+            environ['GIT_EDITOR'] = ':'  # Bypass the editor
+        subprocess.check_call([self._git, 'cl', 'upload'], env=environ)
+
+        self._issue = strip_output([self._git, 'cl', 'issue'])
+
+        # deal with the aftermath of failed executions of this script.
+        if 'autogenerated_deps_roll_branch' == self._original_branch:
+            subprocess.check_call([self._git, 'checkout', 'master'])
+        else:
+            subprocess.check_call(
+                [self._git, 'checkout', self._original_branch])
+
+        if 'autogenerated_deps_roll_branch' == self._branch_name:
+            subprocess.check_call(
+                [self._git, 'branch', '-D', self._branch_name])
+        if self._stash:
+            subprocess.check_call([self._git, 'stash', 'pop'])
+
+    @property
+    def issue(self):
+        """
+        Returns:
+            a string describing the codereview issue, after __exit__
+            has been called.

[borenet, 2014/01/06 14:06:46]

Should this raise an exception if called before __exit__?

[hal.canary, 2014/01/06 18:27:57]

I thought about that (which was why I had it in a getter), but ended up just
documenting this behavior and removing the extra function.

+        """
+        return self._issue
+
+
+def change_skia_deps(revision, hashval, depspath):
+    """Update the DEPS file.
+
+    Modify the skia_revision and skia_hash entries in the given DEPS file.
+
+    Args:
+        revision: (int) Skia SVN revision.
+        hashval: (string) Skia Git hash.
+        depspath: (string) path to DEPS file.
+    """
+    temp_file = tempfile.NamedTemporaryFile(delete=False,
+                                            prefix='skia_DEPS_ROLL_tmp_')
+    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": "%d",' % revision
+        deps_regex_hash_repl = '"skia_hash": "%s",' % hashval
+
+        with open(depspath, 'r') as input_stream:
+            for line in input_stream:
+                line = deps_regex_rev.sub(deps_regex_rev_repl, line)
+                line = deps_regex_hash.sub(deps_regex_hash_repl, line)
+                temp_file.write(line)
+    finally:
+        temp_file.close()
+    shutil.move(temp_file.name, depspath)
+
+
+def roll_deps(revision, hashval, chromium_dir, save_branches, git):
+    """Upload changed DEPS and a whitespace change.
+
+    Given the correct hashval, create two Reitveld issues.    Returns a
+    tuple containing textual description of the two issues.
+
+    Args:
+        revision: (int) Skia SVN revision.
+        hashval: (string) Skia Git hash.
+        chromium_dir: (string) path to a local chromium git repository.
+        save_branches: (boolean) iff false, delete temprary branches.

[borenet, 2014/01/06 14:06:46]

"temporary"

[hal.canary, 2014/01/06 18:27:57]

Done.

+        git: (string) git executable.
+    """
+    cwd = os.getcwd()
+    os.chdir(chromium_dir)
+    try:
+        master_hash = fetch_origin(git)
+
+        message = 'roll skia DEPS to %d' % revision
+        branch = message.replace(' ','_') if save_branches else None
+        codereview = GitBranchCLUpload(message, ['DEPS'], git, branch)
+        with codereview:

[borenet, 2014/01/06 14:06:46]

Why not do this on one line:
with GitBranchCLUpload(message, ['DEPS'], git, branch) as codereview:
    ...

?

[hal.canary, 2014/01/06 18:27:57]

I tried that;  that syntax leaves codereview == None outside of the block.

+            change_skia_deps(revision, hashval, 'DEPS')
+        if save_branches:
+            deps_issue = '%s\n    branch: %s' % (codereview.issue, branch)
+        else:
+            deps_issue = codereview.issue
+
+        message = 'whitespace change %s' % master_hash[:8]    # Unique name

[borenet, 2014/01/06 14:06:46]

Could this point to deps_issue as well?  So that we know they're related?  It
would be nice to have each CL point to the other, but that would require
amending the CL description for one.

[hal.canary, 2014/01/06 18:27:57]

That sounds like a pain.

+        branch = message.replace(' ','_') if save_branches else None
+        codereview = GitBranchCLUpload(message, ['DEPS'], git, branch)
+        with codereview:
+            with open('DEPS', 'a') as output_stream:
+                output_stream.write('\n')
+        if save_branches:
+            whitespace_issue = '%s\n    branch: %s' % (
+                codereview.issue, branch)
+        else:
+            whitespace_issue = codereview.issue
+
+        return deps_issue, whitespace_issue
+    finally:
+        os.chdir(cwd)
+
+
+def find_hash_and_roll_deps(revision, chromium_dir, search_depth,
+                            save_branches, git):
+    """Call find_hash_from_revision() and roll_deps().
+
+    Args:
+        chromium_dir: (string) path to Chromium Git repository.
+        revision: (int) the Skia SVN revision number.
+        search_depth: (int) how far back to look for the revision.
+        git: (string) Git executable.
+        save_branches: (boolean) save the temporary branches.
+    """
+    hashval = find_hash_from_revision(revision, search_depth, git)
+    if not hashval:
+        raise Exception('failed to find revision')
+
+    print 'revision = @%d\nhash = %s\n' % (revision, hashval)
+
+    deps_issue, whitespace_issue = roll_deps(
+        revision, hashval, chromium_dir, save_branches, git)
+    print '\nDEPS roll:\n    %s\n' % deps_issue
+    print 'Whitespace change:\n    %s\n' % whitespace_issue
+
+
+def main(args):
+    """
+    main function; see module-level docstring and option_parser help.
+    """
+    option_parser = optparse.OptionParser(usage=__doc__)
+    # Anyone using this script on a regular basis should set the
+    # CHROMIUM_REPO_PATH environment variable.
+    option_parser.add_option(
+        '-c', '--chromium_path', help='Path to Chromium Git repository.',
+        default=os.environ.get('CHROMIUM_REPO_PATH'))
+    option_parser.add_option(
+        '-r', '--revision', help='The Skia SVN revision number', type="int")
+    option_parser.add_option(
+        '', '--search_depth', help='How far back to look for the revision',
+        type="int", default=100)
+    option_parser.add_option(
+        '', '--git_path', help='Git executable', default='git')
+    option_parser.add_option(
+        '', '--save_branches', help='Save the temporary branches',
+        action="store_true", dest="save_branches", default=False)
+
+    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.')
+    test_git(options.git_path)
+
+    find_hash_and_roll_deps(
+        options.revision, options.chromium_path, options.search_depth,
+        options.save_branches, options.git_path)
+
+
+if __name__ == '__main__':
+    main(sys.argv[1:])
+