From 118fb1cdcb74b05f57e0a052643f929f5491fe4d Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Thu, 1 Sep 2011 20:04:24 +0000 Subject: [PATCH] Fix parallelization of multiple solutions once for all. Verify correctness with a unit test written explicitly to verify all corner cases. BUG=60725 TEST=new unit test Review URL: http://codereview.chromium.org/6598087 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@99234 0039d316-1c4b-4281-b951-d872f2087c98 --- gclient.py | 69 +++++++++++++++++++++++++++++++++---------- tests/gclient_test.py | 40 ++++++++++++++++++++----- 2 files changed, 86 insertions(+), 23 deletions(-) diff --git a/gclient.py b/gclient.py index d86c3663e2..e2257724e5 100644 --- a/gclient.py +++ b/gclient.py @@ -141,6 +141,9 @@ class Dependency(GClientKeywords, gclient_utils.WorkItem): def __init__(self, parent, name, url, safesync_url, custom_deps, custom_vars, deps_file, should_process): + # Warning: this function can be called from any thread. Both + # self.dependencies and self.requirements are read and modified from + # multiple threads at the same time. Sad. GClientKeywords.__init__(self) gclient_utils.WorkItem.__init__(self) self.parent = parent @@ -167,11 +170,9 @@ class Dependency(GClientKeywords, gclient_utils.WorkItem): # This dependency had its hook run self.hooks_ran = False # Required dependencies to run before running this one: - self.requirements = [] - if self.parent and self.parent.name: - self.requirements.append(self.parent.name) - if isinstance(self.url, self.FromImpl): - self.requirements.append(self.url.module_name) + self.requirements = set() + + self._FindDependencies() # Sanity checks if not self.name and self.parent: @@ -185,6 +186,54 @@ class Dependency(GClientKeywords, gclient_utils.WorkItem): raise gclient_utils.Error('deps_file name must not be a path, just a ' 'filename. %s' % self.deps_file) + def _FindDependencies(self): + """Setup self.requirements and find any other dependency who would have self + as a requirement. + """ + # self.parent is implicitly a requirement. This will be recursive by + # definition. + if self.parent and self.parent.name: + self.requirements.add(self.parent.name) + + # For a tree with at least 2 levels*, the leaf node needs to depend + # on the level higher up in an orderly way. + # This becomes messy for >2 depth as the DEPS file format is a dictionary, + # thus unsorted, while the .gclient format is a list thus sorted. + # + # * _recursion_limit is hard coded 2 and there is no hope to change this + # value. + # + # Interestingly enough, the following condition only works in the case we + # want: self is a 2nd level node. 3nd level node wouldn't need this since + # they already have their parent as a requirement. + if self.parent in self.root_parent().dependencies: + root_deps = self.root_parent().dependencies + for i in range(0, root_deps.index(self.parent)): + value = root_deps[i] + if value.name: + self.requirements.add(value.name) + + if isinstance(self.url, self.FromImpl): + self.requirements.add(self.url.module_name) + + if self.name: + def yield_full_tree(root): + """Depth-first recursion.""" + yield root + for i in root.dependencies: + for j in yield_full_tree(i): + yield j + + for obj in yield_full_tree(self.root_parent()): + if obj is self or not obj.name: + continue + # Step 1: Find any requirements self may need. + if self.name.startswith(posixpath.join(obj.name, '')): + self.requirements.add(obj.name) + # Step 2: Find any requirements self may impose. + if obj.name.startswith(posixpath.join(self.name, '')): + obj.requirements.add(self.name) + def LateOverride(self, url): """Resolves the parsed url from url. @@ -406,16 +455,6 @@ class Dependency(GClientKeywords, gclient_utils.WorkItem): if self.recursion_limit() > 0: # Then we can parse the DEPS file. self.ParseDepsFile() - # Adjust the implicit dependency requirement; e.g. if a DEPS file contains - # both src/foo and src/foo/bar, src/foo/bar is implicitly dependent of - # src/foo. Yes, it's O(n^2)... It's important to do that before - # enqueueing them. - for s in self.dependencies: - for s2 in self.dependencies: - if s is s2: - continue - if s.name.startswith(posixpath.join(s2.name, '')): - s.requirements.append(s2.name) # Parse the dependencies of this dependency. for s in self.dependencies: diff --git a/tests/gclient_test.py b/tests/gclient_test.py index 065d718d61..ecbf0357b0 100755 --- a/tests/gclient_test.py +++ b/tests/gclient_test.py @@ -8,6 +8,7 @@ See gclient_smoketest.py for integration tests. """ +from __future__ import with_statement import Queue import logging import os @@ -94,6 +95,7 @@ class GclientTest(trial_dir.TestCase): 'solutions = [\n' ' { "name": "foo", "url": "svn://example.com/foo" },\n' ' { "name": "bar", "url": "svn://example.com/bar" },\n' + ' { "name": "bar/empty", "url": "svn://example.com/bar_empty" },\n' ']') write( os.path.join('foo', 'DEPS'), @@ -108,6 +110,10 @@ class GclientTest(trial_dir.TestCase): 'deps = {\n' ' "foo/dir1/dir2": "/dir1/dir2",\n' '}') + write( + os.path.join('bar/empty', 'DEPS'), + 'deps = {\n' + '}') obj = gclient.GClient.LoadCurrentConfig(options) self._check_requirements(obj.dependencies[0], {}) @@ -115,14 +121,23 @@ class GclientTest(trial_dir.TestCase): obj.RunOnDeps('None', args) # The trick here is to manually process the list to make sure it's out of # order. - obj.dependencies[0].dependencies.sort(key=lambda x: x.name, reverse=reverse) + for i in obj.dependencies: + i.dependencies.sort(key=lambda x: x.name, reverse=reverse) actual = self._get_processed() - # We don't care of the ordering of this item. + # We don't care of the ordering of these items: + self.assertEquals( + ['svn://example.com/bar', 'svn://example.com/foo'], sorted(actual[0:2])) + actual = actual[2:] + # Ordering may not be exact in case of parallel jobs. + self.assertTrue( + actual.index('svn://example.com/bar/dir1/dir2') > + actual.index('svn://example.com/foo/dir1')) actual.remove('svn://example.com/bar/dir1/dir2') + + # Ordering may not be exact in case of parallel jobs. + actual.remove('svn://example.com/bar_empty') self.assertEquals( [ - 'svn://example.com/foo', - 'svn://example.com/bar', 'svn://example.com/foo/dir1', 'svn://example.com/foo/dir1/dir4', 'svn://example.com/foo/dir1/dir2/dir3', @@ -133,19 +148,28 @@ class GclientTest(trial_dir.TestCase): obj.dependencies[0], { 'foo/dir1': ['foo'], - 'foo/dir1/dir2/dir3': ['foo', 'foo/dir1'], - 'foo/dir1/dir2/dir3/dir4': ['foo', 'foo/dir1', 'foo/dir1/dir2/dir3'], + 'foo/dir1/dir2/dir3': ['foo', 'foo/dir1', 'foo/dir1/dir2'], + 'foo/dir1/dir2/dir3/dir4': + ['foo', 'foo/dir1', 'foo/dir1/dir2', 'foo/dir1/dir2/dir3'], 'foo/dir1/dir4': ['foo', 'foo/dir1'], }) self._check_requirements( obj.dependencies[1], { - 'foo/dir1/dir2': ['bar'], + 'foo/dir1/dir2': ['bar', 'foo', 'foo/dir1'], + }) + self._check_requirements( + obj, + { + 'foo': [], + 'bar': [], + 'bar/empty': ['bar'], }) def _check_requirements(self, solution, expected): for dependency in solution.dependencies: - self.assertEquals(expected.pop(dependency.name), dependency.requirements) + self.assertEquals( + expected.pop(dependency.name), sorted(dependency.requirements)) self.assertEquals({}, expected) def _get_processed(self):