Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Tolerate target cycles when using dependency inference (#10393)" #10401

Merged
merged 1 commit into from Jul 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
72 changes: 20 additions & 52 deletions src/python/pants/base/specs.py
Expand Up @@ -5,18 +5,7 @@
import re
from abc import ABC, ABCMeta, abstractmethod
from dataclasses import dataclass
from typing import (
TYPE_CHECKING,
Dict,
Iterable,
Iterator,
List,
Optional,
Sequence,
Tuple,
Union,
cast,
)
from typing import Any, Dict, Iterable, Iterator, List, Optional, Sequence, Tuple, Union, cast

from pants.engine.collection import Collection
from pants.engine.fs import PathGlobs
Expand All @@ -28,9 +17,6 @@
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."""
Expand All @@ -54,9 +40,7 @@ class AddressFamilyResolutionError(Exception):
pass

@abstractmethod
def matching_address_families(
self, address_families_dict: Dict[str, "AddressFamily"]
) -> List["AddressFamily"]:
def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]:
"""Given a dict of (namespace path) -> AddressFamily, return the values matching this
address spec.

Expand All @@ -65,8 +49,8 @@ def matching_address_families(

@classmethod
def address_families_for_dir(
cls, address_families_dict: Dict[str, "AddressFamily"], spec_dir_path: str
) -> List["AddressFamily"]:
cls, address_families_dict: Dict[str, Any], spec_dir_path: str
) -> List[Any]:
"""Implementation of `matching_address_families()` for address specs matching at most one
directory."""
maybe_af = address_families_dict.get(spec_dir_path, None)
Expand All @@ -80,7 +64,7 @@ class AddressResolutionError(Exception):
pass

@abstractmethod
def address_target_pairs_from_address_families(self, address_families: List["AddressFamily"]):
def address_target_pairs_from_address_families(self, address_families: List[Any]):
"""Given a list of AddressFamily, return (address, target) pairs matching this address spec.

:raises: :class:`SingleAddress._SingleAddressResolutionError` for resolution errors with a
Expand All @@ -100,11 +84,11 @@ def all_address_target_pairs(cls, address_families):
return addr_tgt_pairs

@abstractmethod
def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]:
def make_glob_patterns(self, address_mapper: Any) -> 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: "AddressMapper") -> List[str]:
def globs_in_single_dir(cls, spec_dir_path: str, address_mapper: Any) -> 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]

Expand All @@ -125,20 +109,16 @@ 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, "AddressFamily"]
) -> List["AddressFamily"]:
def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]:
return self.address_families_for_dir(address_families_dict, self.directory)

class _SingleAddressResolutionError(Exception):
def __init__(self, single_address_family: "AddressFamily", name: str) -> None:
def __init__(self, single_address_family: Any, 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["AddressFamily"]
):
def address_target_pairs_from_address_families(self, address_families: Sequence[Any]):
"""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
Expand All @@ -157,7 +137,7 @@ def address_target_pairs_from_address_families(
assert len(addr_tgt_pairs) == 1
return addr_tgt_pairs

def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]:
def make_glob_patterns(self, address_mapper: Any) -> List[str]:
return self.globs_in_single_dir(self.directory, address_mapper)


Expand All @@ -170,17 +150,13 @@ class SiblingAddresses(AddressSpec):
def to_spec_string(self) -> str:
return f"{self.directory}:"

def matching_address_families(
self, address_families_dict: Dict[str, "AddressFamily"]
) -> List["AddressFamily"]:
def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]:
return self.address_families_for_dir(address_families_dict, self.directory)

def address_target_pairs_from_address_families(
self, address_families: Sequence["AddressFamily"]
):
def address_target_pairs_from_address_families(self, address_families: Sequence[Any]):
return self.all_address_target_pairs(address_families)

def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]:
def make_glob_patterns(self, address_mapper: Any) -> List[str]:
return self.globs_in_single_dir(self.directory, address_mapper)


Expand All @@ -194,26 +170,22 @@ class DescendantAddresses(AddressSpec):
def to_spec_string(self) -> str:
return f"{self.directory}::"

def matching_address_families(
self, address_families_dict: Dict[str, "AddressFamily"]
) -> List["AddressFamily"]:
def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]:
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["AddressFamily"]
):
def address_target_pairs_from_address_families(self, address_families: Sequence[Any]):
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(
f"Address spec {repr(self.to_spec_string())} does not match any targets."
)
return addr_tgt_pairs

def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]:
def make_glob_patterns(self, address_mapper: Any) -> List[str]:
return [os.path.join(self.directory, "**", pat) for pat in address_mapper.build_patterns]


Expand All @@ -226,21 +198,17 @@ class AscendantAddresses(AddressSpec):
def to_spec_string(self) -> str:
return f"{self.directory}^"

def matching_address_families(
self, address_families_dict: Dict[str, "AddressFamily"]
) -> List["AddressFamily"]:
def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]:
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: Sequence["AddressFamily"]
):
def address_target_pairs_from_address_families(self, address_families):
return self.all_address_target_pairs(address_families)

def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]:
def make_glob_patterns(self, address_mapper: Any) -> List[str]:
return [
os.path.join(f, pattern)
for pattern in address_mapper.build_patterns
Expand Down
8 changes: 3 additions & 5 deletions src/python/pants/engine/fs.py
Expand Up @@ -4,7 +4,7 @@
import os
from dataclasses import dataclass
from pathlib import Path
from typing import TYPE_CHECKING, Iterable, Optional, Tuple
from typing import Any, Iterable, Optional, Tuple

from pants.engine.collection import Collection
from pants.engine.rules import RootRule, side_effecting
Expand All @@ -18,9 +18,6 @@
)
from pants.util.meta import frozen_after_init

if TYPE_CHECKING:
from pants.engine.internals.scheduler import SchedulerSession


@dataclass(frozen=True)
class FileContent:
Expand Down Expand Up @@ -285,7 +282,8 @@ class UrlToFetch:
class Workspace:
"""Abstract handle for operations that touch the real local filesystem."""

_scheduler: "SchedulerSession"
# TODO: SchedulerSession. Untyped because `fs.py` and `scheduler.py` have a cycle.
_scheduler: Any

def materialize_directory(self, directory_to_materialize: DirectoryToMaterialize):
"""Materialize one single directory digest to disk.
Expand Down
11 changes: 5 additions & 6 deletions src/python/pants/engine/interactive_process.py
Expand Up @@ -3,16 +3,13 @@

import itertools
from dataclasses import dataclass
from typing import TYPE_CHECKING, Iterable, Mapping, Optional, Tuple
from typing import Any, Iterable, Mapping, Optional, Tuple, cast

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:
Expand Down Expand Up @@ -59,11 +56,13 @@ def __post_init__(self):
@side_effecting
@dataclass(frozen=True)
class InteractiveRunner:
_scheduler: "SchedulerSession"
_scheduler: Any

def run_process(self, request: InteractiveProcess) -> InteractiveProcessResult:
ExceptionSink.toggle_ignoring_sigint_v2_engine(True)
return self._scheduler.run_local_interactive_process(request)
return cast(
InteractiveProcessResult, self._scheduler.run_local_interactive_process(request)
)


def rules():
Expand Down
28 changes: 2 additions & 26 deletions src/python/pants/engine/internals/graph.py
Expand Up @@ -141,32 +141,8 @@ 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]))

# 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))
dependencies = await MultiGet(Get(TransitiveTarget, Address, d) for d in dependency_addresses)
return TransitiveTarget(root, dependencies)


@rule
Expand Down
29 changes: 0 additions & 29 deletions src/python/pants/engine/internals/graph_test.py
Expand Up @@ -133,35 +133,6 @@ 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(
Expand Down