DEPS roll script

tools/roll_deps.py

+#!/usr/bin/python

[borenet, 2014/01/03 13:52:36]

Lots of style nits, per
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html

[hal.canary, 2014/01/03 16:33:30]

Done.

+
+# Copyright 2013 Google Inc.

[borenet, 2014/01/03 13:52:36]

2014

[hal.canary, 2014/01/03 16:33:30]

Done.

+#
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+

[borenet, 2014/01/03 13:52:36]

Two spaces between top-level definitions.

[hal.canary, 2014/01/03 16:33:30]

Done.

+import os
+import subprocess
+import shutil
+import sys
+import tempfile
+import re
+
+git = 'git'
+skia_url = 'https://skia.googlesource.com/skia.git'

[borenet, 2014/01/03 13:52:36]

Please capitalize these global variables.

[hal.canary, 2014/01/03 16:33:30]

Done.

+
+def rollDeps(revision, chromium_dir, N=100):

[borenet, 2014/01/03 13:52:36]

Prefer underscored function names: roll_deps

[hal.canary, 2014/01/03 16:33:30]

Done.

+    '''
+    This function:
+
+    -  searches through the last N commits to find out the hash that is

[epoger, 2014/01/03 14:54:08]

instead of apparent constant "N", perhaps num_commits ?

[hal.canary, 2014/01/03 16:33:30]

Done.

+       associated with the revision number.

[epoger, 2014/01/03 14:54:08]

I guess you mean "SVN revision number"?

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

Done.

+
+    -  creates a new branch in the chromium tree, modifies the DEPS
+       file, commits, and uploads to Rietveld.
+
+    -  create a whitespace-only commit and uploads that to to Rietveld.

[epoger, 2014/01/03 14:54:08]

create -> creates
to to -> to

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

Done.

+    '''
+    temp_dir = tempfile.mkdtemp(prefix='git_skia_tmp_')
+    devnull = open(os.devnull, "w")
+    if 0 != subprocess.call([
+            git, 'clone', '--depth={}'.format(N), '--single-branch',
+            skia_url, temp_dir], stdout=devnull, stderr=devnull):
+        raise Exception("Failed to grab a copy of Skia.")
+    for i in xrange(N):
+        proc = subprocess.Popen([
+                git, 'log', '-n', '1', '--format=format:%B',
+                'origin/master~{}'.format(i) ],
+                                cwd=temp_dir, stdout= subprocess.PIPE,
+                                stderr=devnull)
+        revision_regex = re.compile('@{}'.format(revision))
+        hashval = None
+        for line in proc.stdout:
+            if revision_regex.search(line) is not None:
+                proc.stdout.close()
+                proc = subprocess.Popen([
+                        git, 'log', '-n', '1', '--format=format:%H',
+                        'origin/master~{}'.format(i)],
+                                        cwd=temp_dir, stdout= subprocess.PIPE,
+                                        stderr=devnull)
+                hashval = proc.stdout.read().strip()
+                break
+        if hashval is not None:
+            break
+    if hashval is None:
+        raise Exception('failed to find revision')

[borenet, 2014/01/03 13:52:36]

Please move this block into its own function, "revision_to_commit_hash" or some
similar.

[hal.canary, 2014/01/03 16:33:30]

Done.

+    print revision, repr(hashval)
+    os.chdir(chromium_dir)
+    ##  Assume local tree is in good shape.
+    ##################################################
+    subprocess.check_call([git, 'checkout', 'master'])
+    ##################################################

[borenet, 2014/01/03 13:52:36]

Why are these surrounded by #'s?

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

Done.

+    subprocess.check_call([git, 'pull'])
+    subprocess.check_call([git, 'checkout', '-b',
+                           'roll_skia_DEPS_to_{}'.format(revision)])
+    temp_file = tempfile.NamedTemporaryFile(delete=False,
+                                            prefix='skia_DEPS_ROLL_tmp_')

[borenet, 2014/01/03 13:52:36]

Please wrap everything down to "temp_file.close()" in a "try" and put the close
statement in a "finally".

[hal.canary, 2014/01/03 16:33:30]

Done.

+    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)

[borenet, 2014/01/03 13:52:36]

Prefer this syntax, here and elsewhere:
deps_regex_rev_repl = '"skia_revision": "%d",' % revision

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

Done.

+    deps_regex_hash_repl = '"skia_hash": "{}",'.format(hashval)
+
+    with open('DEPS', '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)
+    temp_file.close()
+    shutil.copyfile(temp_file.name, 'DEPS')
+    subprocess.check_call([git, 'add', 'DEPS'])
+    message = 'roll skia DEPS to {}'.format(revision)
+    subprocess.check_call([git, 'commit', '-m', message])
+    subprocess.check_call([git, 'cl', 'upload',
+                           '-t', message, '-m', message])
+    deps_issue = subprocess.check_output([git, 'cl', 'issue'])

[borenet, 2014/01/03 13:52:36]

Any way we can split some of the "branch/modify/commit/upload" process into
functions?

[hal.canary, 2014/01/03 16:33:30]

Done.

+
+    ##################################################
+    subprocess.check_call([git, 'checkout', 'master'])
+    ##################################################
+
+    head = subprocess.check_output([git, 'show-ref', 'HEAD', '--hash'])
+    message = 'whitespace_change_{}'.format(head.strip()[:8])
+    subprocess.check_call([git, 'checkout', '-b', message])
+    with open('whitespace.txt', 'a') as o:
+        o.write('\n')
+    subprocess.check_call([git, 'add', 'whitespace.txt'])
+    subprocess.check_call([git, 'commit', '-m', message])
+    subprocess.check_call([git, 'cl', 'upload',
+                           '-t', message, '-m', message])
+    subprocess.check_call([git, 'cl', 'issue'])

[borenet, 2014/01/03 13:52:36]

Did you mean:
deps_issue = subprocess.check_output[git, 'cl', 'issue'])
?

We need to keep track of both uploaded CLs

[hal.canary, 2014/01/03 16:33:30]

Done.

+
+    print deps_issue
+
+    ##################################################
+    subprocess.check_call([git, 'checkout', 'master'])
+    ##################################################
+
+if __name__ == '__main__':
+    if len(sys.argv) < 3:

[rmistry, 2014/01/03 15:03:56]

Please use something like optparse instead, eg:
https://code.google.com/p/skia/source/browse/buildbot/compute_engine_scripts/...

[hal.canary, 2014/01/03 16:33:30]

Done.

+        print 'useage {} REVISION_NUMBER CHROMIUM_DIR'.format(sys.argv[0])

[epoger, 2014/01/03 14:54:08]

useage -> usage

[hal.canary, 2014/01/03 16:33:30]

Done.

+        exit(1)
+    revision = int(sys.argv[1])

[borenet, 2014/01/03 13:52:36]

No need to use int() if you use %d in the format string above.

[epoger, 2014/01/03 14:54:08]

Eric, which format string are you referring to?  I don't follow.

[borenet, 2014/01/03 14:58:46]

Line 66.

[epoger, 2014/01/03 15:08:51]

As far as I can tell, this would work as-is without the int().

But I think it's good to call int() here... you'll find out as soon as possible
if the user entered a nonnumeric value for the revision!

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

I just asked the optparse module to require an int.

+    chromium_dir = sys.argv[2]
+    rollDeps(revision, chromium_dir)

[borenet, 2014/01/03 13:52:36]

Can just pass sys.argv directly into roll_deps without declaring additional
variables.

[epoger, 2014/01/03 14:54:08]

We could, but personally I prefer the way Hal wrote it originally.  Makes
roll_deps() more reusable...

[borenet, 2014/01/03 14:58:46]

As far as I can tell, the only difference between what's written and this:

roll_deps(int(sys.argv[1]), sys.argv[2])

is that the way it's written, two unneeded global variables get defined.  How
does roll_deps become any more or less reusable?

[epoger, 2014/01/03 15:08:51]

I misunderstood.  I thought you were recommending this call roll_deps(sys.argv),
and changing roll_deps to take a single argv_array parameter.

roll_deps(int(sys.argv[1]), sys.argv[2])
or
roll_deps(revision=int(sys.argv[1]), chromium_dir=sys.argv[2])
SGTM

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

I refactored to use optparse.