Skip to content

Commit

Permalink
Fix some dependencies-like fields not showing up with project introsp…
Browse files Browse the repository at this point in the history
…ection (pantsbuild#10923)

### Problem

Closes pantsbuild#10888.

We now have several fields that act like dependencies, but are not quite it. In most code, we don't want the dependencies showing up with `TransitiveTargets` and `DependenciesRequest`; for example, with `relocated_files()`, it's important that the original files targets are not included when we're hydrating sources for the transitive closure.

But with project introspection, we do need these dependencies-like fields to be used. This is important for `--changed-since --changed-dependees` to work properly.

### Solution

Add a new `SpecialCasedDependencies` superclass. This is not a normal field, e.g. not setting `alias`. It's closer in spirit to a "Field template", e.g. `StringField` and `BoolField`.

Both `DependenciesRequest` and `TransitiveTargetsRequest` get new boolean flags to determine if they should include any subclasses of `SpecialCasedDependencies` or not. Then, we teach the project introspection goals to use the new behavior.

[ci skip-rust]
  • Loading branch information
Eric-Arellano authored Oct 9, 2020
1 parent 9100678 commit 7607e5f
Show file tree
Hide file tree
Showing 13 changed files with 257 additions and 114 deletions.
3 changes: 2 additions & 1 deletion src/python/pants/backend/project_info/dependees.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ async def map_addresses_to_dependees() -> AddressToDependees:
)
all_targets = {*all_expanded_targets, *all_explicit_targets}
dependencies_per_target = await MultiGet(
Get(Addresses, DependenciesRequest(tgt.get(Dependencies))) for tgt in all_targets
Get(Addresses, DependenciesRequest(tgt.get(Dependencies), include_special_cased_deps=True))
for tgt in all_targets
)

address_to_dependees = defaultdict(set)
Expand Down
15 changes: 13 additions & 2 deletions src/python/pants/backend/project_info/dependees_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,17 @@
from pants.backend.project_info.dependees import DependeesGoal
from pants.backend.project_info.dependees import DependeesOutputFormat as OutputFormat
from pants.backend.project_info.dependees import rules as dependee_rules
from pants.engine.target import Dependencies, Target
from pants.engine.target import Dependencies, SpecialCasedDependencies, Target
from pants.testutil.test_base import TestBase


class SpecialDeps(SpecialCasedDependencies):
alias = "special_deps"


class MockTarget(Target):
alias = "tgt"
core_fields = (Dependencies,)
core_fields = (Dependencies, SpecialDeps)


class DependeesTest(TestBase):
Expand Down Expand Up @@ -139,3 +143,10 @@ def test_multiple_specified_targets(self) -> None:
}"""
).splitlines(),
)

def test_special_cased_dependencies(self) -> None:
self.add_to_build_file("special", "tgt(special_deps=['intermediate'])")
self.assert_dependees(targets=["intermediate"], expected=["leaf", "special"])
self.assert_dependees(
targets=["base"], transitive=True, expected=["intermediate", "leaf", "special"]
)
10 changes: 8 additions & 2 deletions src/python/pants/backend/project_info/dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,18 @@ async def dependencies(
console: Console, addresses: Addresses, dependencies_subsystem: DependenciesSubsystem
) -> Dependencies:
if dependencies_subsystem.transitive:
transitive_targets = await Get(TransitiveTargets, TransitiveTargetsRequest(addresses))
transitive_targets = await Get(
TransitiveTargets, TransitiveTargetsRequest(addresses, include_special_cased_deps=True)
)
targets = Targets(transitive_targets.dependencies)
else:
target_roots = await Get(UnexpandedTargets, Addresses, addresses)
dependencies_per_target_root = await MultiGet(
Get(Targets, DependenciesRequest(tgt.get(DependenciesField))) for tgt in target_roots
Get(
Targets,
DependenciesRequest(tgt.get(DependenciesField), include_special_cased_deps=True),
)
for tgt in target_roots
)
targets = Targets(itertools.chain.from_iterable(dependencies_per_target_root))

Expand Down
31 changes: 30 additions & 1 deletion src/python/pants/backend/project_info/dependencies_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,26 @@

from pants.backend.project_info.dependencies import Dependencies, DependencyType, rules
from pants.backend.python.target_types import PythonLibrary, PythonRequirementLibrary
from pants.engine.target import SpecialCasedDependencies, Target
from pants.testutil.rule_runner import RuleRunner


# We verify that any subclasses of `SpecialCasedDependencies` will show up with the `dependencies`
# goal by creating a mock target.
class SpecialDepsField(SpecialCasedDependencies):
alias = "special_deps"


class SpecialDepsTarget(Target):
alias = "special_deps_tgt"
core_fields = (SpecialDepsField,)


@pytest.fixture
def rule_runner() -> RuleRunner:
return RuleRunner(rules=rules(), target_types=[PythonLibrary, PythonRequirementLibrary])
return RuleRunner(
rules=rules(), target_types=[PythonLibrary, PythonRequirementLibrary, SpecialDepsTarget]
)


def create_python_library(
Expand Down Expand Up @@ -65,6 +79,21 @@ def test_no_dependencies(rule_runner: RuleRunner) -> None:
assert_dependencies(rule_runner, specs=["some/target"], expected=[], transitive=True)


def test_special_cased_dependencies(rule_runner: RuleRunner) -> None:
rule_runner.add_to_build_file(
"",
dedent(
"""\
special_deps_tgt(name='t1')
special_deps_tgt(name='t2', special_deps=[':t1'])
special_deps_tgt(name='t3', special_deps=[':t2'])
"""
),
)
assert_dependencies(rule_runner, specs=["//:t3"], expected=["//:t2"])
assert_dependencies(rule_runner, specs=["//:t3"], expected=["//:t1", "//:t2"], transitive=True)


def test_python_dependencies(rule_runner: RuleRunner) -> None:
create_python_requirement_library(rule_runner, name="req1")
create_python_requirement_library(rule_runner, name="req2")
Expand Down
4 changes: 3 additions & 1 deletion src/python/pants/backend/project_info/filedeps.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,9 @@ async def file_deps(
) -> Filedeps:
targets: Iterable[Target]
if filedeps_subsystem.transitive:
transitive_targets = await Get(TransitiveTargets, TransitiveTargetsRequest(addresses))
transitive_targets = await Get(
TransitiveTargets, TransitiveTargetsRequest(addresses, include_special_cased_deps=True)
)
targets = transitive_targets.closure
elif filedeps_subsystem.globs:
targets = await Get(UnexpandedTargets, Addresses, addresses)
Expand Down
26 changes: 14 additions & 12 deletions src/python/pants/backend/python/goals/pytest_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -157,21 +157,23 @@ async def setup_pytest_for_target(

# Create any assets that the test depends on through the `runtime_package_dependencies` field.
assets: Tuple[BuiltPackage, ...] = ()
if (
request.field_set.runtime_package_dependencies.value
or request.field_set.runtime_binary_dependencies.value
):
unparsed_addresses = (
*(request.field_set.runtime_package_dependencies.value or ()),
*(request.field_set.runtime_binary_dependencies.value or ()),
)
runtime_package_targets = await Get(
Targets,
UnparsedAddressInputs(unparsed_addresses, owning_address=request.field_set.address),
unparsed_runtime_packages = (
request.field_set.runtime_package_dependencies.to_unparsed_address_inputs()
)
unparsed_runtime_binaries = (
request.field_set.runtime_binary_dependencies.to_unparsed_address_inputs()
)
if unparsed_runtime_packages.values or unparsed_runtime_binaries.values:
runtime_package_targets, runtime_binary_dependencies = await MultiGet(
Get(Targets, UnparsedAddressInputs, unparsed_runtime_packages),
Get(Targets, UnparsedAddressInputs, unparsed_runtime_binaries),
)
field_sets_per_target = await Get(
FieldSetsPerTarget,
FieldSetsPerTargetRequest(PackageFieldSet, runtime_package_targets),
FieldSetsPerTargetRequest(
PackageFieldSet,
itertools.chain(runtime_package_targets, runtime_binary_dependencies),
),
)
assets = await MultiGet(
Get(BuiltPackage, PackageFieldSet, field_set)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -446,11 +446,17 @@ def test_runtime_package_dependency(rule_runner: RuleRunner) -> None:
rule_runner.create_file(
f"{PACKAGE}/test_binary_call.py",
dedent(
"""\
f"""\
import os.path
import subprocess
def test_embedded_binary():
assert b"Hello, test!" in subprocess.check_output(args=['./bin.pex'])
# Ensure that we didn't accidentally pull in the binary's sources. This is a
# special type of dependency that should not be included with the rest of the
# normal dependencies.
assert os.path.exists("{BINARY_SOURCE.path}") is False
"""
),
)
Expand Down
12 changes: 5 additions & 7 deletions src/python/pants/backend/python/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@
ProvidesField,
ScalarField,
Sources,
SpecialCasedDependencies,
StringField,
StringOrStringSequenceField,
StringSequenceField,
Target,
WrappedTarget,
)
Expand Down Expand Up @@ -269,9 +269,7 @@ class PythonTestsDependencies(Dependencies):
supports_transitive_excludes = True


# TODO(#10888): Teach project introspection goals that this is a special type of the `Dependencies`
# field.
class PythonRuntimePackageDependencies(StringSequenceField):
class PythonRuntimePackageDependencies(SpecialCasedDependencies):
"""Addresses to targets that can be built with the `./pants package` goal and whose resulting
assets should be included in the test run.
Expand All @@ -286,14 +284,14 @@ class PythonRuntimePackageDependencies(StringSequenceField):
alias = "runtime_package_dependencies"


class PythonRuntimeBinaryDependencies(StringSequenceField):
class PythonRuntimeBinaryDependencies(SpecialCasedDependencies):
"""Deprecated in favor of the `runtime_build_dependencies` field, which works with more target
types like `archive` and `python_awslambda`."""

alias = "runtime_binary_dependencies"

@classmethod
def compute_value(
def sanitize_raw_value(
cls, raw_value: Optional[Iterable[str]], *, address: Address
) -> Optional[Tuple[str, ...]]:
if raw_value is not None:
Expand All @@ -302,7 +300,7 @@ def compute_value(
"field is now deprecated in favor of the more flexible "
"`runtime_package_dependencies` field, and it will be removed in 2.1.0.dev0."
)
return super().compute_value(raw_value, address=address)
return super().sanitize_raw_value(raw_value, address=address)


class PythonTestsTimeout(IntField):
Expand Down
33 changes: 12 additions & 21 deletions src/python/pants/core/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from dataclasses import dataclass
from typing import Tuple

from pants.core.goals.package import (
BuiltPackage,
Expand All @@ -24,8 +23,8 @@
HydratedSources,
HydrateSourcesRequest,
Sources,
SpecialCasedDependencies,
StringField,
StringSequenceField,
Target,
Targets,
WrappedTarget,
Expand Down Expand Up @@ -67,9 +66,7 @@ class RelocatedFilesSources(Sources):
expected_num_files = 0


# TODO(#10888): Teach project introspection goals that this is a special type of the `Dependencies`
# field.
class RelocatedFilesOriginalTargets(StringSequenceField):
class RelocatedFilesOriginalTargets(SpecialCasedDependencies):
"""Addresses to the original `files()` targets that you want to relocate, such as
`['//:json_files']`.
Expand All @@ -79,7 +76,6 @@ class RelocatedFilesOriginalTargets(StringSequenceField):

alias = "files_targets"
required = True
value: Tuple[str, ...]


class RelocatedFilesSrcField(StringField):
Expand Down Expand Up @@ -166,7 +162,11 @@ async def relocate_files(request: RelocateFilesViaCodegenRequest) -> GeneratedSo
AddressInput,
AddressInput.parse(v, relative_to=request.protocol_target.address.spec_path),
)
for v in request.protocol_target.get(RelocatedFilesOriginalTargets).value
for v in (
request.protocol_target.get(RelocatedFilesOriginalTargets)
.to_unparsed_address_inputs()
.values
)
)
original_files_sources = await MultiGet(
Get(HydratedSources, HydrateSourcesRequest(wrapped_tgt.target.get(Sources)))
Expand Down Expand Up @@ -227,9 +227,8 @@ class GenericTarget(Target):
# `archive` target
# -----------------------------------------------------------------------------------------------

# TODO(#10888): Teach project introspection goals that this is a special type of the `Dependencies`
# field.
class ArchivePackages(StringSequenceField):

class ArchivePackages(SpecialCasedDependencies):
"""Addresses to any targets that can be built with `./pants package`.
Pants will build the assets as if you had run `./pants package`. It will include the
Expand All @@ -243,9 +242,7 @@ class ArchivePackages(StringSequenceField):
alias = "packages"


# TODO(#10888): Teach project introspection goals that this is a special type of the `Dependencies`
# field.
class ArchiveFiles(StringSequenceField):
class ArchiveFiles(SpecialCasedDependencies):
"""Addresses to any `files` or `relocated_files` targets to include in the archive, e.g.
`["resources:logo"]`.
Expand Down Expand Up @@ -298,14 +295,8 @@ async def package_archive_target(
field_set: ArchiveFieldSet, global_options: GlobalOptions
) -> BuiltPackage:
package_targets, files_targets = await MultiGet(
Get(
Targets,
UnparsedAddressInputs(field_set.packages.value or (), owning_address=field_set.address),
),
Get(
Targets,
UnparsedAddressInputs(field_set.files.value or (), owning_address=field_set.address),
),
Get(Targets, UnparsedAddressInputs, field_set.packages.to_unparsed_address_inputs()),
Get(Targets, UnparsedAddressInputs, field_set.files.to_unparsed_address_inputs()),
)

package_field_sets_per_target = await Get(
Expand Down
40 changes: 39 additions & 1 deletion src/python/pants/engine/internals/graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
InjectedDependencies,
RegisteredTargetTypes,
Sources,
SpecialCasedDependencies,
Subtargets,
Target,
TargetRootsToFieldSets,
Expand Down Expand Up @@ -309,7 +310,14 @@ async def transitive_targets(request: TransitiveTargetsRequest) -> TransitiveTar
dependency_mapping: Dict[Address, Tuple[Address, ...]] = {}
while queued:
direct_dependencies = await MultiGet(
Get(Targets, DependenciesRequest(tgt.get(Dependencies))) for tgt in queued
Get(
Targets,
DependenciesRequest(
tgt.get(Dependencies),
include_special_cased_deps=request.include_special_cased_deps,
),
)
for tgt in queued
)

dependency_mapping.update(
Expand Down Expand Up @@ -865,13 +873,43 @@ async def resolve_dependencies(
t.address for t in subtargets.subtargets if t.address != request.field.address
)

# If the target has `SpecialCasedDependencies`, such as the `archive` target having
# `files` and `packages` fields, then we possibly include those too. We don't want to always
# include those dependencies because they should often be excluded from the result due to
# being handled elsewhere in the calling code.
special_cased: Tuple[Address, ...] = ()
if request.include_special_cased_deps:
wrapped_tgt = await Get(WrappedTarget, Address, request.field.address)
# Unlike normal, we don't use `tgt.get()` because there may be >1 subclass of
# SpecialCasedDependencies.
special_cased_fields = tuple(
field
for field in wrapped_tgt.target.field_values.values()
if isinstance(field, SpecialCasedDependencies)
)
# We can't use the normal `Get(Addresses, UnparsedAddressInputs)` due to a graph cycle.
special_cased = await MultiGet(
Get(
Address,
AddressInput,
AddressInput.parse(
addr,
relative_to=request.field.address.spec_path,
subproject_roots=global_options.options.subproject_roots,
),
)
for special_cased_field in special_cased_fields
for addr in special_cased_field.to_unparsed_address_inputs().values
)

result = {
addr
for addr in (
*subtarget_addresses,
*literal_addresses,
*itertools.chain.from_iterable(injected),
*itertools.chain.from_iterable(inferred),
*special_cased,
)
if addr not in ignored_addresses
}
Expand Down
Loading

0 comments on commit 7607e5f

Please sign in to comment.