From d2ecdda620752f46a26cec8ec63233214d0c10ce Mon Sep 17 00:00:00 2001 From: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> Date: Fri, 17 Jul 2020 15:54:31 -0700 Subject: [PATCH] Tolerate target cycles when using dependency inference (#10393) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes https://github.com/pantsbuild/pants/issues/10059. It's common in many languages, including Python, to sometimes have import cycles. When using generated subtargets—meaning either dependency inference or explicit file dependencies—Pants will tolerate these cycles. For now, there is no other way to express that a cycle should be tolerated. In the future, we may consider adding a syntax to BUILD files to allow indicating that cycles should be tolerated; it's easier to loosen than it is to tighten. [ci skip-rust-tests] --- src/python/pants/base/specs.py | 72 +++++++++++++------ src/python/pants/engine/fs.py | 8 ++- .../pants/engine/interactive_process.py | 11 +-- src/python/pants/engine/internals/graph.py | 28 +++++++- .../pants/engine/internals/graph_test.py | 29 ++++++++ 5 files changed, 118 insertions(+), 30 deletions(-) diff --git a/src/python/pants/base/specs.py b/src/python/pants/base/specs.py index 77a5418b05b..83a7b69a205 100644 --- a/src/python/pants/base/specs.py +++ b/src/python/pants/base/specs.py @@ -5,7 +5,18 @@ import re from abc import ABC, ABCMeta, abstractmethod from dataclasses import dataclass -from typing import Any, Dict, Iterable, Iterator, List, Optional, Sequence, Tuple, Union, cast +from typing import ( + TYPE_CHECKING, + Dict, + Iterable, + Iterator, + List, + Optional, + Sequence, + Tuple, + Union, + cast, +) from pants.engine.collection import Collection from pants.engine.fs import PathGlobs @@ -17,6 +28,9 @@ from pants.util.memo import memoized_property from pants.util.meta import frozen_after_init +if TYPE_CHECKING: + from pants.engine.internals.mapper import AddressFamily, AddressMapper + class Spec(ABC): """A specification for what Pants should operate on.""" @@ -40,7 +54,9 @@ class AddressFamilyResolutionError(Exception): pass @abstractmethod - def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]: + def matching_address_families( + self, address_families_dict: Dict[str, "AddressFamily"] + ) -> List["AddressFamily"]: """Given a dict of (namespace path) -> AddressFamily, return the values matching this address spec. @@ -49,8 +65,8 @@ def matching_address_families(self, address_families_dict: Dict[str, Any]) -> Li @classmethod def address_families_for_dir( - cls, address_families_dict: Dict[str, Any], spec_dir_path: str - ) -> List[Any]: + cls, address_families_dict: Dict[str, "AddressFamily"], spec_dir_path: str + ) -> List["AddressFamily"]: """Implementation of `matching_address_families()` for address specs matching at most one directory.""" maybe_af = address_families_dict.get(spec_dir_path, None) @@ -64,7 +80,7 @@ class AddressResolutionError(Exception): pass @abstractmethod - def address_target_pairs_from_address_families(self, address_families: List[Any]): + def address_target_pairs_from_address_families(self, address_families: List["AddressFamily"]): """Given a list of AddressFamily, return (address, target) pairs matching this address spec. :raises: :class:`SingleAddress._SingleAddressResolutionError` for resolution errors with a @@ -84,11 +100,11 @@ def all_address_target_pairs(cls, address_families): return addr_tgt_pairs @abstractmethod - def make_glob_patterns(self, address_mapper: Any) -> List[str]: + def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]: """Generate glob patterns matching exactly all the BUILD files this address spec covers.""" @classmethod - def globs_in_single_dir(cls, spec_dir_path: str, address_mapper: Any) -> List[str]: + def globs_in_single_dir(cls, spec_dir_path: str, address_mapper: "AddressMapper") -> List[str]: """Implementation of `make_glob_patterns()` which only allows a single base directory.""" return [os.path.join(spec_dir_path, pat) for pat in address_mapper.build_patterns] @@ -109,16 +125,20 @@ def __post_init__(self) -> None: def to_spec_string(self) -> str: return "{}:{}".format(self.directory, self.name) - def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]: + def matching_address_families( + self, address_families_dict: Dict[str, "AddressFamily"] + ) -> List["AddressFamily"]: return self.address_families_for_dir(address_families_dict, self.directory) class _SingleAddressResolutionError(Exception): - def __init__(self, single_address_family: Any, name: str) -> None: + def __init__(self, single_address_family: "AddressFamily", name: str) -> None: super().__init__() self.single_address_family = single_address_family self.name = name - def address_target_pairs_from_address_families(self, address_families: Sequence[Any]): + def address_target_pairs_from_address_families( + self, address_families: Sequence["AddressFamily"] + ): """Return the pair for the single target matching the single AddressFamily, or error. :raises: :class:`SingleAddress._SingleAddressResolutionError` if no targets could be found for a @@ -137,7 +157,7 @@ def address_target_pairs_from_address_families(self, address_families: Sequence[ assert len(addr_tgt_pairs) == 1 return addr_tgt_pairs - def make_glob_patterns(self, address_mapper: Any) -> List[str]: + def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]: return self.globs_in_single_dir(self.directory, address_mapper) @@ -150,13 +170,17 @@ class SiblingAddresses(AddressSpec): def to_spec_string(self) -> str: return f"{self.directory}:" - def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]: + def matching_address_families( + self, address_families_dict: Dict[str, "AddressFamily"] + ) -> List["AddressFamily"]: return self.address_families_for_dir(address_families_dict, self.directory) - def address_target_pairs_from_address_families(self, address_families: Sequence[Any]): + def address_target_pairs_from_address_families( + self, address_families: Sequence["AddressFamily"] + ): return self.all_address_target_pairs(address_families) - def make_glob_patterns(self, address_mapper: Any) -> List[str]: + def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]: return self.globs_in_single_dir(self.directory, address_mapper) @@ -170,14 +194,18 @@ class DescendantAddresses(AddressSpec): def to_spec_string(self) -> str: return f"{self.directory}::" - def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]: + def matching_address_families( + self, address_families_dict: Dict[str, "AddressFamily"] + ) -> List["AddressFamily"]: return [ af for ns, af in address_families_dict.items() if fast_relpath_optional(ns, self.directory) is not None ] - def address_target_pairs_from_address_families(self, address_families: Sequence[Any]): + def address_target_pairs_from_address_families( + self, address_families: Sequence["AddressFamily"] + ): addr_tgt_pairs = self.all_address_target_pairs(address_families) if self.error_if_no_matches and len(addr_tgt_pairs) == 0: raise self.AddressResolutionError( @@ -185,7 +213,7 @@ def address_target_pairs_from_address_families(self, address_families: Sequence[ ) return addr_tgt_pairs - def make_glob_patterns(self, address_mapper: Any) -> List[str]: + def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]: return [os.path.join(self.directory, "**", pat) for pat in address_mapper.build_patterns] @@ -198,17 +226,21 @@ class AscendantAddresses(AddressSpec): def to_spec_string(self) -> str: return f"{self.directory}^" - def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]: + def matching_address_families( + self, address_families_dict: Dict[str, "AddressFamily"] + ) -> List["AddressFamily"]: return [ af for ns, af in address_families_dict.items() if fast_relpath_optional(self.directory, ns) is not None ] - def address_target_pairs_from_address_families(self, address_families): + def address_target_pairs_from_address_families( + self, address_families: Sequence["AddressFamily"] + ): return self.all_address_target_pairs(address_families) - def make_glob_patterns(self, address_mapper: Any) -> List[str]: + def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]: return [ os.path.join(f, pattern) for pattern in address_mapper.build_patterns diff --git a/src/python/pants/engine/fs.py b/src/python/pants/engine/fs.py index 99116e58695..fd40a620654 100644 --- a/src/python/pants/engine/fs.py +++ b/src/python/pants/engine/fs.py @@ -4,7 +4,7 @@ import os from dataclasses import dataclass from pathlib import Path -from typing import Any, Iterable, Optional, Tuple +from typing import TYPE_CHECKING, Iterable, Optional, Tuple from pants.engine.collection import Collection from pants.engine.rules import RootRule, side_effecting @@ -18,6 +18,9 @@ ) from pants.util.meta import frozen_after_init +if TYPE_CHECKING: + from pants.engine.internals.scheduler import SchedulerSession + @dataclass(frozen=True) class FileContent: @@ -282,8 +285,7 @@ class UrlToFetch: class Workspace: """Abstract handle for operations that touch the real local filesystem.""" - # TODO: SchedulerSession. Untyped because `fs.py` and `scheduler.py` have a cycle. - _scheduler: Any + _scheduler: "SchedulerSession" def materialize_directory(self, directory_to_materialize: DirectoryToMaterialize): """Materialize one single directory digest to disk. diff --git a/src/python/pants/engine/interactive_process.py b/src/python/pants/engine/interactive_process.py index a750a43ca4f..23470e8a034 100644 --- a/src/python/pants/engine/interactive_process.py +++ b/src/python/pants/engine/interactive_process.py @@ -3,13 +3,16 @@ import itertools from dataclasses import dataclass -from typing import Any, Iterable, Mapping, Optional, Tuple, cast +from typing import TYPE_CHECKING, Iterable, Mapping, Optional, Tuple from pants.base.exception_sink import ExceptionSink from pants.engine.fs import EMPTY_DIGEST, Digest from pants.engine.rules import RootRule, side_effecting from pants.util.meta import frozen_after_init +if TYPE_CHECKING: + from pants.engine.internals.scheduler import SchedulerSession + @dataclass(frozen=True) class InteractiveProcessResult: @@ -56,13 +59,11 @@ def __post_init__(self): @side_effecting @dataclass(frozen=True) class InteractiveRunner: - _scheduler: Any + _scheduler: "SchedulerSession" def run_process(self, request: InteractiveProcess) -> InteractiveProcessResult: ExceptionSink.toggle_ignoring_sigint_v2_engine(True) - return cast( - InteractiveProcessResult, self._scheduler.run_local_interactive_process(request) - ) + return self._scheduler.run_local_interactive_process(request) def rules(): diff --git a/src/python/pants/engine/internals/graph.py b/src/python/pants/engine/internals/graph.py index 1b3cdb9dd75..6b93f2790fc 100644 --- a/src/python/pants/engine/internals/graph.py +++ b/src/python/pants/engine/internals/graph.py @@ -141,8 +141,32 @@ async def transitive_target(wrapped_root: WrappedTarget) -> TransitiveTarget: 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) + + # For generated subtargets, we use a weak Get, which means that any dependency cycles will + # return None, rather than TransitiveTarget. + transitive_dependencies = await MultiGet( + Get(TransitiveTarget, Address, addr, weak=bool(addr.generated_base_target_name)) + for addr in dependency_addresses + ) + non_cyclic_dependencies = [] + cyclic_addresses = [] + for transitive_dep, address in zip(transitive_dependencies, dependency_addresses): + if transitive_dep: + non_cyclic_dependencies.append(transitive_dep) + else: + cyclic_addresses.append(address) + cyclic_dependencies = await MultiGet( + Get(WrappedTarget, Address, addr) for addr in cyclic_addresses + ) + + all_dependencies = ( + *non_cyclic_dependencies, + *( + TransitiveTarget(wrapped_tgt.target, dependencies=()) + for wrapped_tgt in cyclic_dependencies + ), + ) + return TransitiveTarget(root, tuple(all_dependencies)) @rule diff --git a/src/python/pants/engine/internals/graph_test.py b/src/python/pants/engine/internals/graph_test.py index aa8ae965f94..3bf8da0abb0 100644 --- a/src/python/pants/engine/internals/graph_test.py +++ b/src/python/pants/engine/internals/graph_test.py @@ -133,6 +133,35 @@ def get_target(name: str) -> Target: assert transitive_targets.dependencies == FrozenOrderedSet([d1, d2, d3, t2, t1]) assert transitive_targets.closure == FrozenOrderedSet([root, d2, d1, d3, t2, t1]) + def test_transitive_targets_tolerates_cycles(self) -> None: + """For generated subtargets, we should tolerate cycles between targets. + + This only works with generated subtargets, so we use explicit file dependencies in this + test. + """ + self.create_files("", ["dep.txt", "t1.txt", "t2.txt"]) + self.add_to_build_file( + "", + dedent( + """\ + target(name='dep', sources=['dep.txt']) + target(name='t1', sources=['t1.txt'], dependencies=['dep.txt', 't2.txt']) + target(name='t2', sources=['t2.txt'], dependencies=['t1.txt']) + """ + ), + ) + result = self.request_single_product( + TransitiveTargets, + Params(Addresses([Address.parse("//:t2")]), create_options_bootstrapper()), + ) + assert len(result.roots) == 1 + assert result.roots[0].address == Address.parse("//:t2") + assert [tgt.address for tgt in result.dependencies] == [ + Address("", target_name="t1.txt", generated_base_target_name="t1"), + Address("", target_name="dep.txt", generated_base_target_name="dep"), + Address("", target_name="t2.txt", generated_base_target_name="t2"), + ] + def test_resolve_generated_subtarget(self) -> None: self.add_to_build_file("demo", "target(sources=['f1.txt', 'f2.txt'])") generated_target_addresss = Address(