gclient: Use only recursedeps to decide whether to process a dependency.

Bug: 839925
Change-Id: If86cfbdbea795b6da53dec1ea6e44c5af8a3b846
Reviewed-on: https://chromium-review.googlesource.com/1086270
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

gclient.py

@@ -358,7 +358,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
 
   def __init__(self, parent, name, url, managed, custom_deps,
                custom_vars, custom_hooks, deps_file, should_process,
-               relative, condition, print_outbuf=False):
+               should_recurse, relative, condition, print_outbuf=False):
     gclient_utils.WorkItem.__init__(self, name)
     DependencySettings.__init__(
         self, parent, url, managed, custom_deps, custom_vars,
@@ -402,17 +402,14 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     # unavailable
     self._got_revision = None
 
-    # This is a mutable value that overrides the normal recursion limit for this
-    # dependency.  It is read from the actual DEPS file so cannot be set on
-    # class instantiation.
-    self.recursion_override = None
     # recursedeps is a mutable value that selectively overrides the default
-    # 'no recursion' setting on a dep-by-dep basis.  It will replace
-    # recursion_override.
+    # 'no recursion' setting on a dep-by-dep basis.
     #
-    # It will be a dictionary of {deps_name: {"deps_file": depfile_name}} or
-    # None.
-    self.recursedeps = None
+    # It will be a dictionary of {deps_name: depfile_namee}
+    self.recursedeps = {}
+
+    # Whether we should process this dependency's DEPS file.
+    self._should_recurse = should_recurse
 
     self._OverrideUrl()
     # This is inherited from WorkItem.  We want the URL to be a resource.
@@ -497,9 +494,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     # 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.
@@ -517,24 +511,8 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     return requirements
 
   @property
-  def try_recursedeps(self):
-    """Returns False if recursion_override is ever specified."""
-    if self.recursion_override is not None:
-      return False
-    return self.parent.try_recursedeps
-
-  @property
-  def recursion_limit(self):
-    """Returns > 0 if this dependency is not too recursed to be processed."""
-    # We continue to support the absence of recursedeps until tools and DEPS
-    # using recursion_override are updated.
-    if self.try_recursedeps and self.parent.recursedeps != None:
-      if self.name in self.parent.recursedeps:
-        return 1
-
-    if self.recursion_override is not None:
-      return self.recursion_override
-    return max(self.parent.recursion_limit - 1, 0)
+  def should_recurse(self):
+    return self._should_recurse
 
   def verify_validity(self):
     """Verifies that this Dependency is fine to add as a child of another one.
@@ -609,12 +587,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     """Convert a deps dict to a dict of Dependency objects."""
     deps_to_add = []
     for name, dep_value in deps.iteritems():
-      should_process = self.recursion_limit and self.should_process
-      deps_file = self.deps_file
-      if self.recursedeps is not None:
-        ent = self.recursedeps.get(name)
-        if ent is not None:
-          deps_file = ent['deps_file']
+      should_process = self.should_process
       if dep_value is None:
         continue
 
@@ -653,8 +626,9 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
                 custom_deps=None,
                 custom_vars=self.custom_vars,
                 custom_hooks=None,
-                deps_file=deps_file,
+                deps_file=self.recursedeps.get(name, self.deps_file),
                 should_process=should_process,
+                should_recurse=name in self.recursedeps,
                 relative=use_relative_paths,
                 condition=condition))
 
@@ -743,17 +717,15 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
       rel_prefix = os.path.dirname(self.name)
 
     if 'recursion' in local_scope:
-      self.recursion_override = local_scope.get('recursion')
       logging.warning(
-          'Setting %s recursion to %d.', self.name, self.recursion_limit)
-    self.recursedeps = None
+          '%s: Ignoring recursion = %d.', self.name, local_scope['recursion'])
+
     if 'recursedeps' in local_scope:
-      self.recursedeps = {}
       for ent in local_scope['recursedeps']:
         if isinstance(ent, basestring):
-          self.recursedeps[ent] = {"deps_file": self.deps_file}
+          self.recursedeps[ent] = self.deps_file
         else:  # (depname, depsfilename)
-          self.recursedeps[ent[0]] = {"deps_file": ent[1]}
+          self.recursedeps[ent[0]] = ent[1]
       logging.warning('Found recursedeps %r.', repr(self.recursedeps))
 
       if rel_prefix:
@@ -788,7 +760,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
       if 'action' in hook:
         hooks_to_run.append(hook)
 
-    if self.recursion_limit:
+    if self.should_recurse:
       self._pre_deps_hooks = [
           Hook.from_dict(hook, variables=self.get_vars(), verbose=True,
                          conditions=self.condition)
@@ -914,12 +886,12 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
         while file_list[i].startswith(('\\', '/')):
           file_list[i] = file_list[i][1:]
 
-    if self.recursion_limit:
+    if self.should_recurse:
       self.ParseDepsFile()
 
     self._run_is_done(file_list or [])
 
-    if self.recursion_limit:
+    if self.should_recurse:
       if command in ('update', 'revert') and not options.noprehooks:
         self.RunPreDepsHooks()
       # Parse the dependencies of this dependency.
@@ -1015,7 +987,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     RunOnDeps() must have been called before to load the DEPS.
     """
     result = []
-    if not self.should_process or not self.recursion_limit:
+    if not self.should_process or not self.should_recurse:
       # Don't run the hook when it is above recursion_limit.
       return result
     # If "--force" was specified, run all hooks regardless of what files have
@@ -1304,6 +1276,7 @@ solutions = %(solution_list)s
         custom_hooks=None,
         deps_file='unused',
         should_process=True,
+        should_recurse=True,
         relative=None,
         condition=None,
         print_outbuf=True)
@@ -1407,6 +1380,7 @@ it or fix the checkout.
             custom_hooks=s.get('custom_hooks', []),
             deps_file=s.get('deps_file', 'DEPS'),
             should_process=True,
+            should_recurse=True,
             relative=None,
             condition=None,
             print_outbuf=True))
@@ -1798,16 +1772,6 @@ it or fix the checkout.
     """What deps_os entries that are to be parsed."""
     return self._enforced_os
 
-  @property
-  def recursion_limit(self):
-    """How recursive can each dependencies in DEPS file can load DEPS file."""
-    return self._recursion_limit
-
-  @property
-  def try_recursedeps(self):
-    """Whether to attempt using recursedeps-style recursion processing."""
-    return True
-
   @property
   def target_os(self):
     return self._enforced_os
@@ -1837,6 +1801,7 @@ class CipdDependency(Dependency):
         custom_hooks=None,
         deps_file=None,
         should_process=should_process,
+        should_recurse=False,
         relative=relative,
         condition=condition)
     if relative:
@@ -2047,8 +2012,7 @@ class Flattener(object):
 
     def add_deps_file(dep):
       # Only include DEPS files referenced by recursedeps.
-      if not (dep.parent is None or
-              (dep.name in (dep.parent.recursedeps or {}))):
+      if not dep.should_recurse:
         return
       deps_file = dep.deps_file
       deps_path = os.path.join(self._client.root_dir, dep.name, deps_file)
@@ -2129,9 +2093,9 @@ class Flattener(object):
     for sub_dep in dep.dependencies:
       self._add_dep(sub_dep)
 
-    deps_by_name = {d.name: d for d in dep.dependencies}
-    for recurse_dep_name in (dep.recursedeps or []):
-      self._flatten_dep(deps_by_name[recurse_dep_name])
+    for d in dep.dependencies:
+      if d.should_recurse:
+        self._flatten_dep(d)
 
 
 def CMDflatten(parser, args):

gclient_scm.py

@@ -1321,7 +1321,7 @@ class CipdRoot(object):
           suffix='.ensure', delete=False) as ensure_file:
         for subdir, packages in sorted(self._packages_by_subdir.iteritems()):
           ensure_file.write('@Subdir %s\n' % subdir)
-          for package in packages:
+          for package in sorted(packages, key=lambda p: p.name):
             ensure_file.write('%s %s\n' % (package.name, package.version))
           ensure_file.write('\n')
       yield ensure_file.name

tests/gclient_smoketest.py

@@ -1063,7 +1063,8 @@ class GClientSmokeGIT(GClientSmokeBase):
         '',
         '# ' + self.git_base + 'repo_2@%s, DEPS' % (
                  self.githash('repo_2', 1)[:7]),
-        '# ' + self.git_base + 'repo_8, DEPS'
+        '# ' + self.git_base + 'repo_6, DEPS',
+        '# ' + self.git_base + 'repo_8, DEPS',
     ], deps_contents.splitlines())
 
   def testFlattenPinAllDeps(self):
@@ -1232,6 +1233,8 @@ class GClientSmokeGIT(GClientSmokeBase):
         '',
         '# ' + self.git_base + 'repo_2@%s, DEPS' % (
             self.githash('repo_2', 1)),
+        '# ' + self.git_base + 'repo_6@%s, DEPS' % (
+            self.githash('repo_6', 1)),
         '# ' + self.git_base + 'repo_8@%s, DEPS' % (
             self.githash('repo_8', 1)),
     ], deps_contents.splitlines())
@@ -1317,6 +1320,7 @@ class GClientSmokeGIT(GClientSmokeBase):
         '',
         '}',
         '',
+        '# ' + self.git_base + 'repo_10, DEPS',
         '# ' + self.git_base + 'repo_11, DEPS',
         '# ' + self.git_base + 'repo_8, DEPS',
         '# ' + self.git_base + 'repo_9, DEPS',
@@ -1326,6 +1330,8 @@ class GClientSmokeGIT(GClientSmokeBase):
       deps_files_contents = json.load(f)
 
     self.assertEqual([
+      {'url': self.git_base + 'repo_10', 'deps_file': 'DEPS',
+       'hierarchy': [['src', self.git_base + 'repo_10']]},
       {'url': self.git_base + 'repo_11', 'deps_file': 'DEPS',
        'hierarchy': [['src', self.git_base + 'repo_10'],
                      ['src/repo11', self.git_base + 'repo_11']]},
@@ -1387,7 +1393,8 @@ class GClientSmokeGIT(GClientSmokeBase):
         '  },',
         '',
         '}',
-        ''
+        '',
+        '# ' + self.git_base + 'repo_14, DEPS',
     ], deps_contents.splitlines())
 
 

tests/gclient_test.py

@@ -201,8 +201,19 @@ class GclientTest(trial_dir.TestCase):
     # auto-fixed.
     url = 'proto://host/path/@revision'
     d = gclient.Dependency(
-        None, 'name', url, None, None, None,
-        None, '', True, False, None, True)
+        parent=None,
+        name='name',
+        url=url,
+        managed=None,
+        custom_deps=None,
+        custom_vars=None,
+        custom_hooks=None,
+        deps_file='',
+        should_process=True,
+        should_recurse=False,
+        relative=False,
+        condition=None,
+        print_outbuf=True)
     self.assertEquals('proto://host/path@revision', d.url)
 
   def testStr(self):
@@ -212,19 +223,51 @@ class GclientTest(trial_dir.TestCase):
     obj.add_dependencies_and_close(
       [
         gclient.Dependency(
-            obj, 'foo', 'svn://example.com/foo', None,
-            None, None, None, 'DEPS', True, False, None, True),
+            parent=obj,
+            name='foo',
+            url='svn://example.com/foo',
+            managed=None,
+            custom_deps=None,
+            custom_vars=None,
+            custom_hooks=None,
+            deps_file='DEPS',
+            should_process=True,
+            should_recurse=True,
+            relative=False,
+            condition=None,
+            print_outbuf=True),
         gclient.Dependency(
-            obj, 'bar', 'svn://example.com/bar', None,
-            None, None, None, 'DEPS', True, False, None, True),
+            parent=obj,
+            name='bar',
+            url='svn://example.com/bar',
+            managed=None,
+            custom_deps=None,
+            custom_vars=None,
+            custom_hooks=None,
+            deps_file='DEPS',
+            should_process=True,
+            should_recurse=False,
+            relative=False,
+            condition=None,
+            print_outbuf=True),
       ],
       [])
     obj.dependencies[0].add_dependencies_and_close(
       [
         gclient.Dependency(
-            obj.dependencies[0], 'foo/dir1', 'svn://example.com/foo/dir1',
-            None, None, None, None, 'DEPS', True,
-            False, None, True),
+            parent=obj.dependencies[0],
+            name='foo/dir1',
+            url='svn://example.com/foo/dir1',
+            managed=None,
+            custom_deps=None,
+            custom_vars=None,
+            custom_hooks=None,
+            deps_file='DEPS',
+            should_process=True,
+            should_recurse=False,
+            relative=False,
+            condition=None,
+            print_outbuf=True),
       ],
       [])
     # TODO(ehmaldonado): Improve this test.
@@ -593,56 +636,6 @@ class GclientTest(trial_dir.TestCase):
           ],
         sorted(self._get_processed()))
 
-  def testRecursionOverride(self):
-    """Verifies gclient respects the |recursion| var syntax.
-
-    We check several things here:
-    - |recursion| = 3 sets recursion on the foo dep to exactly 3
-      (we pull /fizz, but not /fuzz)
-    - pulling foo/bar at recursion level 1 (in .gclient) is overriden by
-      a later pull of foo/bar at recursion level 2 (in the dep tree)
-    """
-    write(
-        '.gclient',
-        'solutions = [\n'
-        '  { "name": "foo", "url": "svn://example.com/foo" },\n'
-        '  { "name": "foo/bar", "url": "svn://example.com/bar" },\n'
-          ']')
-    write(
-        os.path.join('foo', 'DEPS'),
-        'deps = {\n'
-        '  "bar": "/bar",\n'
-        '}\n'
-        'recursion = 3')
-    write(
-        os.path.join('bar', 'DEPS'),
-        'deps = {\n'
-        '  "baz": "/baz",\n'
-        '}')
-    write(
-        os.path.join('baz', 'DEPS'),
-        'deps = {\n'
-        '  "fizz": "/fizz",\n'
-        '}')
-    write(
-        os.path.join('fizz', 'DEPS'),
-        'deps = {\n'
-        '  "fuzz": "/fuzz",\n'
-        '}')
-
-    options, _ = gclient.OptionParser().parse_args([])
-    obj = gclient.GClient.LoadCurrentConfig(options)
-    obj.RunOnDeps('None', [])
-    self.assertEquals(
-        [
-          ('foo', 'svn://example.com/foo'),
-          ('foo/bar', 'svn://example.com/bar'),
-          ('bar', 'svn://example.com/bar'),
-          ('baz', 'svn://example.com/baz'),
-          ('fizz', 'svn://example.com/fizz'),
-        ],
-        self._get_processed())
-
   def testRecursedepsOverride(self):
     """Verifies gclient respects the |recursedeps| var syntax.
 
@@ -777,95 +770,6 @@ class GclientTest(trial_dir.TestCase):
         ],
         self._get_processed())
 
-  def testRecursionOverridesRecursedeps(self):
-    """Verifies gclient respects |recursion| over |recursedeps|.
-
-    |recursion| is set in a top-level DEPS file.  That value is meant
-    to affect how many subdeps are parsed via recursion.
-
-    |recursedeps| is set in each DEPS file to control whether or not
-    to recurse into the immediate next subdep.
-
-    This test verifies that if both syntaxes are mixed in a DEPS file,
-    we disable |recursedeps| support and only obey |recursion|.
-
-    Since this setting is evaluated per DEPS file, recursed DEPS
-    files will each be re-evaluated according to the per DEPS rules.
-    So a DEPS that only contains |recursedeps| could then override any
-    previous |recursion| setting.  There is extra processing to ensure
-    this does not happen.
-
-    For this test to work correctly, we need to use a DEPS chain that
-    only contains recursion controls in the top DEPS file.
-
-    In foo, |recursion| and |recursedeps| are specified.  When we see
-    |recursion|, we stop trying to use |recursedeps|.
-
-    There are 2 constructions of DEPS here that are key to this test:
-
-    (1) In foo, if we used |recursedeps| instead of |recursion|, we
-        would also pull in bar.  Since bar's DEPS doesn't contain any
-        recursion statements, we would stop processing at bar.
-
-    (2) In fizz, if we used |recursedeps| at all, we should pull in
-        fuzz.
-
-    We expect to keep going past bar (satisfying 1) and we don't
-    expect to pull in fuzz (satisfying 2).
-    """
-    write(
-        '.gclient',
-        'solutions = [\n'
-        '  { "name": "foo", "url": "svn://example.com/foo" },\n'
-        '  { "name": "foo/bar", "url": "svn://example.com/bar" },\n'
-          ']')
-    write(
-        os.path.join('foo', 'DEPS'),
-        'deps = {\n'
-        '  "bar": "/bar",\n'
-        '}\n'
-        'recursion = 3\n'
-        'recursedeps = ["bar"]')
-    write(
-        os.path.join('bar', 'DEPS'),
-        'deps = {\n'
-        '  "baz": "/baz",\n'
-        '}')
-    write(
-        os.path.join('baz', 'DEPS'),
-        'deps = {\n'
-        '  "fizz": "/fizz",\n'
-        '}')
-    write(
-        os.path.join('fizz', 'DEPS'),
-        'deps = {\n'
-        '  "fuzz": "/fuzz",\n'
-        '}\n'
-        'recursedeps = ["fuzz"]')
-    write(
-        os.path.join('fuzz', 'DEPS'),
-        'deps = {\n'
-        '  "tar": "/tar",\n'
-        '}')
-
-    options, _ = gclient.OptionParser().parse_args([])
-    obj = gclient.GClient.LoadCurrentConfig(options)
-    obj.RunOnDeps('None', [])
-    self.assertEquals(
-        [
-          ('foo', 'svn://example.com/foo'),
-          ('foo/bar', 'svn://example.com/bar'),
-          ('bar', 'svn://example.com/bar'),
-          # Deps after this would have been skipped if we were obeying
-          # |recursedeps|.
-          ('baz', 'svn://example.com/baz'),
-          ('fizz', 'svn://example.com/fizz'),
-          # And this dep would have been picked up if we were obeying
-          # |recursedeps|.
-          # 'svn://example.com/foo/bar/baz/fuzz',
-        ],
-        self._get_processed())
-
   def testRecursedepsAltfile(self):
     """Verifies gclient respects the |recursedeps| var syntax with overridden
     target DEPS file.
@@ -1130,23 +1034,43 @@ class GclientTest(trial_dir.TestCase):
     obj.add_dependencies_and_close(
       [
         gclient.Dependency(
-            obj, 'foo', 'svn://example.com/foo', None,
-            None, None, None, 'DEPS', True,
-            False, None, True),
+            parent=obj,
+            name='foo',
+            url='svn://example.com/foo',
+            managed=None,
+            custom_deps=None,
+            custom_vars=None,
+            custom_hooks=None,
+            deps_file='DEPS',
+            should_process=True,
+            should_recurse=True,
+            relative=False,
+            condition=None,
+            print_outbuf=True),
       ],
       [])
     obj.dependencies[0].add_dependencies_and_close(
       [
-        gclient.CipdDependency(obj.dependencies[0], 'foo',
-                               {'package': 'foo_package',
+        gclient.CipdDependency(
+            parent=obj.dependencies[0],
+            name='foo',
+            dep_value={'package': 'foo_package',
                        'version': 'foo_version'},
-                               cipd_root, None, True, False,
-                               'fake_condition'),
-        gclient.CipdDependency(obj.dependencies[0], 'foo',
-                               {'package': 'bar_package',
+            cipd_root=cipd_root,
+            custom_vars=None,
+            should_process=True,
+            relative=False,
+            condition='fake_condition'),
+        gclient.CipdDependency(
+            parent=obj.dependencies[0],
+            name='bar',
+            dep_value={'package': 'bar_package',
                        'version': 'bar_version'},
-                               cipd_root, None, True, False,
-                               'fake_condition'),
+            cipd_root=cipd_root,
+            custom_vars=None,
+            should_process=True,
+            relative=False,
+            condition='fake_condition'),
       ],
       [])
     dep0 = obj.dependencies[0].dependencies[0]