|
|
Created:
9 years, 7 months ago by chrisha Modified:
9 years, 7 months ago CC:
sawbuck-changes_googlegroups.com Base URL:
http://sawbuck.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionCreate gitdeps.py script.
This emulates 'gclient' behaviour, but for 'deep' GIT dependencies. It will be necessary once Chrome has transitioned to a pure GIT checkout, as we will be unable to roll DEPS beyond that point.
BUG=
R=sebmarchand@chromium.org
Committed: https://code.google.com/p/sawbuck/source/detail?r=2274
Patch Set 1 #
Total comments: 62
Patch Set 2 : Addressed nits, added unittests. #Patch Set 3 : Better unittest comments. #
MessagesTotal messages: 6
This grew to be a sizable script in order to handle all of the edge cases, and to make it gracefully incremental and reentrant (ie: can be rerun after a failed run without botching everything up). A further CL will replace the appropriate DEPS entries with a GITDEPS file and a hook. PTAL?
Sign in to reply to this message.
a handful of nits ! (Sorry I should honor my newly acquired python readability). Most of them concern the comments, we're not really strict about them in our project, should we ? https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py File syzygy/build/gitdeps.py (right): https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:50: import shutil unused import. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:95: option_parser.error('Output directory does not exist: %s' % this call isn't consistent with the next one, here you use the % operator and not below. (I don't have any preference, pick the one you prefer). https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:108: class RepoOptions: s/:/(object):/ ? https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:110: def __new__(self): Missing BL before this def. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:111: self.repository = None Class method __new__ should have 'cls' or 'class_' as first argument. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:129: """Given a key/value from a DEPS file, generate a corresponding RepoOptions The style guide say that the doc string should start with a one-line summary... (physical line). https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:130: tuple. Arg section is missing as well as Returns and Raises. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:153: name = name + ' ' use += here. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:163: """Returns the full case-sensitive filename for the given path. If the path Ditto (for the one-line thing). https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:164: does not exist, returns the original input path as is. Missing Args and Returns section. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:166: pattern = "%s[%s]" % (name[:-1], name[-1]) s/"/'/. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:200: _GIT_CONFIG_SECTION_RE = re.compile('^\s*\[(.*?)\]\2*$') should you prefix those strings with a r ? (r'^\s*\[(.*?)\]\2*$')... https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:202: def _GetGitOrigin(path): Add 2 BL before function definition. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:228: """Given a path in a GIT repository, normalizes it so it will match that Document the arguments and the return value. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:235: path = path + '/' use += here. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:240: """Renames a checkout so that it can be subsequently deleted.""" Document args, return value and what can be raised here. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:250: except: Do you want to catch a particular class of exceptions ? https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:268: contents. Returns a tuple of (path, contents). Same thing for the one line comment and the arg/return value documentation. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:292: must not already exist. Ditto. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:339: dry_run=options.dry_run, stdout=None, stderr=None) Indent + 1 https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:343: _DIR_JUNCTION_RE = re.compile('^.*<JUNCTION>\s+(.+)\s+\[(.+)\]$') Same thing for the 'r' prefix. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:344: def _GetJunctionInfo(junction): Add 2 BL before function definition. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:345: """Returns the target of a junction, if it exists, None otherwise.""" Missing documentation. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:360: directory to the specified sub-directory of the GIT checkout. Same thing here (one line comment and doc) https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:372: if dest == None: s/==/is/ https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:392: """Installs a repository as configured by the options. Assumes that the Ditto. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:476: def _FindGlobalVariableInAstTree(tree, name, functions = {}): remove the WS around the =. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:477: """Finds and evaluates to global assignment of the variables |name| in the Ditto for the doc. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:484: def visit_BinOp(self, binop_node): Missing BL before def, weird method name (no camel case here?) https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:542: deps = _FindGlobalVariableInAstTree(tree, 'deps', functions = {'Var': _Var}) remove spaces around the =. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:580: for j in open(state_path, 'rb').readlines(): Use the default iterator here ?
Sign in to reply to this message.
PTAL? https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py File syzygy/build/gitdeps.py (right): https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:50: import shutil On 2014/08/21 18:37:29, Sébastien Marchand wrote: > unused import. Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:95: option_parser.error('Output directory does not exist: %s' % On 2014/08/21 18:37:29, Sébastien Marchand wrote: > this call isn't consistent with the next one, here you use the % operator and > not below. (I don't have any preference, pick the one you prefer). Ah, option_parser needs the %. It's 'logging' that takes extra arguments... https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:108: class RepoOptions: On 2014/08/21 18:37:30, Sébastien Marchand wrote: > s/:/(object):/ ? Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:110: def __new__(self): On 2014/08/21 18:37:31, Sébastien Marchand wrote: > Missing BL before this def. Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:111: self.repository = None On 2014/08/21 18:37:29, Sébastien Marchand wrote: > Class method __new__ should have 'cls' or 'class_' as first argument. Good catch, I wanted __init__. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:129: """Given a key/value from a DEPS file, generate a corresponding RepoOptions On 2014/08/21 18:37:30, Sébastien Marchand wrote: > The style guide say that the doc string should start with a one-line summary... > (physical line). We're not generating PyDocs, and these aren't externally visible functions (all start with leading _, which means private internal by convention). No need to be ultra thorough with the docs, like functions in anonymous namespaces in C++. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:130: tuple. On 2014/08/21 18:37:30, Sébastien Marchand wrote: > Arg section is missing as well as Returns and Raises. That's for 'external API' functions, much like we use Doxygen style comments for public member functions. No need to be so thorough with internal functions, as they're only used locally. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:153: name = name + ' ' On 2014/08/21 18:37:30, Sébastien Marchand wrote: > use += here. Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:163: """Returns the full case-sensitive filename for the given path. If the path On 2014/08/21 18:37:30, Sébastien Marchand wrote: > Ditto (for the one-line thing). Ignoring this. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:164: does not exist, returns the original input path as is. On 2014/08/21 18:37:30, Sébastien Marchand wrote: > Missing Args and Returns section. Ignoring this. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:166: pattern = "%s[%s]" % (name[:-1], name[-1]) On 2014/08/21 18:37:29, Sébastien Marchand wrote: > s/"/'/. Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:200: _GIT_CONFIG_SECTION_RE = re.compile('^\s*\[(.*?)\]\2*$') On 2014/08/21 18:37:30, Sébastien Marchand wrote: > should you prefix those strings with a r ? (r'^\s*\[(.*?)\]\2*$')... Doesn't really make a difference here, as \s and \2 are not valid escape sequences, so aren't translated. But used an 'r' to make this intent explicit. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:202: def _GetGitOrigin(path): On 2014/08/21 18:37:30, Sébastien Marchand wrote: > Add 2 BL before function definition. Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:228: """Given a path in a GIT repository, normalizes it so it will match that On 2014/08/21 18:37:29, Sébastien Marchand wrote: > Document the arguments and the return value. Ignored. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:235: path = path + '/' On 2014/08/21 18:37:30, Sébastien Marchand wrote: > use += here. Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:240: """Renames a checkout so that it can be subsequently deleted.""" On 2014/08/21 18:37:30, Sébastien Marchand wrote: > Document args, return value and what can be raised here. Ignored. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:250: except: On 2014/08/21 18:37:30, Sébastien Marchand wrote: > Do you want to catch a particular class of exceptions ? Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:268: contents. Returns a tuple of (path, contents). On 2014/08/21 18:37:31, Sébastien Marchand wrote: > Same thing for the one line comment and the arg/return value documentation. Ignored :) https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:292: must not already exist. On 2014/08/21 18:37:29, Sébastien Marchand wrote: > Ditto. Ditto. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:339: dry_run=options.dry_run, stdout=None, stderr=None) On 2014/08/21 18:37:30, Sébastien Marchand wrote: > Indent + 1 Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:343: _DIR_JUNCTION_RE = re.compile('^.*<JUNCTION>\s+(.+)\s+\[(.+)\]$') On 2014/08/21 18:37:30, Sébastien Marchand wrote: > Same thing for the 'r' prefix. Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:344: def _GetJunctionInfo(junction): On 2014/08/21 18:37:30, Sébastien Marchand wrote: > Add 2 BL before function definition. Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:345: """Returns the target of a junction, if it exists, None otherwise.""" On 2014/08/21 18:37:30, Sébastien Marchand wrote: > Missing documentation. Ignored. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:360: directory to the specified sub-directory of the GIT checkout. On 2014/08/21 18:37:29, Sébastien Marchand wrote: > Same thing here (one line comment and doc) Ignored :) https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:372: if dest == None: On 2014/08/21 18:37:29, Sébastien Marchand wrote: > s/==/is/ Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:392: """Installs a repository as configured by the options. Assumes that the On 2014/08/21 18:37:30, Sébastien Marchand wrote: > Ditto. Ignored. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:476: def _FindGlobalVariableInAstTree(tree, name, functions = {}): On 2014/08/21 18:37:30, Sébastien Marchand wrote: > remove the WS around the =. Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:477: """Finds and evaluates to global assignment of the variables |name| in the On 2014/08/21 18:37:29, Sébastien Marchand wrote: > Ditto for the doc. Ignored. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:484: def visit_BinOp(self, binop_node): On 2014/08/21 18:37:29, Sébastien Marchand wrote: > Missing BL before def, weird method name (no camel case here?) The name isn't chosen by me, I'm overriding methods in the ast.NodeTransformer base class. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:542: deps = _FindGlobalVariableInAstTree(tree, 'deps', functions = {'Var': _Var}) On 2014/08/21 18:37:29, Sébastien Marchand wrote: > remove spaces around the =. Done. https://codereview.appspot.com/123660043/diff/1/syzygy/build/gitdeps.py#newco... syzygy/build/gitdeps.py:580: for j in open(state_path, 'rb').readlines(): On 2014/08/21 18:37:30, Sébastien Marchand wrote: > Use the default iterator here ? Done.
Sign in to reply to this message.
lgtm.
Sign in to reply to this message.
Thanks, committing!
Sign in to reply to this message.
|