Tolerate dependency cycles when using the v2 engine

src/python/pants/engine/internals/graph.py

@@ -1,9 +1,10 @@
 # Copyright 2020 Pants project contributors (see CONTRIBUTORS.md).
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
+import itertools
 import logging
 import os.path
-from collections import defaultdict, deque
+from collections import defaultdict
 from dataclasses import dataclass
 from pathlib import PurePath
 from typing import DefaultDict, List, Tuple, Union, cast
@@ -39,7 +40,6 @@
     Targets,
     TargetsWithOrigins,
     TargetWithOrigin,
-    TransitiveTarget,
     TransitiveTargets,
     UnrecognizedTargetTypeException,
     WrappedTarget,
@@ -123,37 +123,24 @@ async def resolve_targets_with_origins(
 
 
 @rule
-async def transitive_target(wrapped_root: WrappedTarget) -> TransitiveTarget:
-    root = wrapped_root.target
-    if not root.has_field(Dependencies):
-        return TransitiveTarget(root, ())
-    dependency_addresses = await Get[Addresses](DependenciesRequest(root[Dependencies]))
-    dependencies = await MultiGet(Get[TransitiveTarget](Address, d) for d in dependency_addresses)
-    return TransitiveTarget(root, dependencies)
+async def transitive_targets(targets: Targets) -> TransitiveTargets:
+    """Find all the targets transitively depended upon by the target roots.
 
-
-@rule
-async def transitive_targets(addresses: Addresses) -> TransitiveTargets:
-    """Given Addresses, kicks off recursion on expansion of TransitiveTargets.
-
-    The TransitiveTarget dataclass represents a structure-shared graph, which we walk and flatten
-    here. The engine memoizes the computation of TransitiveTarget, so when multiple
-    TransitiveTargets objects are being constructed for multiple roots, their structure will be
-    shared.
+    This uses iteration, rather than recursion, so that we can tolerate dependency cycles. Unlike a
+    traditional BFS algorithm, we batch each round of traversals via `MultiGet` for improved
+    performance / concurrency.
     """
-    transitive_targets = await MultiGet(Get[TransitiveTarget](Address, a) for a in addresses)
-
-    closure: OrderedSet[Target] = OrderedSet()
-    to_visit = deque(transitive_targets)
-
-    while to_visit:
-        tt = to_visit.popleft()
-        if tt.root in closure:
-            continue
-        closure.add(tt.root)
-        to_visit.extend(tt.dependencies)
-
-    return TransitiveTargets(tuple(tt.root for tt in transitive_targets), FrozenOrderedSet(closure))
+    visited: OrderedSet[Target] = OrderedSet()
+    queued = FrozenOrderedSet(targets)
+    while queued:
+        visited.update(queued)
+        direct_dependencies = await MultiGet(
+            Get[Targets](DependenciesRequest(tgt.get(Dependencies))) for tgt in queued
+        )
+        queued = FrozenOrderedSet(itertools.chain.from_iterable(direct_dependencies)).difference(
+            visited
+        )
+    return TransitiveTargets(tuple(targets), FrozenOrderedSet(visited))
 
 
 # -----------------------------------------------------------------------------------------------
@@ -313,7 +300,6 @@ def rules():
         resolve_targets,
         resolve_target_with_origin,
         resolve_targets_with_origins,
-        transitive_target,
         transitive_targets,
         find_owners,
         addresses_with_origins_from_filesystem_specs,

src/python/pants/engine/internals/graph_test.py

@@ -11,7 +11,6 @@
     DependenciesRequest,
     Target,
     Targets,
-    TransitiveTarget,
     TransitiveTargets,
     WrappedTarget,
 )
@@ -44,6 +43,11 @@ def test_transitive_targets(self) -> None:
             {Dependencies.alias: [d1.address, d2.address, d3.address]},
             address=Address.parse(":root"),
         )
+        cycle1_addr = Address.parse(":cycle1")
+        cycle2_addr = Address.parse(":cycle2")
+        cycle1 = MockTarget({Dependencies.alias: [cycle2_addr]}, address=cycle1_addr)
+        # Also include a dependency on itself (self-cycle)
+        cycle2 = MockTarget({Dependencies.alias: [cycle1_addr, cycle2_addr]}, address=cycle2_addr)
 
         self.add_to_build_file(
             "",
@@ -55,6 +59,8 @@ def test_transitive_targets(self) -> None:
                 target(name='d2', dependencies=[':t2'])
                 target(name='d3')
                 target(name='root', dependencies=[':d1', ':d2', ':d3'])
+                target(name='cycle1', dependencies=[':cycle2'])
+                target(name='cycle2', dependencies=[':cycle1', ':cycle2'])
                 """
             ),
         )
@@ -64,17 +70,13 @@ def test_transitive_targets(self) -> None:
         )
         assert direct_deps == Targets([d1, d2, d3])
 
-        transitive_target = self.request_single_product(
-            TransitiveTarget, Params(WrappedTarget(root), create_options_bootstrapper())
-        )
-        assert transitive_target.root == root
-        assert {
-            dep_transitive_target.root for dep_transitive_target in transitive_target.dependencies
-        } == {d1, d2, d3}
-
         transitive_targets = self.request_single_product(
             TransitiveTargets,
-            Params(Addresses([root.address, d2.address]), create_options_bootstrapper()),
+            Params(
+                Addresses([root.address, d2.address, cycle2_addr]), create_options_bootstrapper()
+            ),
+        )
+        assert transitive_targets.roots == (root, d2, cycle2)
+        assert transitive_targets.closure == FrozenOrderedSet(
+            [root, d2, cycle2, d1, d3, t2, cycle1, t1]
         )
-        assert transitive_targets.roots == (root, d2)
-        assert transitive_targets.closure == FrozenOrderedSet([root, d2, d1, d3, t2, t1])

src/python/pants/engine/target.py

@@ -537,14 +537,6 @@ def targets(self) -> Tuple[Target, ...]:
         return tuple(tgt_with_origin.target for tgt_with_origin in self)
 
 
-@dataclass(frozen=True)
-class TransitiveTarget:
-    """A recursive structure wrapping a Target root and TransitiveTarget deps."""
-
-    root: Target
-    dependencies: Tuple["TransitiveTarget", ...]
-
-
 @dataclass(frozen=True)
 class TransitiveTargets:
     """A set of Target roots, and their transitive, flattened, de-duped closure."""