Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
126523002:
Changes to roll_deps.py (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. |
DescriptionChanges to roll_deps.py
- Code cleanup
- Stop assuming that chromium's checkout would be via git-svn.
- Verbose commit message for the deps revision
- Shorter branch names.
- New default: save_branches = yes.
- New option: --git_hash=GIT_HASH
BUG=skia:1973
BUG=skia:1974
BUG=skia:1993
BUG=skia:1995
R=borenet@google.com
Committed: https://code.google.com/p/skia/source/detail?r=12974
Patch Set 1 : argh! #
Total comments: 14
Patch Set 2 : response to requested changes #
Total comments: 16
Patch Set 3 : again #Patch Set 4 : skia:1995 #Messages
Total messages: 9 (0 generated)
PTAL
Looks good, just a few comments. https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:492: def search_within_file(path, pattern, default=None): I think this function could live outside of roll_deps, even if it's only used here. Personally, I find that nested functions which are more than a couple of lines disrupt the visual flow and make things harder to read. This particular function seems like it could be useful within a separate, misc utility module. https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:497: first match. Please document that the pattern can't contain newlines. https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:520: def search_within_string(input_string, pattern, default=None): Same comment about this function: I think it could go at the module level. https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:546: branch = 'control_%s' % master_hash[:8] Maybe we should have a short_hash(hash) function? https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:566: assert revision != int(old_revision) Can we move the whitespace CL uploading to after the DEPS roll CL? Otherwise, we could leak whitespace CLs if this script is repeatedly run on up-to-date code.
A couple more... https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:133: action='store_false', dest='save_branches', default=True) If these are changing the same variable, why do we need both? Can we only include the flag that gives the non-default option? https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:561: branch = 'roll_%d_%s' % (revision, master_hash[:8]) I just found this: http://stackoverflow.com/a/6065944 Which gives some git branch naming conventions. Groups of branches piqued my interest... we could name the branches according to a scheme like this: branch = 'depsroll/%s/%s' % (revision, 'roll' or 'control') That way the branches would be organized into a convenient depsroll group, then divided by revision, so that the roll and control are paired. Thoughts?
ptal https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:133: action='store_false', dest='save_branches', default=True) On 2014/01/07 21:39:34, borenet wrote: > If these are changing the same variable, why do we need both? Can we only > include the flag that gives the non-default option? Done. https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:492: def search_within_file(path, pattern, default=None): On 2014/01/07 21:31:12, borenet wrote: > I think this function could live outside of roll_deps, even if it's only used > here. Personally, I find that nested functions which are more than a couple of > lines disrupt the visual flow and make things harder to read. This particular > function seems like it could be useful within a separate, misc utility module. Done. It's in a utility class. If you want to make a utility module, give us a second client for it. https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:497: first match. On 2014/01/07 21:31:12, borenet wrote: > Please document that the pattern can't contain newlines. Done. https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:520: def search_within_string(input_string, pattern, default=None): On 2014/01/07 21:31:12, borenet wrote: > Same comment about this function: I think it could go at the module level. Done. https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:546: branch = 'control_%s' % master_hash[:8] On 2014/01/07 21:31:12, borenet wrote: > Maybe we should have a short_hash(hash) function? In Python, string[:8] is clear enough. https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:561: branch = 'roll_%d_%s' % (revision, master_hash[:8]) On 2014/01/07 21:39:34, borenet wrote: > That way the branches would be organized into a convenient depsroll group, then > divided by revision, so that the roll and control are paired. Thoughts? Good idea, but defering to robert. https://codereview.chromium.org/126523002/diff/20001/tools/roll_deps.py#newco... tools/roll_deps.py:566: assert revision != int(old_revision) On 2014/01/07 21:31:12, borenet wrote: > Can we move the whitespace CL uploading to after the DEPS roll CL? Otherwise, > we could leak whitespace CLs if this script is repeatedly run on up-to-date > code. I moved that assert up. And turned it into a check that returns early when false.
https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:57: self.vsp = VerboseSubprocess(self.verbose) This could be named something more descriptive; even "subprocess" would probably be fine. https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:429: shutil.rmtree(skia_dir) I'm confused - won't this delete our checkout before we can use it? https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:436: print '~~$', 'cd', self._original_cwd Please put this before os.chdir so that we get the log info before any error which may occur. https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:460: class ReSearch(object): Now that you've split this into a class with only static methods, it might as well be its own module... https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:587: subprocess.check_call([git, 'stash', 'save']) Why not use your wrapper? https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:761: If set, revision is ignored. This and revision should default to None so that only one needs to be provided.
https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:321: '(?P<return>[0-9]+)') Unfortunately, we need to add another case: "LKGR w/ DEPS up to revision 243575"
https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:57: self.vsp = VerboseSubprocess(self.verbose) On 2014/01/08 19:20:21, borenet wrote: > This could be named something more descriptive; even "subprocess" would probably > be fine. That sounds like a terrible namespace collision. https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:321: '(?P<return>[0-9]+)') On 2014/01/08 19:27:40, borenet wrote: > Unfortunately, we need to add another case: > > "LKGR w/ DEPS up to revision 243575" Done. https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:429: shutil.rmtree(skia_dir) On 2014/01/08 19:20:21, borenet wrote: > I'm confused - won't this delete our checkout before we can use it? Done. https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:436: print '~~$', 'cd', self._original_cwd On 2014/01/08 19:20:21, borenet wrote: > Please put this before os.chdir so that we get the log info before any error > which may occur. Done. https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:460: class ReSearch(object): On 2014/01/08 19:20:21, borenet wrote: > Now that you've split this into a class with only static methods, it might as > well be its own module... As soon as someone else needs it, we can do that. https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:587: subprocess.check_call([git, 'stash', 'save']) On 2014/01/08 19:20:21, borenet wrote: > Why not use your wrapper? Done. https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:761: If set, revision is ignored. On 2014/01/08 19:20:21, borenet wrote: > This and revision should default to None so that only one needs to be provided. Done.
LGTM. Member variable rename optional https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py File tools/roll_deps.py (right): https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:57: self.vsp = VerboseSubprocess(self.verbose) On 2014/01/08 19:59:35, Hal Canary wrote: > On 2014/01/08 19:20:21, borenet wrote: > > This could be named something more descriptive; even "subprocess" would > probably > > be fine. > > That sounds like a terrible namespace collision. Not if you're using it with self.subprocess.* or config.subprocess.*, but if you'd like something else that's fine. "vsp" just isn't very descriptive. https://codereview.chromium.org/126523002/diff/110001/tools/roll_deps.py#newc... tools/roll_deps.py:321: '(?P<return>[0-9]+)') On 2014/01/08 19:59:35, Hal Canary wrote: > On 2014/01/08 19:27:40, borenet wrote: > > Unfortunately, we need to add another case: > > > > "LKGR w/ DEPS up to revision 243575" > > Done. Thanks.
Message was sent while issue was closed.
Committed patchset #4 manually as r12974 (presubmit successful). |
