From 0bcfd18035fee8f29247aac766cbfbfe7e57c6cd Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Mon, 10 Oct 2011 20:06:09 +0000 Subject: [PATCH] Move all mutations into a specific place. This makes coherency checks more consistent for DEPS vs .gclient. It's still not thread safe but the mutation code paths are easier to follow. R=dpranke@chromium.org BUG= TEST= Review URL: http://codereview.chromium.org/8135019 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@104772 0039d316-1c4b-4281-b951-d872f2087c98 --- gclient.py | 92 +++++++++++++++++++++++++------------------ tests/gclient_test.py | 50 ++++++++++++----------- 2 files changed, 81 insertions(+), 61 deletions(-) diff --git a/gclient.py b/gclient.py index ed64354f1..5f40f4ed9 100644 --- a/gclient.py +++ b/gclient.py @@ -258,8 +258,31 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): # This dependency had its hook run self._hooks_ran = False - # Setup self.requirements and find any other dependency who would have self - # as a requirement. + if not self.name and self.parent: + raise gclient_utils.Error('Dependency without name') + + def setup_requirements(self): + """Setup self.requirements and find any other dependency who would have self + as a requirement. + + Returns True if this entry should be added, False if it is a duplicate of + another entry. + """ + if self.name in [s.name for s in self.parent.dependencies]: + raise gclient_utils.Error( + 'The same name "%s" appears multiple times in the deps section' % + self.name) + if self.should_process: + siblings = [d for d in self.root.subtree(False) if d.name == self.name] + for sibling in siblings: + if self.url != sibling.url: + raise gclient_utils.Error( + 'Dependency %s specified more than once:\n %s\nvs\n %s' % + (self.name, sibling.hierarchy(), self.hierarchy())) + # In theory we could keep it as a shadow of the other one. In + # practice, simply ignore it. + logging.warn('Won\'t process duplicate dependency %s' % sibling) + return False # self.parent is implicitly a requirement. This will be recursive by # definition. @@ -298,9 +321,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): # Step 2: Find any requirements self may impose. if obj.name.startswith(posixpath.join(self.name, '')): obj.add_requirement(self.name) - - if not self.name and self.parent: - raise gclient_utils.Error('Dependency without name') + return True def LateOverride(self, url): """Resolves the parsed url from url. @@ -416,8 +437,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): else: deps.update(os_deps) - self._deps_hooks.extend(local_scope.get('hooks', [])) - # If a line is in custom_deps, but not in the solution, we want to append # this line to the solution. for d in self.custom_deps: @@ -437,31 +456,21 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): deps = rel_deps # Convert the deps into real Dependency. + deps_to_add = [] for name, url in deps.iteritems(): - if name in [s.name for s in self._dependencies]: - raise gclient_utils.Error( - 'The same name "%s" appears multiple times in the deps section' % - name) should_process = self.recursion_limit and self.should_process - if should_process: - tree = dict((d.name, d) for d in self.root.subtree(False)) - if name in tree: - if url == tree[name].url: - logging.info('Won\'t process duplicate dependency %s' % tree[name]) - # In theory we could keep it as a shadow of the other one. In - # practice, simply ignore it. - #should_process = False - continue - else: - raise gclient_utils.Error( - 'Dependency %s specified more than once:\n %s\nvs\n %s' % - (name, tree[name].hierarchy(), self.hierarchy())) - self._dependencies.append( - Dependency( - self, name, url, None, None, None, None, - self.deps_file, should_process)) - self._deps_parsed = True - logging.debug('Loaded: %s' % str(self)) + deps_to_add.append(Dependency( + self, name, url, None, None, None, None, + self.deps_file, should_process)) + self.add_dependencies_and_close(deps_to_add, local_scope.get('hooks', [])) + logging.info('ParseDepsFile(%s) done' % self.name) + + def add_dependencies_and_close(self, deps_to_add, hooks): + """Adds the dependencies, hooks and mark the parsing as done.""" + for dep in deps_to_add: + if dep.setup_requirements(): + self.add_dependency(dep) + self._mark_as_parsed(hooks) # Arguments number differs from overridden method # pylint: disable=W0221 @@ -632,7 +641,17 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): if j.should_process: yield j + @gclient_utils.lockedmethod + def add_dependency(self, new_dep): + self._dependencies.append(new_dep) + + @gclient_utils.lockedmethod + def _mark_as_parsed(self, new_hooks): + self._deps_hooks.extend(new_hooks) + self._deps_parsed = True + @property + @gclient_utils.lockedmethod def dependencies(self): return tuple(self._dependencies) @@ -765,13 +784,11 @@ solutions = [ exec(content, config_dict) except SyntaxError, e: gclient_utils.SyntaxErrorToError('.gclient', e) + + deps_to_add = [] for s in config_dict.get('solutions', []): try: - tree = dict((d.name, d) for d in self.root.subtree(False)) - if s['name'] in tree: - raise gclient_utils.Error( - 'Dependency %s specified more than once in .gclient' % s['name']) - self._dependencies.append(Dependency( + deps_to_add.append(Dependency( self, s['name'], s['url'], s.get('safesync_url', None), s.get('managed', True), @@ -782,9 +799,8 @@ solutions = [ except KeyError: raise gclient_utils.Error('Invalid .gclient file. Solution is ' 'incomplete: %s' % s) - # .gclient can have hooks. - self._deps_hooks = config_dict.get('hooks', []) - self._deps_parsed = True + self.add_dependencies_and_close(deps_to_add, config_dict.get('hooks', [])) + logging.info('SetConfig() done') def SaveConfig(self): gclient_utils.FileWrite(os.path.join(self.root_dir, diff --git a/tests/gclient_test.py b/tests/gclient_test.py index d883fc761..a3b0c115b 100755 --- a/tests/gclient_test.py +++ b/tests/gclient_test.py @@ -212,33 +212,37 @@ class GclientTest(trial_dir.TestCase): self.assertEquals('proto://host/path@revision', d.url) def testStr(self): - # Make sure __str__() works fine. - # pylint: disable=W0212 parser = gclient.Parser() options, _ = parser.parse_args([]) obj = gclient.GClient('foo', options) - obj._dependencies.append( - gclient.Dependency(obj, 'foo', 'url', None, None, None, None, 'DEPS', - True)) - obj._dependencies.append( - gclient.Dependency(obj, 'bar', 'url', None, None, None, None, 'DEPS', - True)) - obj.dependencies[0]._dependencies.append( - gclient.Dependency( - obj.dependencies[0], 'foo/dir1', 'url', None, None, None, None, - 'DEPS', True)) - obj.dependencies[0]._dependencies.append( - gclient.Dependency( - obj.dependencies[0], 'foo/dir2', - gclient.GClientKeywords.FromImpl('bar'), None, None, None, None, - 'DEPS', True)) - obj.dependencies[0]._dependencies.append( - gclient.Dependency( - obj.dependencies[0], 'foo/dir3', - gclient.GClientKeywords.FileImpl('url'), None, None, None, None, - 'DEPS', True)) + obj.add_dependencies_and_close( + [ + gclient.Dependency( + obj, 'foo', 'url', None, None, None, None, 'DEPS', True), + gclient.Dependency( + obj, 'bar', 'url', None, None, None, None, 'DEPS', True), + ], + []) + obj.dependencies[0].add_dependencies_and_close( + [ + gclient.Dependency( + obj.dependencies[0], 'foo/dir1', 'url', None, None, None, None, + 'DEPS', True), + gclient.Dependency( + obj.dependencies[0], 'foo/dir2', + gclient.GClientKeywords.FromImpl('bar'), None, None, None, None, + 'DEPS', True), + gclient.Dependency( + obj.dependencies[0], 'foo/dir3', + gclient.GClientKeywords.FileImpl('url'), None, None, None, None, + 'DEPS', True), + ], + []) + # Make sure __str__() works fine. + # pylint: disable=W0212 obj.dependencies[0]._file_list.append('foo') - self.assertEquals(434, len(str(obj)), '%d\n%s' % (len(str(obj)), str(obj))) + str_obj = str(obj) + self.assertEquals(472, len(str_obj), '%d\n%s' % (len(str_obj), str_obj)) if __name__ == '__main__':