From fd13827425a69a64fb1864b9816a1ab724f268ef Mon Sep 17 00:00:00 2001 From: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> Date: Fri, 20 May 2022 11:50:29 -0500 Subject: [PATCH] Revert `tags` changes, except for the `python_tests` bug fix (#15553) Revert https://github.com/pantsbuild/pants/pull/15413. It did fix two bugs, but was too risky. In particular, it resulted in https://github.com/pantsbuild/pants/issues/15544. I wasn't able to cherry-pick the fix https://github.com/pantsbuild/pants/pull/15546 because the tests have diverged too much. So instead, we should revert. However, we keep the fix for `python_tests`, which was a 2.11-specific regression. [ci skip-rust] --- .../backend/codegen/export_codegen_goal.py | 4 +- src/python/pants/core/goals/check.py | 4 +- src/python/pants/core/goals/export.py | 8 +- src/python/pants/core/goals/fmt.py | 4 +- src/python/pants/core/goals/lint.py | 4 +- src/python/pants/core/goals/repl.py | 8 +- .../pants/engine/internals/build_files.py | 8 +- src/python/pants/engine/internals/graph.py | 31 ++----- .../pants/engine/internals/graph_test.py | 81 +------------------ src/python/pants/engine/internals/mapper.py | 2 +- .../pants/engine/internals/mapper_test.py | 11 ++- src/python/pants/engine/target.py | 19 ----- src/python/pants/vcs/changed.py | 2 +- 13 files changed, 38 insertions(+), 148 deletions(-) diff --git a/src/python/pants/backend/codegen/export_codegen_goal.py b/src/python/pants/backend/codegen/export_codegen_goal.py index fcf5a2658d7..57fdf1ae56e 100644 --- a/src/python/pants/backend/codegen/export_codegen_goal.py +++ b/src/python/pants/backend/codegen/export_codegen_goal.py @@ -8,12 +8,12 @@ from pants.engine.goal import Goal, GoalSubsystem from pants.engine.rules import Get, MultiGet, collect_rules, goal_rule from pants.engine.target import ( - FilteredTargets, GenerateSourcesRequest, HydratedSources, HydrateSourcesRequest, RegisteredTargetTypes, SourcesField, + Targets, ) from pants.engine.unions import UnionMembership @@ -35,7 +35,7 @@ class ExportCodegen(Goal): @goal_rule async def export_codegen( - targets: FilteredTargets, + targets: Targets, union_membership: UnionMembership, workspace: Workspace, dist_dir: DistDir, diff --git a/src/python/pants/core/goals/check.py b/src/python/pants/core/goals/check.py index f6bc3982d65..fe3d256f92e 100644 --- a/src/python/pants/core/goals/check.py +++ b/src/python/pants/core/goals/check.py @@ -21,7 +21,7 @@ from pants.engine.goal import Goal, GoalSubsystem from pants.engine.process import FallibleProcessResult from pants.engine.rules import Get, MultiGet, QueryRule, collect_rules, goal_rule -from pants.engine.target import FilteredTargets +from pants.engine.target import Targets from pants.engine.unions import UnionMembership, union from pants.option.option_types import StrListOption from pants.util.logging import LogLevel @@ -160,7 +160,7 @@ class Check(Goal): async def check( console: Console, workspace: Workspace, - targets: FilteredTargets, + targets: Targets, dist_dir: DistDir, union_membership: UnionMembership, check_subsystem: CheckSubsystem, diff --git a/src/python/pants/core/goals/export.py b/src/python/pants/core/goals/export.py index a9129223292..da94a3d9e37 100644 --- a/src/python/pants/core/goals/export.py +++ b/src/python/pants/core/goals/export.py @@ -5,7 +5,7 @@ import os from dataclasses import dataclass -from typing import Iterable, Mapping, Sequence, cast +from typing import Iterable, Mapping, cast from pants.base.build_root import BuildRoot from pants.core.util_rules.distdir import DistDir @@ -17,7 +17,7 @@ from pants.engine.internals.selectors import Effect, Get, MultiGet from pants.engine.process import InteractiveProcess, InteractiveProcessResult from pants.engine.rules import collect_rules, goal_rule -from pants.engine.target import FilteredTargets, Target +from pants.engine.target import Targets from pants.engine.unions import UnionMembership, union from pants.util.dirutil import safe_rmtree from pants.util.frozendict import FrozenDict @@ -36,7 +36,7 @@ class ExportRequest: Subclass and install a member of this type to export data. """ - targets: Sequence[Target] + targets: Targets @frozen_after_init @@ -107,7 +107,7 @@ class Export(Goal): @goal_rule async def export( console: Console, - targets: FilteredTargets, + targets: Targets, workspace: Workspace, union_membership: UnionMembership, build_root: BuildRoot, diff --git a/src/python/pants/core/goals/fmt.py b/src/python/pants/core/goals/fmt.py index 51f862e2f39..f1a9e8d772d 100644 --- a/src/python/pants/core/goals/fmt.py +++ b/src/python/pants/core/goals/fmt.py @@ -21,7 +21,7 @@ from pants.engine.goal import Goal, GoalSubsystem from pants.engine.process import FallibleProcessResult, ProcessResult from pants.engine.rules import Get, MultiGet, collect_rules, goal_rule, rule -from pants.engine.target import FilteredTargets, SourcesField, Targets +from pants.engine.target import SourcesField, Targets from pants.engine.unions import UnionMembership, union from pants.option.option_types import IntOption, StrListOption from pants.util.collections import partition_sequentially @@ -155,7 +155,7 @@ class Fmt(Goal): @goal_rule async def fmt( console: Console, - targets: FilteredTargets, + targets: Targets, fmt_subsystem: FmtSubsystem, workspace: Workspace, union_membership: UnionMembership, diff --git a/src/python/pants/core/goals/lint.py b/src/python/pants/core/goals/lint.py index d3b0573b926..55e4c248642 100644 --- a/src/python/pants/core/goals/lint.py +++ b/src/python/pants/core/goals/lint.py @@ -22,7 +22,7 @@ from pants.engine.goal import Goal, GoalSubsystem from pants.engine.process import FallibleProcessResult from pants.engine.rules import Get, MultiGet, collect_rules, goal_rule -from pants.engine.target import FieldSet, FilteredTargets +from pants.engine.target import FieldSet, Targets from pants.engine.unions import UnionMembership, union from pants.option.option_types import IntOption, StrListOption from pants.util.collections import partition_sequentially @@ -185,7 +185,7 @@ class Lint(Goal): async def lint( console: Console, workspace: Workspace, - targets: FilteredTargets, + targets: Targets, specs_snapshot: SpecsSnapshot, lint_subsystem: LintSubsystem, union_membership: UnionMembership, diff --git a/src/python/pants/core/goals/repl.py b/src/python/pants/core/goals/repl.py index 9be5f19a63f..4d5f2883055 100644 --- a/src/python/pants/core/goals/repl.py +++ b/src/python/pants/core/goals/repl.py @@ -6,7 +6,7 @@ from abc import ABC from dataclasses import dataclass from pathlib import PurePath -from typing import ClassVar, Iterable, Mapping, Optional, Sequence, Tuple +from typing import ClassVar, Iterable, Mapping, Optional, Tuple from pants.base.build_root import BuildRoot from pants.engine.addresses import Addresses @@ -17,7 +17,7 @@ from pants.engine.internals.native_engine import EMPTY_DIGEST from pants.engine.process import InteractiveProcess, InteractiveProcessResult from pants.engine.rules import Effect, Get, collect_rules, goal_rule -from pants.engine.target import FilteredTargets, Target +from pants.engine.target import Targets from pants.engine.unions import UnionMembership, union from pants.option.global_options import GlobalOptions from pants.option.option_types import BoolOption, StrOption @@ -37,7 +37,7 @@ class ReplImplementation(ABC): name: ClassVar[str] - targets: Sequence[Target] + targets: Targets chroot: str # Absolute path of the chroot the sources will be materialized to. def in_chroot(self, relpath: str) -> str: @@ -102,7 +102,7 @@ async def run_repl( console: Console, workspace: Workspace, repl_subsystem: ReplSubsystem, - specified_targets: FilteredTargets, + specified_targets: Targets, build_root: BuildRoot, union_membership: UnionMembership, global_options: GlobalOptions, diff --git a/src/python/pants/engine/internals/build_files.py b/src/python/pants/engine/internals/build_files.py index 6557f0cf1b1..cd7f7874f5c 100644 --- a/src/python/pants/engine/internals/build_files.py +++ b/src/python/pants/engine/internals/build_files.py @@ -14,7 +14,7 @@ from pants.engine.addresses import Address, Addresses, AddressInput, BuildFileAddress from pants.engine.engine_aware import EngineAwareParameter from pants.engine.fs import DigestContents, GlobMatchErrorBehavior, PathGlobs, Paths -from pants.engine.internals.mapper import AddressFamily, AddressMap, SpecsFilter +from pants.engine.internals.mapper import AddressFamily, AddressMap, AddressSpecsFilter from pants.engine.internals.parametrize import _TargetParametrizations from pants.engine.internals.parser import BuildFilePreludeSymbols, Parser, error_on_imports from pants.engine.internals.target_adaptor import TargetAdaptor @@ -172,8 +172,8 @@ async def find_target_adaptor(address: Address) -> TargetAdaptor: @rule -def setup_address_specs_filter(global_options: GlobalOptions) -> SpecsFilter: - return SpecsFilter( +def setup_address_specs_filter(global_options: GlobalOptions) -> AddressSpecsFilter: + return AddressSpecsFilter( tags=global_options.tag, exclude_target_regexps=global_options.exclude_target_regexp ) @@ -182,7 +182,7 @@ def setup_address_specs_filter(global_options: GlobalOptions) -> SpecsFilter: async def addresses_from_address_specs( address_specs: AddressSpecs, build_file_options: BuildFileOptions, - specs_filter: SpecsFilter, + specs_filter: AddressSpecsFilter, ) -> Addresses: matched_addresses: OrderedSet[Address] = OrderedSet() filtering_disabled = address_specs.filter_by_global_options is False diff --git a/src/python/pants/engine/internals/graph.py b/src/python/pants/engine/internals/graph.py index 4914f95c281..af01ef8b3d0 100644 --- a/src/python/pants/engine/internals/graph.py +++ b/src/python/pants/engine/internals/graph.py @@ -39,7 +39,6 @@ SpecsSnapshot, ) from pants.engine.internals import native_engine -from pants.engine.internals.mapper import SpecsFilter from pants.engine.internals.parametrize import Parametrize, _TargetParametrization from pants.engine.internals.parametrize import ( # noqa: F401 _TargetParametrizations as _TargetParametrizations, @@ -60,7 +59,6 @@ FieldSet, FieldSetsPerTarget, FieldSetsPerTargetRequest, - FilteredTargets, GeneratedSources, GeneratedTargets, GenerateSourcesRequest, @@ -344,11 +342,6 @@ async def resolve_targets( return Targets(expanded_targets) -@rule -def filter_targets(targets: Targets, specs_filter: SpecsFilter) -> FilteredTargets: - return FilteredTargets(tgt for tgt in targets if specs_filter.matches(tgt)) - - @rule(desc="Find all targets in the project", level=LogLevel.DEBUG) async def find_all_targets(_: AllTargetsRequest) -> AllTargets: tgts = await Get(Targets, AddressSpecs([MaybeEmptyDescendantAddresses("")])) @@ -619,7 +612,6 @@ class OwnersRequest: sources: tuple[str, ...] owners_not_found_behavior: OwnersNotFoundBehavior = OwnersNotFoundBehavior.ignore - filter_by_global_options: bool = False class Owners(Collection[Address]): @@ -642,18 +634,10 @@ async def find_owners(owners_request: OwnersRequest) -> Owners: # glob. live_candidate_specs = tuple(AscendantAddresses(directory=d) for d in live_dirs) deleted_candidate_specs = tuple(AscendantAddresses(directory=d) for d in deleted_dirs) - live_get: Get[FilteredTargets | Targets, AddressSpecs] - if owners_request.filter_by_global_options: - live_get = Get( - FilteredTargets, AddressSpecs(live_candidate_specs, filter_by_global_options=True) - ) - deleted_get = Get( - UnexpandedTargets, AddressSpecs(deleted_candidate_specs, filter_by_global_options=True) - ) - else: - live_get = Get(Targets, AddressSpecs(live_candidate_specs)) - deleted_get = Get(UnexpandedTargets, AddressSpecs(deleted_candidate_specs)) - live_candidate_tgts, deleted_candidate_tgts = await MultiGet(live_get, deleted_get) + live_candidate_tgts, deleted_candidate_tgts = await MultiGet( + Get(Targets, AddressSpecs(live_candidate_specs)), + Get(UnexpandedTargets, AddressSpecs(deleted_candidate_specs)), + ) matching_addresses: OrderedSet[Address] = OrderedSet() unmatched_sources = set(owners_request.sources) @@ -678,7 +662,7 @@ async def find_owners(owners_request: OwnersRequest) -> Owners: # primary ownership, but the target still should match the file. We can't use # `tgt.get()` because this is a mixin, and there technically may be >1 field. secondary_owner_fields = tuple( - field + field # type: ignore[misc] for field in candidate_tgt.field_values.values() if isinstance(field, SecondaryOwnerMixin) ) @@ -763,8 +747,7 @@ async def addresses_from_filesystem_specs( for spec in filesystem_specs.file_includes ) owners_per_include = await MultiGet( - Get(Owners, OwnersRequest(paths.files, filter_by_global_options=True)) - for paths in paths_per_include + Get(Owners, OwnersRequest(sources=paths.files)) for paths in paths_per_include ) addresses: set[Address] = set() for spec, owners in zip(filesystem_specs.file_includes, owners_per_include): @@ -1320,7 +1303,7 @@ async def find_valid_field_sets_for_target_roots( ) -> TargetRootsToFieldSets: # NB: This must be in an `await Get`, rather than the rule signature, to avoid a rule graph # issue. - targets = await Get(FilteredTargets, Specs, specs) + targets = await Get(Targets, Specs, specs) field_sets_per_target = await Get( FieldSetsPerTarget, FieldSetsPerTargetRequest(request.field_set_superclass, targets) ) diff --git a/src/python/pants/engine/internals/graph_test.py b/src/python/pants/engine/internals/graph_test.py index 473ae3bf226..ef7cb66ebdb 100644 --- a/src/python/pants/engine/internals/graph_test.py +++ b/src/python/pants/engine/internals/graph_test.py @@ -15,7 +15,6 @@ from pants.base.specs import ( AddressLiteralSpec, AddressSpecs, - DescendantAddresses, FileGlobSpec, FileLiteralSpec, FilesystemSpec, @@ -58,7 +57,6 @@ DependenciesRequest, ExplicitlyProvidedDependencies, FieldSet, - FilteredTargets, GeneratedSources, GenerateSourcesRequest, HydratedSources, @@ -107,13 +105,7 @@ class SpecialCasedDeps2(SpecialCasedDependencies): class MockTarget(Target): alias = "target" - core_fields = ( - MockDependencies, - MultipleSourcesField, - SpecialCasedDeps1, - SpecialCasedDeps2, - Tags, - ) + core_fields = (MockDependencies, MultipleSourcesField, SpecialCasedDeps1, SpecialCasedDeps2) deprecated_alias = "deprecated_target" deprecated_alias_removal_version = "9.9.9.dev0" @@ -144,7 +136,6 @@ def transitive_targets_rule_runner() -> RuleRunner: QueryRule(CoarsenedTargets, [Addresses]), QueryRule(Targets, [DependenciesRequest]), QueryRule(TransitiveTargets, [TransitiveTargetsRequest]), - QueryRule(FilteredTargets, [Specs]), ], target_types=[MockTarget, MockTargetGenerator, MockGeneratedTarget], ) @@ -627,66 +618,6 @@ def test_find_all_targets(transitive_targets_rule_runner: RuleRunner) -> None: assert {t.address for t in all_unexpanded} == {*expected, Address("", target_name="generator")} -def test_filtered_targets(transitive_targets_rule_runner: RuleRunner) -> None: - transitive_targets_rule_runner.write_files( - { - "addr_specs/f1.txt": "", - "addr_specs/f2.txt": "", - "addr_specs/BUILD": dedent( - """\ - generator( - sources=["*.txt"], - tags=["a"], - overrides={"f2.txt": {"tags": ["b"]}}, - ) - - target(name='t', tags=["a"]) - """ - ), - "fs_specs/f1.txt": "", - "fs_specs/f2.txt": "", - "fs_specs/BUILD": dedent( - """\ - generator( - sources=["*.txt"], - tags=["a"], - overrides={"f2.txt": {"tags": ["b"]}}, - ) - - target(name='t', sources=["f1.txt"], tags=["a"]) - """ - ), - } - ) - specs = Specs( - AddressSpecs([DescendantAddresses("addr_specs")], filter_by_global_options=True), - FilesystemSpecs([FileGlobSpec("fs_specs/*.txt")]), - ) - - def check(tags_option: str | None, expected: set[Address]) -> None: - if tags_option: - transitive_targets_rule_runner.set_options([f"--tag={tags_option}"]) - result = transitive_targets_rule_runner.request(FilteredTargets, [specs]) - assert {t.address for t in result} == expected - - addr_f1 = Address("addr_specs", relative_file_path="f1.txt") - addr_f2 = Address("addr_specs", relative_file_path="f2.txt") - addr_direct = Address("addr_specs", target_name="t") - - fs_f1 = Address("fs_specs", relative_file_path="f1.txt") - fs_f2 = Address("fs_specs", relative_file_path="f2.txt") - fs_direct = Address("fs_specs", target_name="t") - - all_a_tags = {addr_f1, addr_direct, fs_f1, fs_direct} - all_b_tags = {addr_f2, fs_f2} - - check(None, {*all_a_tags, *all_b_tags}) - check("a", all_a_tags) - check("b", all_b_tags) - check("-a", all_b_tags) - check("-b", all_a_tags) - - def test_resolve_specs_snapshot() -> None: """This tests that convert filesystem specs and/or address specs into a single snapshot. @@ -910,22 +841,18 @@ def test_filesystem_specs_glob(specs_rule_runner: RuleRunner) -> None: """\ generator(name='generator', sources=['*.txt']) target(name='not-generator', sources=['*.txt']) - target(name='skip-me', sources=['*.txt']) - target(name='bad-tag', sources=['*.txt'], tags=['skip']) """ ), } ) - specs_rule_runner.set_options(["--tag=-skip", "--exclude-target-regexp=skip-me"]) - all_unskipped_addresses = [ + all_addresses = [ Address("demo", target_name="not-generator"), Address("demo", target_name="generator", relative_file_path="f1.txt"), Address("demo", target_name="generator", relative_file_path="f2.txt"), ] assert ( - resolve_filesystem_specs(specs_rule_runner, [FileGlobSpec("demo/*.txt")]) - == all_unskipped_addresses + resolve_filesystem_specs(specs_rule_runner, [FileGlobSpec("demo/*.txt")]) == all_addresses ) # We should deduplicate between glob and literal specs. assert ( @@ -933,7 +860,7 @@ def test_filesystem_specs_glob(specs_rule_runner: RuleRunner) -> None: specs_rule_runner, [FileGlobSpec("demo/*.txt"), FileLiteralSpec("demo/f1.txt")], ) - == all_unskipped_addresses + == all_addresses ) diff --git a/src/python/pants/engine/internals/mapper.py b/src/python/pants/engine/internals/mapper.py index 2fa1a0e2d3c..3d551e9127e 100644 --- a/src/python/pants/engine/internals/mapper.py +++ b/src/python/pants/engine/internals/mapper.py @@ -154,7 +154,7 @@ def __repr__(self) -> str: @frozen_after_init @dataclass(unsafe_hash=True) -class SpecsFilter: +class AddressSpecsFilter: """Filters addresses with the `--tags` and `--exclude-target-regexp` options.""" tags: tuple[str, ...] diff --git a/src/python/pants/engine/internals/mapper_test.py b/src/python/pants/engine/internals/mapper_test.py index 30e0b63591a..bd4d31f78e8 100644 --- a/src/python/pants/engine/internals/mapper_test.py +++ b/src/python/pants/engine/internals/mapper_test.py @@ -12,9 +12,9 @@ from pants.engine.internals.mapper import ( AddressFamily, AddressMap, + AddressSpecsFilter, DifferingFamiliesError, DuplicateNameError, - SpecsFilter, ) from pants.engine.internals.parser import BuildFilePreludeSymbols, Parser from pants.engine.internals.target_adaptor import TargetAdaptor @@ -130,8 +130,8 @@ def test_address_family_duplicate_names() -> None: ) -def test_specs_filter() -> None: - specs_filter = SpecsFilter(tags=["-a", "+b"], exclude_target_regexps=["skip-me"]) +def test_address_specs_filter_tags() -> None: + specs_filter = AddressSpecsFilter(tags=["-a", "+b"]) class MockTgt(Target): alias = "tgt" @@ -142,12 +142,11 @@ def make_tgt(name: str, tags: list[str] | None = None) -> MockTgt: untagged_tgt = make_tgt(name="untagged") b_tagged_tgt = make_tgt(name="b-tagged", tags=["b"]) - b_tagged_exclude_regex_tgt = make_tgt(name="skip-me", tags=["b"]) a_and_b_tagged_tgt = make_tgt(name="a-and-b-tagged", tags=["a", "b"]) def matches(tgt: MockTgt) -> bool: return specs_filter.matches(tgt) + assert matches(untagged_tgt) is False assert matches(b_tagged_tgt) is True - for t in (untagged_tgt, b_tagged_exclude_regex_tgt, a_and_b_tagged_tgt): - assert matches(t) is False + assert matches(a_and_b_tagged_tgt) is False diff --git a/src/python/pants/engine/target.py b/src/python/pants/engine/target.py index d0013bc4d73..bd11a084706 100644 --- a/src/python/pants/engine/target.py +++ b/src/python/pants/engine/target.py @@ -652,25 +652,6 @@ def expect_single(self) -> Target: return self[0] -# This distinct type is necessary because of https://github.com/pantsbuild/pants/issues/14977. -# -# NB: We still proactively apply filtering inside `AddressSpecs` and `FilesystemSpecs`, which is -# earlier in the rule pipeline of `Specs -> Addresses -> UnexpandedTargets -> Targets -> -# FilteredTargets`. That is necessary so that project-introspection goals like `list` which don't -# use `FilteredTargets` still have filtering applied. -class FilteredTargets(Collection[Target]): - """A heterogenous collection of Target instances that have been filtered with the global options - `--tag` and `--exclude-target-regexp`. - - Outside of the extra filtering, this type is identical to `Targets`, including its handling of - target generators. - """ - - def expect_single(self) -> Target: - assert_single_address([tgt.address for tgt in self]) - return self[0] - - class UnexpandedTargets(Collection[Target]): """Like `Targets`, but will not replace target generators with their generated targets (e.g. replace `python_sources` "BUILD targets" with generated `python_source` "file targets").""" diff --git a/src/python/pants/vcs/changed.py b/src/python/pants/vcs/changed.py index 7f359492fd6..575838bf3eb 100644 --- a/src/python/pants/vcs/changed.py +++ b/src/python/pants/vcs/changed.py @@ -39,7 +39,7 @@ class ChangedAddresses(Collection[Address]): @rule async def find_changed_owners(request: ChangedRequest) -> ChangedAddresses: - owners = await Get(Owners, OwnersRequest(request.sources, filter_by_global_options=True)) + owners = await Get(Owners, OwnersRequest(request.sources)) if request.dependees == DependeesOption.NONE: return ChangedAddresses(owners) dependees_with_roots = await Get(