Skip to content

Commit

Permalink
Remove last bits of SecondaryOwnershipMixin (pantsbuild#19742)
Browse files Browse the repository at this point in the history
This change removes much
It's something I have worked towards
And now it is done
  • Loading branch information
thejcannon authored Aug 31, 2023
1 parent d3ba1cb commit ed217e2
Show file tree
Hide file tree
Showing 12 changed files with 19 additions and 458 deletions.
13 changes: 1 addition & 12 deletions src/python/pants/backend/python/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
OptionalSingleSourceField,
OverridesField,
ScalarField,
SecondaryOwnerMixin,
SingleSourceField,
SpecialCasedDependencies,
StringField,
Expand All @@ -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
Expand Down Expand Up @@ -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(
Expand All @@ -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.
"""
Expand All @@ -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.
Expand Down
14 changes: 0 additions & 14 deletions src/python/pants/backend/python/target_types_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down
14 changes: 1 addition & 13 deletions src/python/pants/backend/python/util_rules/faas.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,21 +69,19 @@
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

logger = logging.getLogger(__name__)


class PythonFaaSHandlerField(StringField, AsyncFieldMixin, SecondaryOwnerMixin):
class PythonFaaSHandlerField(StringField, AsyncFieldMixin):
alias = "handler"
required = True
value: str
Expand All @@ -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.
"""
)

Expand All @@ -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:
Expand Down
11 changes: 1 addition & 10 deletions src/python/pants/backend/python/util_rules/faas_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from __future__ import annotations

from textwrap import dedent
from typing import List, Optional
from typing import Optional

import pytest

Expand Down Expand Up @@ -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
Expand Down
4 changes: 0 additions & 4 deletions src/python/pants/base/specs_integration_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
]
68 changes: 8 additions & 60 deletions src/python/pants/core/goals/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -39,7 +27,6 @@
BoolField,
FieldSet,
NoApplicableTargetsBehavior,
SecondaryOwnerMixin,
Target,
TargetRootsToFieldSets,
TargetRootsToFieldSetsRequest,
Expand Down Expand Up @@ -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]:
Expand All @@ -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
Expand Down
89 changes: 2 additions & 87 deletions src/python/pants/core/goals/run_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

import os
import sys
from dataclasses import dataclass
from typing import Iterable, Mapping, cast

import pytest
Expand All @@ -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,
Expand Down Expand Up @@ -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 = ()
Expand Down Expand Up @@ -144,8 +132,8 @@ def single_target_run(
],
union_membership=UnionMembership(
{
RunFieldSet: [TestRunFieldSet, TestRunSecondaryFieldSet],
RunDebugAdapterRequest: [TestRunFieldSet, TestRunSecondaryFieldSet],
RunFieldSet: [TestRunFieldSet],
RunDebugAdapterRequest: [TestRunFieldSet],
},
),
)
Expand Down Expand Up @@ -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]}
)
Loading

0 comments on commit ed217e2

Please sign in to comment.