From ed217e216dd820c76e4281dc0e2fa203bb289846 Mon Sep 17 00:00:00 2001 From: Josh Cannon Date: Thu, 31 Aug 2023 15:06:09 -0500 Subject: [PATCH] Remove last bits of `SecondaryOwnershipMixin` (#19742) This change removes much It's something I have worked towards And now it is done --- .../pants/backend/python/target_types.py | 13 +-- .../pants/backend/python/target_types_test.py | 14 --- .../pants/backend/python/util_rules/faas.py | 14 +-- .../backend/python/util_rules/faas_test.py | 11 +-- .../pants/base/specs_integration_test.py | 4 - src/python/pants/core/goals/run.py | 68 ++----------- src/python/pants/core/goals/run_test.py | 89 +---------------- src/python/pants/engine/internals/graph.py | 31 +----- .../pants/engine/internals/graph_test.py | 26 +---- .../pants/engine/internals/specs_rules.py | 66 +------------ .../engine/internals/specs_rules_test.py | 96 ------------------- src/python/pants/engine/target.py | 45 --------- 12 files changed, 19 insertions(+), 458 deletions(-) diff --git a/src/python/pants/backend/python/target_types.py b/src/python/pants/backend/python/target_types.py index c8b7421f923..a1a5264fd30 100644 --- a/src/python/pants/backend/python/target_types.py +++ b/src/python/pants/backend/python/target_types.py @@ -54,7 +54,6 @@ OptionalSingleSourceField, OverridesField, ScalarField, - SecondaryOwnerMixin, SingleSourceField, SpecialCasedDependencies, StringField, @@ -70,7 +69,6 @@ ) from pants.option.option_types import BoolOption from pants.option.subsystem import Subsystem -from pants.source.filespec import Filespec from pants.util.docutil import bin_name, doc_url, git_url from pants.util.frozendict import FrozenDict from pants.util.pip_requirement import PipRequirement @@ -298,7 +296,7 @@ def spec(self) -> str: return self.name -class EntryPointField(AsyncFieldMixin, SecondaryOwnerMixin, Field): +class EntryPointField(AsyncFieldMixin, Field): alias = "entry_point" default = None help = help_text( @@ -311,8 +309,6 @@ class EntryPointField(AsyncFieldMixin, SecondaryOwnerMixin, Field): 1) `'app.py'`, Pants will convert into the module `path.to.app`; 2) `'app.py:func'`, Pants will convert into `path.to.app:func`. - You must use the file name shorthand for file arguments to work with this target. - You may either set this field or the `script` field, but not both. Leave off both fields to have no entry point. """ @@ -331,13 +327,6 @@ def compute_value(cls, raw_value: Optional[str], address: Address) -> Optional[E except ValueError as e: raise InvalidFieldException(str(e)) - @property - def filespec(self) -> Filespec: - if self.value is None or not self.value.module.endswith(".py"): - return {"includes": []} - full_glob = os.path.join(self.address.spec_path, self.value.module) - return {"includes": [full_glob]} - class PexEntryPointField(EntryPointField): # Specialist subclass for use with `PexBinary` targets. diff --git a/src/python/pants/backend/python/target_types_test.py b/src/python/pants/backend/python/target_types_test.py index 5d7b363b268..7a04800cb3a 100644 --- a/src/python/pants/backend/python/target_types_test.py +++ b/src/python/pants/backend/python/target_types_test.py @@ -66,20 +66,6 @@ def create_tgt(*, script: str | None = None, entry_point: str | None = None) -> assert create_tgt(entry_point="foo")[PexEntryPointField].value == EntryPoint("foo") -@pytest.mark.parametrize( - ["entry_point", "expected"], - ( - ("path.to.module", []), - ("path.to.module:func", []), - ("lambda.py", ["project/dir/lambda.py"]), - ("lambda.py:func", ["project/dir/lambda.py"]), - ), -) -def test_entry_point_filespec(entry_point: str | None, expected: list[str]) -> None: - field = PexEntryPointField(entry_point, Address("project/dir")) - assert field.filespec == {"includes": expected} - - def test_entry_point_validation(caplog) -> None: addr = Address("src/python/project") diff --git a/src/python/pants/backend/python/util_rules/faas.py b/src/python/pants/backend/python/util_rules/faas.py index 7e054905c0e..0c27b6501ea 100644 --- a/src/python/pants/backend/python/util_rules/faas.py +++ b/src/python/pants/backend/python/util_rules/faas.py @@ -69,13 +69,11 @@ InferredDependencies, InvalidFieldException, InvalidTargetException, - SecondaryOwnerMixin, StringField, TransitiveTargets, TransitiveTargetsRequest, ) from pants.engine.unions import UnionRule -from pants.source.filespec import Filespec from pants.source.source_root import SourceRoot, SourceRootRequest from pants.util.docutil import bin_name from pants.util.strutil import help_text, softwrap @@ -83,7 +81,7 @@ logger = logging.getLogger(__name__) -class PythonFaaSHandlerField(StringField, AsyncFieldMixin, SecondaryOwnerMixin): +class PythonFaaSHandlerField(StringField, AsyncFieldMixin): alias = "handler" required = True value: str @@ -92,8 +90,6 @@ class PythonFaaSHandlerField(StringField, AsyncFieldMixin, SecondaryOwnerMixin): You can specify a full module like `'path.to.module:handler_func'` or use a shorthand to specify a file name, using the same syntax as the `sources` field, e.g. `'cloud_function.py:handler_func'`. - - You must use the file name shorthand for file arguments to work with this target. """ ) @@ -107,14 +103,6 @@ def compute_value(cls, raw_value: Optional[str], address: Address) -> str: ) return value - @property - def filespec(self) -> Filespec: - path, _, func = self.value.partition(":") - if not path.endswith(".py"): - return {"includes": []} - full_glob = os.path.join(self.address.spec_path, path) - return {"includes": [full_glob]} - @dataclass(frozen=True) class ResolvedPythonFaaSHandler: diff --git a/src/python/pants/backend/python/util_rules/faas_test.py b/src/python/pants/backend/python/util_rules/faas_test.py index 13566aa1cdf..643b438e07c 100644 --- a/src/python/pants/backend/python/util_rules/faas_test.py +++ b/src/python/pants/backend/python/util_rules/faas_test.py @@ -3,7 +3,7 @@ from __future__ import annotations from textwrap import dedent -from typing import List, Optional +from typing import Optional import pytest @@ -66,15 +66,6 @@ def test_handler_validation(invalid_handler: str) -> None: PythonFaaSHandlerField(invalid_handler, Address("", target_name="t")) -@pytest.mark.parametrize( - ["handler", "expected"], - (("path.to.module:func", []), ("lambda.py:func", ["project/dir/lambda.py"])), -) -def test_handler_filespec(handler: str, expected: List[str]) -> None: - field = PythonFaaSHandlerField(handler, Address("project/dir")) - assert field.filespec == {"includes": expected} - - def test_resolve_handler(rule_runner: RuleRunner) -> None: def assert_resolved( handler: str, *, expected_module: str, expected_func: str, is_file: bool diff --git a/src/python/pants/base/specs_integration_test.py b/src/python/pants/base/specs_integration_test.py index e45c767ec1d..2b208b0e84a 100644 --- a/src/python/pants/base/specs_integration_test.py +++ b/src/python/pants/base/specs_integration_test.py @@ -146,15 +146,11 @@ def test_descendent_addresses() -> None: def test_file_arg() -> None: """Semantics: find the 'owning' target, using generated target rather than target generator when possible (regardless of project introspection vs. "build" goal). - - Also, check that we support 'secondary ownership', e.g. a `pex_binary` being associated with - the the file `app.py` even though it does not have a `sources` field. """ with setup_tmpdir(SOURCES) as tmpdir: assert run( ["list", f"{tmpdir}/py/app.py", f"{tmpdir}/py/utils/strutil_test.py"] ).stdout.splitlines() == [ - f"{tmpdir}/py:bin", f"{tmpdir}/py/app.py:lib", f"{tmpdir}/py/utils/strutil_test.py:../tests", ] diff --git a/src/python/pants/core/goals/run.py b/src/python/pants/core/goals/run.py index 70f03b81bbb..b43eea977b3 100644 --- a/src/python/pants/core/goals/run.py +++ b/src/python/pants/core/goals/run.py @@ -3,23 +3,11 @@ from __future__ import annotations -import dataclasses import logging from abc import ABCMeta from dataclasses import dataclass from enum import Enum -from itertools import filterfalse, tee -from typing import ( - Callable, - ClassVar, - Iterable, - Mapping, - NamedTuple, - Optional, - Tuple, - TypeVar, - Union, -) +from typing import ClassVar, Iterable, Mapping, Optional, Tuple, TypeVar, Union from typing_extensions import final @@ -39,7 +27,6 @@ BoolField, FieldSet, NoApplicableTargetsBehavior, - SecondaryOwnerMixin, Target, TargetRootsToFieldSets, TargetRootsToFieldSetsRequest, @@ -212,18 +199,6 @@ class Run(Goal): environment_behavior = Goal.EnvironmentBehavior.LOCAL_ONLY -class RankedFieldSets(NamedTuple): - primary: tuple[RunFieldSet, ...] - secondary: tuple[RunFieldSet, ...] - - -def _partition( - iterable: Iterable[_T], pred: Callable[[_T], bool] -) -> tuple[tuple[_T, ...], tuple[_T, ...]]: - t1, t2 = tee(iterable) - return tuple(filter(pred, t2)), tuple(filterfalse(pred, t1)) - - async def _find_what_to_run( goal_description: str, ) -> tuple[RunFieldSet, Target]: @@ -233,49 +208,22 @@ async def _find_what_to_run( RunFieldSet, goal_description=goal_description, no_applicable_targets_behavior=NoApplicableTargetsBehavior.error, - warn_on_deprecated_secondary_owner_semantics=False, ), ) + mapping = targets_to_valid_field_sets.mapping - primary_target: Target | None = None - primary_target_rfs: RankedFieldSets | None = None - - for target, field_sets in targets_to_valid_field_sets.mapping.items(): - rfs = RankedFieldSets( - *_partition( - field_sets, - lambda field_set: not any( - isinstance(getattr(field_set, field.name), SecondaryOwnerMixin) - for field in dataclasses.fields(field_set) - ), - ) - ) - # In the case of multiple Targets/FieldSets, prefer the "primary" ones to the "secondary" ones. - if ( - primary_target is None - or primary_target_rfs is None # impossible, but satisfies mypy - or (rfs.primary and not primary_target_rfs.primary) - ): - primary_target = target - primary_target_rfs = rfs - elif (rfs.primary and primary_target_rfs.primary) or ( - rfs.secondary and primary_target_rfs.secondary - ): - raise TooManyTargetsException( - targets_to_valid_field_sets.mapping, goal_description=goal_description - ) + if len(mapping) > 1: + raise TooManyTargetsException(mapping, goal_description=goal_description) - assert primary_target is not None - assert primary_target_rfs is not None - field_sets = primary_target_rfs.primary or primary_target_rfs.secondary + target, field_sets = next(iter(mapping.items())) if len(field_sets) > 1: raise AmbiguousImplementationsException( - primary_target, - primary_target_rfs.primary + primary_target_rfs.secondary, + target, + field_sets, goal_description=goal_description, ) - return field_sets[0], primary_target + return field_sets[0], target @goal_rule diff --git a/src/python/pants/core/goals/run_test.py b/src/python/pants/core/goals/run_test.py index c0714176f39..9a620eeaa68 100644 --- a/src/python/pants/core/goals/run_test.py +++ b/src/python/pants/core/goals/run_test.py @@ -5,7 +5,6 @@ import os import sys -from dataclasses import dataclass from typing import Iterable, Mapping, cast import pytest @@ -29,16 +28,13 @@ ) from pants.engine.process import InteractiveProcess, InteractiveProcessResult from pants.engine.target import ( - Field, FieldSet, - SecondaryOwnerMixin, Target, TargetRootsToFieldSets, TargetRootsToFieldSetsRequest, ) from pants.engine.unions import UnionMembership from pants.option.global_options import GlobalOptions, KeepSandboxes -from pants.source.filespec import Filespec from pants.testutil.option_util import create_goal_subsystem, create_subsystem from pants.testutil.rule_runner import ( MockEffect, @@ -74,14 +70,6 @@ class TestRunFieldSet(RunFieldSet): run_in_sandbox_behavior = RunInSandboxBehavior.NOT_SUPPORTED -@dataclass(frozen=True) -class TestRunSecondaryFieldSet(RunFieldSet): - required_fields = () - run_in_sandbox_behavior = RunInSandboxBehavior.NOT_SUPPORTED - - just_borrowing: SecondaryOwnerField - - class TestBinaryTarget(Target): alias = "binary" core_fields = () @@ -144,8 +132,8 @@ def single_target_run( ], union_membership=UnionMembership( { - RunFieldSet: [TestRunFieldSet, TestRunSecondaryFieldSet], - RunDebugAdapterRequest: [TestRunFieldSet, TestRunSecondaryFieldSet], + RunFieldSet: [TestRunFieldSet], + RunDebugAdapterRequest: [TestRunFieldSet], }, ), ) @@ -202,76 +190,3 @@ def test_multi_field_set_error(rule_runner: RuleRunner) -> None: single_target_run( rule_runner, program_text=program_text, targets_to_field_sets={target: [fs1, fs2]} ) - - -class SecondaryOwnerField(SecondaryOwnerMixin, Field): - alias = "borrowed" - default = None - - @property - def filespec(self) -> Filespec: - return Filespec(includes=[]) - - -def test_filters_secondary_owners_single_target(rule_runner: RuleRunner) -> None: - program_text = f'#!{sys.executable}\nprint("hello")'.encode() - target = TestBinaryTarget({}, Address("some/addr")) - fs1 = TestRunFieldSet.create(target) - fs2 = TestRunSecondaryFieldSet.create(target) - res = single_target_run( - rule_runner, - program_text=program_text, - targets_to_field_sets={target: [fs1, fs2]}, - ) - assert res.exit_code == 0 - - -def test_filters_secondary_owners_multi_target(rule_runner: RuleRunner) -> None: - program_text = f'#!{sys.executable}\nprint("hello")'.encode() - t1 = TestBinaryTarget({}, Address("some/addr1")) - fs1 = TestRunFieldSet.create(t1) - t2 = TestBinaryTarget({}, Address("some/addr2")) - fs2 = TestRunSecondaryFieldSet.create(t2) - res = single_target_run( - rule_runner, - program_text=program_text, - targets_to_field_sets={t1: [fs1], t2: [fs2]}, - ) - assert res.exit_code == 0 - - -def test_only_secondary_owner_ok_single_target(rule_runner: RuleRunner) -> None: - program_text = f'#!{sys.executable}\nprint("hello")'.encode() - target = TestBinaryTarget({}, Address("some/addr")) - field_set = TestRunSecondaryFieldSet.create(target) - res = single_target_run( - rule_runner, - program_text=program_text, - targets_to_field_sets={target: [field_set]}, - ) - assert res.exit_code == 0 - - -def test_only_secondary_owner_error_multi_target(rule_runner: RuleRunner) -> None: - program_text = f'#!{sys.executable}\nprint("hello")'.encode() - t1 = TestBinaryTarget({}, Address("some/addr1")) - fs1 = TestRunSecondaryFieldSet.create(t1) - t2 = TestBinaryTarget({}, Address("some/addr2")) - fs2 = TestRunSecondaryFieldSet.create(t2) - with pytest.raises(TooManyTargetsException): - single_target_run( - rule_runner, - program_text=program_text, - targets_to_field_sets={t1: [fs1], t2: [fs2]}, - ) - - -def test_only_secondary_multi_field_set_error(rule_runner: RuleRunner) -> None: - program_text = f'#!{sys.executable}\nprint("hello")'.encode() - target = TestBinaryTarget({}, Address("some/addr")) - fs1 = TestRunSecondaryFieldSet.create(target) - fs2 = TestRunSecondaryFieldSet.create(target) - with pytest.raises(AmbiguousImplementationsException): - single_target_run( - rule_runner, program_text=program_text, targets_to_field_sets={target: [fs1, fs2]} - ) diff --git a/src/python/pants/engine/internals/graph.py b/src/python/pants/engine/internals/graph.py index 0e20a0bb946..d6cae86a86c 100644 --- a/src/python/pants/engine/internals/graph.py +++ b/src/python/pants/engine/internals/graph.py @@ -11,7 +11,7 @@ import os.path from dataclasses import dataclass from pathlib import PurePath -from typing import Any, Iterable, Iterator, NamedTuple, NewType, Sequence, Type, cast +from typing import Any, Iterable, Iterator, NamedTuple, Sequence, Type, cast from pants.base.deprecated import warn_or_error from pants.base.specs import AncestorGlobSpec, RawSpecsWithoutFileOwners, RecursiveGlobSpec @@ -68,7 +68,6 @@ MultipleSourcesField, OverridesField, RegisteredTargetTypes, - SecondaryOwnerMixin, SourcesField, SourcesPaths, SourcesPathsRequest, @@ -899,15 +898,7 @@ class OwnersRequest: match_if_owning_build_file_included_in_sources: bool = False -# NB: This was changed from: -# class Owners(Collection[Address]): -# pass -# In https://github.com/pantsbuild/pants/pull/19191 to facilitate surgical warning of deprecation -# of SecondaryOwnerMixin. After the Deprecation ends, it can be changed back. -IsPrimary = NewType("IsPrimary", bool) - - -class Owners(FrozenDict[Address, IsPrimary]): +class Owners(FrozenOrderedSet[Address]): pass @@ -962,7 +953,7 @@ def create_live_and_deleted_gets( ) live_candidate_tgts, deleted_candidate_tgts = await MultiGet(live_get, deleted_get) - result = {} + result = set() unmatched_sources = set(owners_request.sources) for live in (True, False): candidate_tgts: Sequence[Target] @@ -987,20 +978,6 @@ def create_live_and_deleted_gets( matching_files = set( candidate_tgt.get(SourcesField).filespec_matcher.matches(list(sources_set)) ) - is_primary = bool(matching_files) - - # Also consider secondary ownership, meaning it's not a `SourcesField` field with - # 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 # type: ignore[misc] - for field in candidate_tgt.field_values.values() - if isinstance(field, SecondaryOwnerMixin) - ) - for secondary_owner_field in secondary_owner_fields: - matching_files.update( - secondary_owner_field.filespec_matcher.matches(list(sources_set)) - ) if not matching_files and not ( owners_request.match_if_owning_build_file_included_in_sources @@ -1009,7 +986,7 @@ def create_live_and_deleted_gets( continue unmatched_sources -= matching_files - result[candidate_tgt.address] = IsPrimary(is_primary) + result.add(candidate_tgt.address) if ( unmatched_sources diff --git a/src/python/pants/engine/internals/graph_test.py b/src/python/pants/engine/internals/graph_test.py index e97cf577325..a78095bde01 100644 --- a/src/python/pants/engine/internals/graph_test.py +++ b/src/python/pants/engine/internals/graph_test.py @@ -9,7 +9,7 @@ from dataclasses import dataclass from pathlib import PurePath from textwrap import dedent -from typing import Iterable, List, Set, Tuple, Type, cast +from typing import Iterable, List, Set, Tuple, Type import pytest @@ -56,7 +56,6 @@ InvalidTargetException, MultipleSourcesField, OverridesField, - SecondaryOwnerMixin, ShouldTraverseDepsPredicate, SingleSourceField, SourcesPaths, @@ -71,7 +70,6 @@ TransitiveTargetsRequest, ) from pants.engine.unions import UnionMembership, UnionRule -from pants.source.filespec import Filespec from pants.testutil.rule_runner import QueryRule, RuleRunner, engine_error from pants.util.ordered_set import FrozenOrderedSet @@ -788,20 +786,6 @@ def get_target(name: str) -> Target: get_target("t1") -class MockSecondaryOwnerField(StringField, AsyncFieldMixin, SecondaryOwnerMixin): - alias = "secondary_owner_field" - required = True - - @property - def filespec(self) -> Filespec: - return {"includes": [os.path.join(self.address.spec_path, cast(str, self.value))]} - - -class MockSecondaryOwnerTarget(Target): - alias = "secondary_owner" - core_fields = (MockSecondaryOwnerField,) - - @pytest.fixture def owners_rule_runner() -> RuleRunner: return RuleRunner( @@ -812,7 +796,6 @@ def owners_rule_runner() -> RuleRunner: MockTarget, MockTargetGenerator, MockGeneratedTarget, - MockSecondaryOwnerTarget, ], # NB: The `graph` module masks the environment is most/all positions. We disable the # inherent environment so that the positions which do require the environment are @@ -853,7 +836,6 @@ def test_owners_source_file_does_not_exist(owners_rule_runner: RuleRunner) -> No """\ target(name='not-generator', sources=['*.txt']) generator(name='generator', sources=['*.txt']) - secondary_owner(name='secondary', secondary_owner_field='deleted.txt') """ ), } @@ -864,7 +846,6 @@ def test_owners_source_file_does_not_exist(owners_rule_runner: RuleRunner) -> No expected={ Address("demo", target_name="generator"), Address("demo", target_name="not-generator"), - Address("demo", target_name="secondary"), }, ) @@ -887,7 +868,6 @@ def test_owners_source_file_does_not_exist(owners_rule_runner: RuleRunner) -> No Address("demo", target_name="generator", relative_file_path="f.txt"), Address("demo", target_name="generator"), Address("demo", target_name="not-generator"), - Address("demo", target_name="secondary"), }, ) @@ -903,7 +883,6 @@ def test_owners_multiple_owners(owners_rule_runner: RuleRunner) -> None: target(name='not-generator-f2', sources=['f2.txt']) generator(name='generator-all', sources=['*.txt']) generator(name='generator-f2', sources=['f2.txt']) - secondary_owner(name='secondary', secondary_owner_field='f1.txt') """ ), } @@ -914,7 +893,6 @@ def test_owners_multiple_owners(owners_rule_runner: RuleRunner) -> None: expected={ Address("demo", target_name="generator-all", relative_file_path="f1.txt"), Address("demo", target_name="not-generator-all"), - Address("demo", target_name="secondary"), }, ) assert_owners( @@ -941,7 +919,6 @@ def test_owners_build_file(owners_rule_runner: RuleRunner) -> None: target(name='f2_first', sources=['f2.txt']) target(name='f2_second', sources=['f2.txt']) generator(name='generated', sources=['*.txt']) - secondary_owner(name='secondary', secondary_owner_field='f1.txt') """ ), } @@ -960,7 +937,6 @@ def test_owners_build_file(owners_rule_runner: RuleRunner) -> None: Address("demo", target_name="f1"), Address("demo", target_name="f2_first"), Address("demo", target_name="f2_second"), - Address("demo", target_name="secondary"), Address("demo", target_name="generated", relative_file_path="f1.txt"), Address("demo", target_name="generated", relative_file_path="f2.txt"), }, diff --git a/src/python/pants/engine/internals/specs_rules.py b/src/python/pants/engine/internals/specs_rules.py index 353ec3b0923..cebe2a3667b 100644 --- a/src/python/pants/engine/internals/specs_rules.py +++ b/src/python/pants/engine/internals/specs_rules.py @@ -3,7 +3,6 @@ from __future__ import annotations -import collections.abc import dataclasses import itertools import logging @@ -12,7 +11,6 @@ from typing import Iterable from pants.backend.project_info.filter_targets import FilterSubsystem -from pants.base.deprecated import CodeRemovedError from pants.base.specs import ( AddressLiteralSpec, AncestorGlobSpec, @@ -64,7 +62,7 @@ from pants.util.docutil import bin_name from pants.util.logging import LogLevel from pants.util.ordered_set import FrozenOrderedSet, OrderedSet -from pants.util.strutil import bullet_list, softwrap +from pants.util.strutil import bullet_list logger = logging.getLogger(__name__) @@ -408,10 +406,6 @@ def __init__( # # We sometimes suggest using `./pants filedeps` to find applicable files. However, this # command only works if at least one of the targets has a SourcesField field. - # - # NB: Even with the "secondary owners" mechanism - used by target types like `pex_binary` - # and `python_aws_lambda_function` to still work with file args - those targets will not show the - # associated files when using filedeps. filedeps_goal_works = any( tgt.class_has_field(SourcesField, union_membership) for tgt in applicable_target_types ) @@ -487,43 +481,6 @@ def __init__( ) -# NB: Remove when SecondaryOwnerMixin is removed -def _maybe_error_secondary_owner_semantics( - addresses_from_nonfile_specs: Addresses, - owners_from_filespecs: Owners, - matched_addresses: collections.abc.Set[Address], -): - """Warn about deprecated semantics of implicitly referring to a target through "Secondary - Ownership". - - E.g. If there's a `pex_binary` whose entry point is `foo.py`, using `foo.py` as a spec to mean - "package the pex binary" is deprecated, and the caller should specify the binary directly. - - This shouldn't warn if both the primary and secondary owner are in the specs (which is common - with specs like `::` or `dir:`). - """ - problematic_target_specs = { - address.spec - for address in matched_addresses - if address in owners_from_filespecs - and not owners_from_filespecs[address] - and address not in addresses_from_nonfile_specs - } - - if problematic_target_specs: - raise CodeRemovedError( - softwrap( - f""" - Support for indirectly referring to a target by using a corresponding file argument, - when the target owning the file isn't applicable was removed in version 2.18.0.dev3. - - Refer to the following targets by their addresses: - {bullet_list(sorted(problematic_target_specs))} - """ - ) - ) - - @rule async def find_valid_field_sets_for_target_roots( request: TargetRootsToFieldSetsRequest, @@ -572,27 +529,6 @@ async def find_valid_field_sets_for_target_roots( ): logger.warning(str(no_applicable_exception)) - # NB: Remove when SecondaryOwnerMixin is removed - if targets_to_applicable_field_sets and request.warn_on_deprecated_secondary_owner_semantics: - _maybe_error_secondary_owner_semantics( - # NB: All of these should be memoized, so it's not inappropriate to request simply for warning sake. - *( - await MultiGet( - Get( - Addresses, - RawSpecsWithoutFileOwners, - RawSpecsWithoutFileOwners.from_raw_specs(specs.includes), - ), - Get( - Owners, - RawSpecsWithOnlyFileOwners, - RawSpecsWithOnlyFileOwners.from_raw_specs(specs.includes), - ), - ) - ), - {tgt.address for tgt in targets_to_applicable_field_sets}, - ) - if request.num_shards > 0: sharded_targets_to_applicable_field_sets = { tgt: value diff --git a/src/python/pants/engine/internals/specs_rules_test.py b/src/python/pants/engine/internals/specs_rules_test.py index 542936c8687..e544c6acd9b 100644 --- a/src/python/pants/engine/internals/specs_rules_test.py +++ b/src/python/pants/engine/internals/specs_rules_test.py @@ -42,7 +42,6 @@ MultipleSourcesField, NoApplicableTargetsBehavior, OverridesField, - SecondaryOwnerMixin, SingleSourceField, StringField, Tags, @@ -53,7 +52,6 @@ TargetRootsToFieldSetsRequest, ) from pants.engine.unions import UnionMembership, UnionRule, union -from pants.source.filespec import Filespec from pants.testutil.rule_runner import RuleRunner, engine_error from pants.util.frozendict import FrozenDict from pants.util.strutil import softwrap @@ -1028,97 +1026,3 @@ class Tgt3(Target): assert "However, you only specified target and file arguments with these target types:" in str( exc ) - - -class FortranSecondaryOwnerField(StringField, SecondaryOwnerMixin): - alias = "borrows" - - @property - def filespec(self) -> Filespec: - assert self.value - return {"includes": [self.value]} - - -def test_secondary_owner_warning(caplog) -> None: - class FortranTarget(Target): - alias = "fortran_target" - core_fields = (FortranSources,) - - class SecondaryOwnerTarget(Target): - alias = "secondary_owner_target" - core_fields = (FortranSecondaryOwnerField,) - - @union - class SomeGoalFieldSet(FieldSet): - pass - - @dataclass(frozen=True) - class SecondaryOwnerFS(SomeGoalFieldSet): - required_fields = (FortranSecondaryOwnerField,) - - secondary_owner: FortranSecondaryOwnerField - - rule_runner = RuleRunner( - rules=[ - QueryRule(TargetRootsToFieldSets, [TargetRootsToFieldSetsRequest, Specs]), - UnionRule(SomeGoalFieldSet, SecondaryOwnerFS), - ], - target_types=[FortranTarget, SecondaryOwnerTarget], - ) - - rule_runner.write_files( - { - "BUILD": dedent( - """\ - fortran_target(name="primary", sources=["a.ft"]) - secondary_owner_target(name="secondary1", borrows="a.ft") - secondary_owner_target(name="secondary2", borrows="a.ft") - """ - ), - "a.ft": "", - } - ) - - request = TargetRootsToFieldSetsRequest( - SomeGoalFieldSet, - goal_description="some goal", - no_applicable_targets_behavior=NoApplicableTargetsBehavior.warn, - ) - - def run_rule(specs: Iterable[Spec]): - caplog.clear() - return rule_runner.request( - TargetRootsToFieldSets, - [ - request, - Specs( - includes=RawSpecs.create(specs, description_of_origin="tests"), - ignores=RawSpecs(description_of_origin="tests"), - ), - ], - ) - - result = run_rule([AddressLiteralSpec("", "primary"), AddressLiteralSpec("", "secondary1")]) - # No warning because the secondary target was referred to by address literally - assert len(caplog.records) == 0 - assert result.mapping - - result = run_rule([AddressLiteralSpec("", "primary")]) - assert len(caplog.records) == 1 - assert "Refer to the following targets" not in caplog.text - assert not result.mapping - - with pytest.raises(ExecutionError) as exc: - run_rule([FileLiteralSpec("a.ft")]) - assert len(caplog.records) == 0 - assert "Refer to the following targets by their addresses:\n * //:secondary1" in str(exc.value) - - with pytest.raises(ExecutionError) as exc: - run_rule([FileLiteralSpec("a.ft"), AddressLiteralSpec("", "secondary1")]) - assert len(caplog.records) == 0 - assert "secondary1" not in str(exc.value) - assert "secondary2" in str(exc.value) - - result = run_rule([RecursiveGlobSpec("")]) - assert len(caplog.records) == 0 - assert result.mapping diff --git a/src/python/pants/engine/target.py b/src/python/pants/engine/target.py index f36ac191074..16bf60a5ef7 100644 --- a/src/python/pants/engine/target.py +++ b/src/python/pants/engine/target.py @@ -1479,7 +1479,6 @@ class TargetRootsToFieldSetsRequest(Generic[_FS]): field_set_superclass: Type[_FS] goal_description: str no_applicable_targets_behavior: NoApplicableTargetsBehavior - warn_on_deprecated_secondary_owner_semantics: bool shard: int num_shards: int @@ -1489,18 +1488,12 @@ def __init__( *, goal_description: str, no_applicable_targets_behavior: NoApplicableTargetsBehavior, - warn_on_deprecated_secondary_owner_semantics: bool = True, shard: int = 0, num_shards: int = -1, ) -> None: object.__setattr__(self, "field_set_superclass", field_set_superclass) object.__setattr__(self, "goal_description", goal_description) object.__setattr__(self, "no_applicable_targets_behavior", no_applicable_targets_behavior) - object.__setattr__( - self, - "warn_on_deprecated_secondary_owner_semantics", - warn_on_deprecated_secondary_owner_semantics, - ) object.__setattr__(self, "shard", shard) object.__setattr__(self, "num_shards", num_shards) @@ -2440,44 +2433,6 @@ def debug_hint(self) -> str: return self.field.address.spec -class SecondaryOwnerMixin(ABC): - """Add to a Field for the target to work with file arguments and `--changed-since`, without it - needing a `SourcesField`. - - Why use this? In a dependency inference world, multiple targets including the same file in the - `sources` field causes issues due to ambiguity over which target to use. So, only one target - should have "primary ownership" of the file. However, you may still want other targets to be - used when that file is included in file arguments. For example, a `python_source` target - being the primary owner of the `.py` file, but a `pex_binary` still working with file - arguments for that file. Secondary ownership means that the target won't be used for things like - dependency inference and hydrating sources, but file arguments will still work. - - There should be a primary owner of the file(s), e.g. the `python_source` in the above example. - Typically, you will want to add a dependency inference rule to infer a dep on that primary - owner. - - All associated files must live in the BUILD target's directory or a subdirectory to work - properly, like the `sources` field. - """ - - @property - @abstractmethod - def filespec(self) -> Filespec: - """A dictionary in the form {'globs': ['full/path/to/f.ext']} representing the field's - associated files. - - Typically, users should use a file name/glob relative to the BUILD file, like the `sources` - field. Then, you can use `os.path.join(self.address.spec_path, self.value)` to relative to - the build root. - """ - - @memoized_property - def filespec_matcher(self) -> FilespecMatcher: - # Note: memoized because parsing the globs is expensive: - # https://github.com/pantsbuild/pants/issues/16122 - return FilespecMatcher(self.filespec["includes"], self.filespec.get("excludes", [])) - - def targets_with_sources_types( sources_types: Iterable[type[SourcesField]], targets: Iterable[Target],