Fix Task fingerprinting. (#6894)

Previously transitive Subsystem dependencies did not count against the
task fingerprint which was a known, latent and slient bug.

Fixes #2739

src/python/pants/subsystem/subsystem_client_mixin.py

@@ -66,6 +66,34 @@ def subsystem_dependencies_iter(cls):
       else:
         yield SubsystemDependency(dep, GLOBAL_SCOPE)
 
+  @classmethod
+  def subsystem_closure_iter(cls):
+    """Iterate over the transitive closure of subsystem dependencies of this Optionable.
+
+    :rtype: :class:`collections.Iterator` of :class:`SubsystemDependency`
+    :raises: :class:`pants.subsystem.subsystem_client_mixin.SubsystemClientMixin.CycleException`
+             if a dependency cycle is detected.
+    """
+    seen = set()
+    dep_path = OrderedSet()
+
+    def iter_subsystem_closure(subsystem_cls):
+      if subsystem_cls in dep_path:
+        raise cls.CycleException(list(dep_path) + [subsystem_cls])
+      dep_path.add(subsystem_cls)
+
+      for dep in subsystem_cls.subsystem_dependencies_iter():
+        if dep not in seen:
+          seen.add(dep)
+          yield dep
+          for d in iter_subsystem_closure(dep.subsystem_cls):
+            yield d
+
+      dep_path.remove(subsystem_cls)
+
+    for dep in iter_subsystem_closure(cls):
+      yield dep
+
   class CycleException(Exception):
     """Thrown when a circular subsystem dependency is detected."""
 
@@ -77,8 +105,9 @@ def __init__(self, cycle):
 
   @classmethod
   def known_scope_infos(cls):
-    """Yields ScopeInfo for all known scopes for this optionable, in no particular order.
+    """Yield ScopeInfo for all known scopes for this optionable, in no particular order.
 
+    :rtype: set of :class:`pants.option.scope.ScopeInfo`
     :raises: :class:`pants.subsystem.subsystem_client_mixin.SubsystemClientMixin.CycleException`
              if a dependency cycle is detected.
     """

src/python/pants/task/task.py

@@ -291,8 +291,7 @@ def fingerprint(self):
     hasher.update(self.stable_name().encode('utf-8'))
     hasher.update(self._options_fingerprint(self.options_scope).encode('utf-8'))
     hasher.update(self.implementation_version_str().encode('utf-8'))
-    # TODO: this is not recursive, but should be: see #2739
-    for dep in self.subsystem_dependencies_iter():
+    for dep in self.subsystem_closure_iter():
       hasher.update(self._options_fingerprint(dep.options_scope()).encode('utf-8'))
     return hasher.hexdigest() if PY3 else hasher.hexdigest().decode('utf-8')
 

tests/python/pants_test/subsystem/test_subsystem.py

@@ -224,3 +224,49 @@ def subsystem_dependencies(cls):
 
     dep_scopes = {dep.options_scope() for dep in SubsystemA.subsystem_dependencies_iter()}
     self.assertEqual({'b', 'dummy.a'}, dep_scopes)
+
+  def test_subsystem_closure_iter(self):
+    class SubsystemA(Subsystem):
+      options_scope = 'a'
+
+    class SubsystemB(Subsystem):
+      options_scope = 'b'
+
+      @classmethod
+      def subsystem_dependencies(cls):
+        return (SubsystemA,)
+
+    class SubsystemC(Subsystem):
+      options_scope = 'c'
+
+      @classmethod
+      def subsystem_dependencies(cls):
+        return (SubsystemA, SubsystemB.scoped(cls))
+
+    class SubsystemD(Subsystem):
+      options_scope = 'd'
+
+      @classmethod
+      def subsystem_dependencies(cls):
+        return (SubsystemC, SubsystemB)
+
+    dep_scopes = {dep.options_scope() for dep in SubsystemD.subsystem_closure_iter()}
+    self.assertEqual({'c', 'a', 'b.c', 'b'}, dep_scopes)
+
+  def test_subsystem_closure_iter_cycle(self):
+    class SubsystemA(Subsystem):
+      options_scope = 'a'
+
+      @classmethod
+      def subsystem_dependencies(cls):
+        return (SubsystemB,)
+
+    class SubsystemB(Subsystem):
+      options_scope = 'b'
+
+      @classmethod
+      def subsystem_dependencies(cls):
+        return (SubsystemA,)
+
+    with self.assertRaises(SubsystemClientMixin.CycleException):
+      list(SubsystemB.subsystem_closure_iter())

tests/python/pants_test/task/test_task.py

@@ -106,6 +106,14 @@ def register_options(cls, register):
     register('--fake-option', type=bool)
 
 
+class SubsystemWithDependencies(Subsystem):
+  options_scope = 'subsystem-with-deps'
+
+  @classmethod
+  def subsystem_dependencies(cls):
+    return super(SubsystemWithDependencies, cls).subsystem_dependencies() + (FakeSubsystem,)
+
+
 class AnotherFakeTask(Task):
   options_scope = 'another-fake-task'
 
@@ -128,6 +136,19 @@ def supports_passthru_args(cls):
     return False
 
 
+class TaskWithTransitiveSubsystemDependencies(Task):
+  options_scope = 'task-with-transitive-subsystem-deps'
+
+  @classmethod
+  def subsystem_dependencies(cls):
+    return super(TaskWithTransitiveSubsystemDependencies, cls).subsystem_dependencies() + (
+      SubsystemWithDependencies,
+    )
+
+  def execute(self):
+    pass
+
+
 class TaskTest(TaskTestBase):
 
   _filename = 'f'
@@ -612,3 +633,20 @@ def test_fingerprint_passthru_args(self):
       passthru_args=['asdf'],
     )
     self.assertEqual(different_task_with_passthru_fp, different_task_with_same_opts_fp)
+
+  def test_fingerprint_transitive(self):
+    fp1 = self._synth_fp(cls=TaskWithTransitiveSubsystemDependencies)
+
+    # A transitive but non-fingerprintable option
+    self.set_options_for_scope(FakeSubsystem.options_scope, **{'fake-option': True})
+    fp2 = self._synth_fp(cls=TaskWithTransitiveSubsystemDependencies)
+    self.assertEqual(fp1, fp2)
+
+    # A transitive fingerprintable option
+    option_spec = {
+      FakeSubsystem.options_scope: {'fake-option': bool},
+    }
+    self.set_options_for_scope(FakeSubsystem.options_scope, **{'fake-option': True})
+    fp3 = self._synth_fp(cls=TaskWithTransitiveSubsystemDependencies,
+                         options_fingerprintable=option_spec)
+    self.assertNotEqual(fp1, fp3)