Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
123523003:
DEPS roll script (Closed)
|
Created:
6 years, 11 months ago by hal.canary Modified:
6 years, 11 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionDEPS roll script
This script:
- searches through the last N commits to find out the hash that is
associated with the revision number.
- 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.
BUG=
R=borenet@google.com
Committed: https://code.google.com/p/skia/source/detail?r=12921
Patch Set 1 #
Total comments: 42
Patch Set 2 : lots of refactoring #
Total comments: 44
Patch Set 3 : changes #
Total comments: 23
Patch Set 4 : changes #
Total comments: 11
Patch Set 5 : again #
Total comments: 1
Patch Set 6 : spelling fix #Messages
Total messages: 20 (0 generated)
ptal
https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode1 tools/roll_deps.py:1: #!/usr/bin/python Lots of style nits, per http://google-styleguide.googlecode.com/svn/trunk/pyguide.html https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode3 tools/roll_deps.py:3: # Copyright 2013 Google Inc. 2014 https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode7 tools/roll_deps.py:7: Two spaces between top-level definitions. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode16 tools/roll_deps.py:16: skia_url = 'https://skia.googlesource.com/skia.git' Please capitalize these global variables. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode18 tools/roll_deps.py:18: def rollDeps(revision, chromium_dir, N=100): Prefer underscored function names: roll_deps https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode57 tools/roll_deps.py:57: raise Exception('failed to find revision') Please move this block into its own function, "revision_to_commit_hash" or some similar. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode63 tools/roll_deps.py:63: ################################################## Why are these surrounded by #'s? https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode68 tools/roll_deps.py:68: prefix='skia_DEPS_ROLL_tmp_') Please wrap everything down to "temp_file.close()" in a "try" and put the close statement in a "finally". https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode72 tools/roll_deps.py:72: deps_regex_rev_repl = '"skia_revision": "{}",'.format(revision) Prefer this syntax, here and elsewhere: deps_regex_rev_repl = '"skia_revision": "%d",' % revision https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode87 tools/roll_deps.py:87: deps_issue = subprocess.check_output([git, 'cl', 'issue']) Any way we can split some of the "branch/modify/commit/upload" process into functions? https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode102 tools/roll_deps.py:102: subprocess.check_call([git, 'cl', 'issue']) Did you mean: deps_issue = subprocess.check_output[git, 'cl', 'issue']) ? We need to keep track of both uploaded CLs https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode114 tools/roll_deps.py:114: revision = int(sys.argv[1]) No need to use int() if you use %d in the format string above. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode116 tools/roll_deps.py:116: rollDeps(revision, chromium_dir) Can just pass sys.argv directly into roll_deps without declaring additional variables.
https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode22 tools/roll_deps.py:22: - searches through the last N commits to find out the hash that is instead of apparent constant "N", perhaps num_commits ? https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode23 tools/roll_deps.py:23: associated with the revision number. I guess you mean "SVN revision number"? https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode28 tools/roll_deps.py:28: - create a whitespace-only commit and uploads that to to Rietveld. create -> creates to to -> to https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode112 tools/roll_deps.py:112: print 'useage {} REVISION_NUMBER CHROMIUM_DIR'.format(sys.argv[0]) useage -> usage https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode114 tools/roll_deps.py:114: revision = int(sys.argv[1]) On 2014/01/03 13:52:36, borenet wrote: > No need to use int() if you use %d in the format string above. Eric, which format string are you referring to? I don't follow. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode116 tools/roll_deps.py:116: rollDeps(revision, chromium_dir) On 2014/01/03 13:52:36, borenet wrote: > Can just pass sys.argv directly into roll_deps without declaring additional > variables. We could, but personally I prefer the way Hal wrote it originally. Makes roll_deps() more reusable...
https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode114 tools/roll_deps.py:114: revision = int(sys.argv[1]) On 2014/01/03 14:54:08, epoger wrote: > On 2014/01/03 13:52:36, borenet wrote: > > No need to use int() if you use %d in the format string above. > > Eric, which format string are you referring to? I don't follow. Line 66. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode116 tools/roll_deps.py:116: rollDeps(revision, chromium_dir) On 2014/01/03 14:54:08, epoger wrote: > On 2014/01/03 13:52:36, borenet wrote: > > Can just pass sys.argv directly into roll_deps without declaring additional > > variables. > > We could, but personally I prefer the way Hal wrote it originally. Makes > roll_deps() more reusable... 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?
https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode111 tools/roll_deps.py:111: if len(sys.argv) < 3: Please use something like optparse instead, eg: https://code.google.com/p/skia/source/browse/buildbot/compute_engine_scripts/...
https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode114 tools/roll_deps.py:114: revision = int(sys.argv[1]) On 2014/01/03 14:58:46, borenet wrote: > On 2014/01/03 14:54:08, epoger wrote: > > On 2014/01/03 13:52:36, borenet wrote: > > > No need to use int() if you use %d in the format string above. > > > > Eric, which format string are you referring to? I don't follow. > > Line 66. 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! https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode116 tools/roll_deps.py:116: rollDeps(revision, chromium_dir) On 2014/01/03 14:58:46, borenet wrote: > On 2014/01/03 14:54:08, epoger wrote: > > On 2014/01/03 13:52:36, borenet wrote: > > > Can just pass sys.argv directly into roll_deps without declaring additional > > > variables. > > > > We could, but personally I prefer the way Hal wrote it originally. Makes > > roll_deps() more reusable... > > 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? 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
PTAL - I've moved from a proof-of-concept to something that looks like a program. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode1 tools/roll_deps.py:1: #!/usr/bin/python On 2014/01/03 13:52:36, borenet wrote: > Lots of style nits, per > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode3 tools/roll_deps.py:3: # Copyright 2013 Google Inc. On 2014/01/03 13:52:36, borenet wrote: > 2014 Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode7 tools/roll_deps.py:7: On 2014/01/03 13:52:36, borenet wrote: > Two spaces between top-level definitions. Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode16 tools/roll_deps.py:16: skia_url = 'https://skia.googlesource.com/skia.git' On 2014/01/03 13:52:36, borenet wrote: > Please capitalize these global variables. Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode18 tools/roll_deps.py:18: def rollDeps(revision, chromium_dir, N=100): On 2014/01/03 13:52:36, borenet wrote: > Prefer underscored function names: roll_deps Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode22 tools/roll_deps.py:22: - searches through the last N commits to find out the hash that is On 2014/01/03 14:54:08, epoger wrote: > instead of apparent constant "N", perhaps num_commits ? Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode57 tools/roll_deps.py:57: raise Exception('failed to find revision') On 2014/01/03 13:52:36, borenet wrote: > Please move this block into its own function, "revision_to_commit_hash" or some > similar. Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode68 tools/roll_deps.py:68: prefix='skia_DEPS_ROLL_tmp_') On 2014/01/03 13:52:36, borenet wrote: > Please wrap everything down to "temp_file.close()" in a "try" and put the close > statement in a "finally". Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode87 tools/roll_deps.py:87: deps_issue = subprocess.check_output([git, 'cl', 'issue']) On 2014/01/03 13:52:36, borenet wrote: > Any way we can split some of the "branch/modify/commit/upload" process into > functions? Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode102 tools/roll_deps.py:102: subprocess.check_call([git, 'cl', 'issue']) On 2014/01/03 13:52:36, borenet wrote: > Did you mean: > deps_issue = subprocess.check_output[git, 'cl', 'issue']) > ? > > We need to keep track of both uploaded CLs Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode111 tools/roll_deps.py:111: if len(sys.argv) < 3: On 2014/01/03 15:03:56, rmistry wrote: > Please use something like optparse instead, eg: > https://code.google.com/p/skia/source/browse/buildbot/compute_engine_scripts/... Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode112 tools/roll_deps.py:112: print 'useage {} REVISION_NUMBER CHROMIUM_DIR'.format(sys.argv[0]) On 2014/01/03 14:54:08, epoger wrote: > useage -> usage Done.
[deferring to Eric for the first pass, unless you'd like me to go first Eric]
https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:5: # Use of this source code is governed by a BSD-style license that can be 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. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:23: ''' """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. """ https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:157: chromium_repo_path_env = os.environ['CHROMIUM_REPO_PATH'] if ( Use: chromium_repo_path_env = os.environ.get('CHROMIUM_REPO_PATH') get() returns None if the key doesn't exist.
Thanks for the updates - I like the modular functions much better. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:19: GIT = 'git' # Assume git is in your path. Change this otherwise. On the bots (and anywhere with depot_tools in the path) this ends up as 'git.bat' on Windows. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:26: ''' 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. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:27: SKIA_URL = 'https://skia.googlesource.com/skia.git' 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. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:29: devnull = open(os.devnull, "w") 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. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:34: raise Exception("Failed to grab a copy of Skia.") Why not use subprocess.check_call, which will raise this exception for you? https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:43: for line in proc.stdout: 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? https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:59: passed to restore_repository to reture to original state. sp: "return"? https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:65: subprocess.check_call([GIT, 'stash', 'save']) 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. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:67: 'HEAD']).strip() nit: indent https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:72: return master_hash.strip(), (old_branch, stash) I think it's a little confusing to return a tuple with a tuple nested inside it. Can we just return a 3-tuple? https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:75: def restore_repository(restore): Please add docstrings for this and the other functions. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:76: (old_branch, stash) = restore Why not take old_branch and stash as parameters to this function? https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:85: prefix='skia_DEPS_ROLL_tmp_') nit: indent https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:91: deps_regex_hash_repl = '"skia_hash": "{}",'.format(hashval) Same comment about the format strings as before... I find "%s" syntax easier to read: deps_regex_rev_repl = '"skia_revision": "%d",' % revision https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:97: temp_file.write(line) 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. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:105: 'HEAD']).strip() nit: indent https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:106: branch_name = 'tmp_%d' % random.randrange(1<<16) 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. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:109: args.extend(file_list) 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. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:135: message) nit: indent https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:158: 'CHROMIUM_REPO_PATH' in os.environ) else None 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') https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:179: options.search_depth) nit: indent
quite a bit of refactoring. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:5: # Use of this source code is governed by a BSD-style license that can be On 2014/01/03 16:46:20, jcgregorio wrote: > 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. Done. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:19: GIT = 'git' # Assume git is in your path. Change this otherwise. On 2014/01/03 18:15:48, borenet wrote: > On the bots (and anywhere with depot_tools in the path) this ends up as > 'git.bat' on Windows. I've made it an option. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:23: ''' On 2014/01/03 16:46:20, jcgregorio wrote: > """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. > """ Done. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:26: ''' On 2014/01/03 18:15:48, borenet wrote: > 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. Done. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:29: devnull = open(os.devnull, "w") On 2014/01/03 18:15:48, borenet wrote: > 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. That's why they introduced subprocess.DEVNULL! https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:34: raise Exception("Failed to grab a copy of Skia.") On 2014/01/03 18:15:48, borenet wrote: > Why not use subprocess.check_call, which will raise this exception for you? Done. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:43: for line in proc.stdout: On 2014/01/03 18:15:48, borenet wrote: > 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? In theory,the output could be very long, but in practice it is probably less than a kB. I'll use check_output. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:59: passed to restore_repository to reture to original state. On 2014/01/03 18:15:48, borenet wrote: > sp: "return"? Done. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:65: subprocess.check_call([GIT, 'stash', 'save']) On 2014/01/03 18:15:48, borenet wrote: > 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. This may be run by anyone who wants to force a DEPS roll. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:72: return master_hash.strip(), (old_branch, stash) On 2014/01/03 18:15:48, borenet wrote: > I think it's a little confusing to return a tuple with a tuple nested inside it. > Can we just return a 3-tuple? I've refactored. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:75: def restore_repository(restore): On 2014/01/03 18:15:48, borenet wrote: > Please add docstrings for this and the other functions. Done. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:91: deps_regex_hash_repl = '"skia_hash": "{}",'.format(hashval) On 2014/01/03 18:15:48, borenet wrote: > Same comment about the format strings as before... I find "%s" syntax easier to > read: > > deps_regex_rev_repl = '"skia_revision": "%d",' % revision I missed those. Done. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:106: branch_name = 'tmp_%d' % random.randrange(1<<16) On 2014/01/03 18:15:48, borenet wrote: > 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. Done. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:109: args.extend(file_list) On 2014/01/03 18:15:48, borenet wrote: > 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. Done. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:157: chromium_repo_path_env = os.environ['CHROMIUM_REPO_PATH'] if ( On 2014/01/03 16:46:20, jcgregorio wrote: > Use: > > chromium_repo_path_env = os.environ.get('CHROMIUM_REPO_PATH') > > get() returns None if the key doesn't exist. Done. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:158: 'CHROMIUM_REPO_PATH' in os.environ) else None On 2014/01/03 18:15:48, borenet wrote: > 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') Done. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:179: options.search_depth) On 2014/01/03 18:15:48, borenet wrote: > nit: indent Done.
It's getting there. I tried to follow the Google Python Style Guide as much as I could. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode23 tools/roll_deps.py:23: associated with the revision number. On 2014/01/03 14:54:08, epoger wrote: > I guess you mean "SVN revision number"? Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode28 tools/roll_deps.py:28: - create a whitespace-only commit and uploads that to to Rietveld. On 2014/01/03 14:54:08, epoger wrote: > create -> creates > to to -> to Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode63 tools/roll_deps.py:63: ################################################## On 2014/01/03 13:52:36, borenet wrote: > Why are these surrounded by #'s? Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode72 tools/roll_deps.py:72: deps_regex_rev_repl = '"skia_revision": "{}",'.format(revision) On 2014/01/03 13:52:36, borenet wrote: > Prefer this syntax, here and elsewhere: > deps_regex_rev_repl = '"skia_revision": "%d",' % revision Done. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode114 tools/roll_deps.py:114: revision = int(sys.argv[1]) On 2014/01/03 15:08:51, epoger wrote: > 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! I just asked the optparse module to require an int. https://codereview.chromium.org/123523003/diff/1/tools/roll_deps.py#newcode116 tools/roll_deps.py:116: rollDeps(revision, chromium_dir) On 2014/01/03 15:08:51, epoger wrote: > On 2014/01/03 14:58:46, borenet wrote: > > On 2014/01/03 14:54:08, epoger wrote: > > > On 2014/01/03 13:52:36, borenet wrote: > > > > Can just pass sys.argv directly into roll_deps without declaring > additional > > > > variables. > > > > > > We could, but personally I prefer the way Hal wrote it originally. Makes > > > roll_deps() more reusable... > > > > 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? > > 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 > I refactored to use optparse. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:27: SKIA_URL = 'https://skia.googlesource.com/skia.git' On 2014/01/03 18:15:48, borenet wrote: > 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. Let's figure out our use-case for modifying the url before fixing the script. It's easy enough to change it later. https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:76: (old_branch, stash) = restore On 2014/01/03 18:15:48, borenet wrote: > Why not take old_branch and stash as parameters to this function? (refactored away) https://codereview.chromium.org/123523003/diff/100001/tools/roll_deps.py#newc... tools/roll_deps.py:109: args.extend(file_list) On 2014/01/03 21:06:15, Hal Canary wrote: > On 2014/01/03 18:15:48, borenet wrote: > > 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. > > Done. Well, that broke the git submodules in some weird undefined way. Back to the file_list.
Looking good at patch set three. A few more comments... https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:54: a string without leading or trailing whitespace. This may be obvious, but I might include "output of the process" somewhere here. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:75: skia_url = 'https://skia.googlesource.com/skia.git' Would prefer that this be in a global variable. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:78: revision_format = 'http://skia.googlecode.com/svn/trunk@%d' Ditto here. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:83: skia_url, temp_dir], stdout=devnull, stderr=devnull) 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. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:116: class GitBranchCLUpload(object): This is a really elegant way of handling this. +1 https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:133: def __init__(self, message, file_list, git, set_branch_name): 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. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:155: self._branch_name = 'autogenerated_deps_roll_branch' Please put this in a default_branch_name variable or something similar. And can this default be set in the constructor? https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:174: subprocess.check_call([self._git, 'commit', '-m', self._message]) self._message probably needs to be double quoted and escaped, right? https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:201: has been called. Should this raise an exception if called before __exit__? https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:245: save_branches: (boolean) iff false, delete temprary branches. "temporary" https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:256: with codereview: Why not do this on one line: with GitBranchCLUpload(message, ['DEPS'], git, branch) as codereview: ... ? https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:263: message = 'whitespace change %s' % master_hash[:8] # Unique name 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.
I think it looks good, once you address Eric's comments. Thanks for your work on this, Hal!
A bunch of lint changes, documentation, a Config class (to keep number of function arguments down), stage_for_commit function in the GitBranchCLUpload class. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:54: a string without leading or trailing whitespace. On 2014/01/06 14:06:46, borenet wrote: > This may be obvious, but I might include "output of the process" somewhere here. Done. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:75: skia_url = 'https://skia.googlesource.com/skia.git' On 2014/01/06 14:06:46, borenet wrote: > Would prefer that this be in a global variable. Done. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:78: revision_format = 'http://skia.googlecode.com/svn/trunk@%d' On 2014/01/06 14:06:46, borenet wrote: > Ditto here. Done. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:83: skia_url, temp_dir], stdout=devnull, stderr=devnull) On 2014/01/06 14:06:46, borenet wrote: > 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. Done. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:116: class GitBranchCLUpload(object): On 2014/01/06 14:06:46, borenet wrote: > This is a really elegant way of handling this. +1 Thanks! https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:155: self._branch_name = 'autogenerated_deps_roll_branch' On 2014/01/06 14:06:46, borenet wrote: > Please put this in a default_branch_name variable or something similar. And can > this default be set in the constructor? Done. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:174: subprocess.check_call([self._git, 'commit', '-m', self._message]) On 2014/01/06 14:06:46, borenet wrote: > self._message probably needs to be double quoted and escaped, right? Nope. execvp doesn't require that. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:201: has been called. On 2014/01/06 14:06:46, borenet wrote: > Should this raise an exception if called before __exit__? 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. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:245: save_branches: (boolean) iff false, delete temprary branches. On 2014/01/06 14:06:46, borenet wrote: > "temporary" Done. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:256: with codereview: On 2014/01/06 14:06:46, borenet wrote: > Why not do this on one line: > with GitBranchCLUpload(message, ['DEPS'], git, branch) as codereview: > ... > > ? I tried that; that syntax leaves codereview == None outside of the block. https://codereview.chromium.org/123523003/diff/240001/tools/roll_deps.py#newc... tools/roll_deps.py:263: message = 'whitespace change %s' % master_hash[:8] # Unique name On 2014/01/06 14:06:46, borenet wrote: > 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. That sounds like a pain.
https://codereview.chromium.org/123523003/diff/340001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/340001/tools/roll_deps.py#newc... tools/roll_deps.py:34: class Config(object): This could be DepsRollConfig or something more descriptive. https://codereview.chromium.org/123523003/diff/340001/tools/roll_deps.py#newc... tools/roll_deps.py:99: 'win_layout_rel', I dislike hard-coded lists like this, but I don't think there's anything we can do about it. https://codereview.chromium.org/123523003/diff/340001/tools/roll_deps.py#newc... tools/roll_deps.py:114: # SKIA_GIT_REPO_PATH environment variable. For clarity, I think this should be "CHECKOUT" instead of "REPO" to indicate that this is a local checkout rather than the repository URL. https://codereview.chromium.org/123523003/diff/340001/tools/roll_deps.py#newc... tools/roll_deps.py:165: class Error(Exception): Please either name this something more specific or just use Exception throughout. https://codereview.chromium.org/123523003/diff/340001/tools/roll_deps.py#newc... tools/roll_deps.py:217: revision: (int) SVN revision number. If -1, use tip-of-tree Why not allow the default to be None? https://codereview.chromium.org/123523003/diff/340001/tools/roll_deps.py#newc... tools/roll_deps.py:243: if -1 == revision: Again, I'd really rather use None as a magic value than -1.
ptal https://codereview.chromium.org/123523003/diff/340001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/340001/tools/roll_deps.py#newc... tools/roll_deps.py:34: class Config(object): On 2014/01/06 19:04:28, borenet wrote: > This could be DepsRollConfig or something more descriptive. Done. https://codereview.chromium.org/123523003/diff/340001/tools/roll_deps.py#newc... tools/roll_deps.py:114: # SKIA_GIT_REPO_PATH environment variable. On 2014/01/06 19:04:28, borenet wrote: > For clarity, I think this should be "CHECKOUT" instead of "REPO" to indicate > that this is a local checkout rather than the repository URL. Done. https://codereview.chromium.org/123523003/diff/340001/tools/roll_deps.py#newc... tools/roll_deps.py:165: class Error(Exception): On 2014/01/06 19:04:28, borenet wrote: > Please either name this something more specific or just use Exception > throughout. Done. https://codereview.chromium.org/123523003/diff/340001/tools/roll_deps.py#newc... tools/roll_deps.py:217: revision: (int) SVN revision number. If -1, use tip-of-tree On 2014/01/06 19:04:28, borenet wrote: > Why not allow the default to be None? Done. https://codereview.chromium.org/123523003/diff/340001/tools/roll_deps.py#newc... tools/roll_deps.py:243: if -1 == revision: On 2014/01/06 19:04:28, borenet wrote: > Again, I'd really rather use None as a magic value than -1. Done.
LGTM with spelling nit. https://codereview.chromium.org/123523003/diff/390001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/123523003/diff/390001/tools/roll_deps.py#newc... tools/roll_deps.py:117: help='Path of a pure-git Skia repositry checkout. If empty,' "repository"
Message was sent while issue was closed.
Committed patchset #6 manually as r12921 (presubmit successful). |
