Skip to content

Commit

Permalink
Fix a regression in dependency inference + the dependees goal
Browse files Browse the repository at this point in the history
Now that we store `--tag` and `--exclude-target-regexp` on `AddressMapper`, rather than `AddressSpecs`, we end up always using those values and always filtering. This is an issue in some places where we use `Get(Addresses, AddressSpecs)` to learn about the universe of targets - we don't want to filter in those cases.

We solve this by adding a field to `AddressSpecs` called `apply_target_filters`. It does not have a default value because we want the author to have to explicitly decide what behavior they want.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
  • Loading branch information
Eric-Arellano committed Aug 6, 2020
1 parent a88f746 commit 976b732
Show file tree
Hide file tree
Showing 8 changed files with 58 additions and 16 deletions.
4 changes: 3 additions & 1 deletion src/python/pants/backend/project_info/dependees.py
Expand Up @@ -27,7 +27,9 @@ class AddressToDependees:
@rule
async def map_addresses_to_dependees() -> AddressToDependees:
# Get every target in the project so that we can iterate over them to find their dependencies.
all_explicit_targets = await Get(Targets, AddressSpecs([DescendantAddresses("")]))
all_explicit_targets = await Get(
Targets, AddressSpecs([DescendantAddresses("")], apply_target_filters=False)
)
dependencies_per_target = await MultiGet(
Get(Addresses, DependenciesRequest(tgt.get(Dependencies))) for tgt in all_explicit_targets
)
Expand Down
Expand Up @@ -64,7 +64,9 @@ def address_for_module(self, module: str) -> Optional[Address]:

@rule
async def map_first_party_modules_to_addresses() -> FirstPartyModuleToAddressMapping:
all_explicit_targets = await Get(Targets, AddressSpecs([DescendantAddresses("")]))
all_explicit_targets = await Get(
Targets, AddressSpecs([DescendantAddresses("")], apply_target_filters=False)
)
candidate_explicit_targets = tuple(
tgt for tgt in all_explicit_targets if tgt.has_field(PythonSources)
)
Expand Down Expand Up @@ -119,7 +121,9 @@ def address_for_module(self, module: str) -> Optional[Address]:

@rule
async def map_third_party_modules_to_addresses() -> ThirdPartyModuleToAddressMapping:
all_targets = await Get(Targets, AddressSpecs([DescendantAddresses("")]))
all_targets = await Get(
Targets, AddressSpecs([DescendantAddresses("")], apply_target_filters=False)
)
modules_to_addresses: Dict[str, Address] = {}
modules_with_multiple_owners: Set[str] = set()
for tgt in all_targets:
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/backend/python/rules/run_setup_py.py
Expand Up @@ -588,7 +588,7 @@ async def get_exporting_owner(owned_dependency: OwnedDependency) -> ExportedTarg
"""
target = owned_dependency.target
ancestor_addrs = AscendantAddresses(target.address.spec_path)
ancestor_tgts = await Get(Targets, AddressSpecs([ancestor_addrs]))
ancestor_tgts = await Get(Targets, AddressSpecs([ancestor_addrs], apply_target_filters=False))
# Note that addresses sort by (spec_path, target_name), and all these targets are
# ancestors of the given target, i.e., their spec_paths are all prefixes. So sorting by
# address will effectively sort by closeness of ancestry to the given target.
Expand Down
25 changes: 23 additions & 2 deletions src/python/pants/base/specs.py
Expand Up @@ -5,13 +5,14 @@
import os
from abc import ABC, ABCMeta, abstractmethod
from dataclasses import dataclass
from typing import Any, Dict, Iterable, 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 GlobExpansionConjunction, GlobMatchErrorBehavior, PathGlobs
from pants.util.collections import assert_single_element
from pants.util.dirutil import fast_relpath_optional, recursive_dirname
from pants.util.memo import memoized_property
from pants.util.meta import frozen_after_init


class Spec(ABC):
Expand Down Expand Up @@ -212,7 +213,21 @@ def to_globs(self, build_patterns: Iterable[str]) -> Tuple[str, ...]:
)


class AddressSpecs(Collection[AddressSpec]):
@frozen_after_init
@dataclass(unsafe_hash=True)
class AddressSpecs:
specs: Tuple[AddressSpec, ...]
apply_target_filters: bool

def __init__(self, specs: Iterable[AddressSpec], *, apply_target_filters: bool) -> None:
"""Create the specs for what addresses Pants should run on.
If `apply_target_filters` is set to True, then the resulting Addresses will be filtered by
the global options `--tag` and `--exclude-target-regexp`.
"""
self.specs = tuple(specs)
self.apply_target_filters = apply_target_filters

@staticmethod
def more_specific(spec1: Optional[AddressSpec], spec2: Optional[AddressSpec]) -> AddressSpec:
# Note that if either of spec1 or spec2 is None, the other will be returned.
Expand All @@ -237,6 +252,12 @@ def to_path_globs(
ignores = (f"!{p}" for p in build_ignore_patterns)
return PathGlobs(globs=(*includes, *ignores))

def __iter__(self) -> Iterator[AddressSpec]:
return iter(self.specs)

def __bool__(self) -> bool:
return bool(self.specs)


class FilesystemSpec(Spec, metaclass=ABCMeta):
pass
Expand Down
5 changes: 4 additions & 1 deletion src/python/pants/engine/internals/build_files.py
Expand Up @@ -207,7 +207,10 @@ async def addresses_with_origins_from_address_specs(
matched_addresses.update(
bfaddr.address
for (bfaddr, tgt) in all_bfaddr_tgt_pairs
if address_mapper.matches_filter_options(bfaddr.address, tgt)
if (
address_specs.apply_target_filters is False
or address_mapper.matches_filter_options(bfaddr.address, tgt)
)
)

return AddressesWithOrigins(
Expand Down
16 changes: 10 additions & 6 deletions src/python/pants/engine/internals/build_files_test.py
Expand Up @@ -89,7 +89,7 @@ def test_address_specs_duplicated() -> None:
address_family = AddressFamily(
"root", {"root": ("root/BUILD", TargetAdaptor(type_alias="", name="root"))}
)
address_specs = AddressSpecs([address_spec, address_spec])
address_specs = AddressSpecs([address_spec, address_spec], apply_target_filters=True)

addresses_with_origins = resolve_addresses_with_origins_from_address_specs(
address_specs, address_family
Expand All @@ -102,7 +102,7 @@ def test_address_specs_duplicated() -> None:

def test_address_specs_tag_filter() -> None:
"""Test that targets are filtered based on `tags`."""
address_specs = AddressSpecs([SiblingAddresses("root")])
address_specs = AddressSpecs([SiblingAddresses("root")], apply_target_filters=True)
address_family = AddressFamily(
"root",
{
Expand All @@ -126,22 +126,26 @@ def test_address_specs_fail_on_nonexistent() -> None:
address_family = AddressFamily(
"root", {"a": ("root/BUILD", TargetAdaptor(type_alias="", name="a"))}
)
address_specs = AddressSpecs([SingleAddress("root", "b"), SingleAddress("root", "a")])
address_specs = AddressSpecs(
[SingleAddress("root", "b"), SingleAddress("root", "a")], apply_target_filters=True
)

expected_rx_str = re.escape("'b' was not found in namespace 'root'. Did you mean one of:\n :a")
with pytest.raises(ResolveError, match=expected_rx_str):
resolve_addresses_with_origins_from_address_specs(address_specs, address_family)

# Ensure that we still catch nonexistent targets later on in the list of command-line
# address specs.
address_specs = AddressSpecs([SingleAddress("root", "a"), SingleAddress("root", "b")])
address_specs = AddressSpecs(
[SingleAddress("root", "a"), SingleAddress("root", "b")], apply_target_filters=True
)
with pytest.raises(ResolveError, match=expected_rx_str):
resolve_addresses_with_origins_from_address_specs(address_specs, address_family)


def test_address_specs_exclude_pattern() -> None:
"""Test that targets are filtered based on exclude patterns."""
address_specs = AddressSpecs([SiblingAddresses("root")])
address_specs = AddressSpecs([SiblingAddresses("root")], apply_target_filters=True)
address_family = AddressFamily(
"root",
{
Expand All @@ -161,7 +165,7 @@ def test_address_specs_exclude_pattern() -> None:

def test_address_specs_exclude_pattern_with_single_address() -> None:
"""Test that single address targets are filtered based on exclude patterns."""
address_specs = AddressSpecs([SingleAddress("root", "not_me")])
address_specs = AddressSpecs([SingleAddress("root", "not_me")], apply_target_filters=True)
address_family = AddressFamily(
"root", {"not_me": ("root/BUILD", TargetAdaptor(type_alias="", name="not_me"))}
)
Expand Down
4 changes: 3 additions & 1 deletion src/python/pants/engine/internals/graph.py
Expand Up @@ -250,7 +250,9 @@ async def find_owners(owners_request: OwnersRequest) -> Owners:

# Walk up the buildroot looking for targets that would conceivably claim changed sources.
candidate_specs = tuple(AscendantAddresses(directory=d) for d in dirs_set)
candidate_targets = await Get(Targets, AddressSpecs(candidate_specs))
candidate_targets = await Get(
Targets, AddressSpecs(candidate_specs, apply_target_filters=False)
)
candidate_target_sources = await MultiGet(
Get(HydratedSources, HydrateSourcesRequest(tgt.get(Sources))) for tgt in candidate_targets
)
Expand Down
10 changes: 8 additions & 2 deletions src/python/pants/init/specs_calculator.py
Expand Up @@ -47,7 +47,10 @@ def parse_specs(cls, raw_specs: Iterable[str], *, build_root: Optional[str] = No
else:
filesystem_specs.add(parsed_spec)

return Specs(AddressSpecs(address_specs), FilesystemSpecs(filesystem_specs))
return Specs(
AddressSpecs(address_specs, apply_target_filters=True),
FilesystemSpecs(filesystem_specs),
)

@classmethod
def create(
Expand Down Expand Up @@ -104,4 +107,7 @@ def create(
else:
address_specs.append(SingleAddress(address.spec_path, address.target_name))

return Specs(AddressSpecs(address_specs), FilesystemSpecs(filesystem_specs))
return Specs(
AddressSpecs(address_specs, apply_target_filters=True),
FilesystemSpecs(filesystem_specs),
)

0 comments on commit 976b732

Please sign in to comment.