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

@@ -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:

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):