Skip to content

Commit

Permalink
[internal] Split shell targets into atom vs generator (#12957)
Browse files Browse the repository at this point in the history
Demos the technique described in #12954. The end user sees no change, including `./pants filter --target-type=python_library` and `./pants peek` behaving the same way. 

But our internal representation now uses the changes from [the proposal](https://docs.google.com/document/d/1HpJn2jTWf5sKob6zhe4SqHZ7KWBlX4a6OJOwnuw-IHo/edit). The major benefit is that now our rules don't need to worry about `Address.is_file_target` - instead, the Target API ensures that no matter what, the rules will only ever operate on files as atoms. Even with fixes like #12952, we'll do the right thing.

This change also frees us up to add an `overrides` field to the target generator safely.

[ci skip-rust]
  • Loading branch information
Eric-Arellano authored Sep 26, 2021
1 parent 700198e commit 906099f
Show file tree
Hide file tree
Showing 16 changed files with 176 additions and 111 deletions.
10 changes: 5 additions & 5 deletions src/python/pants/backend/shell/dependency_inference.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

from pants.backend.shell.lint.shellcheck.subsystem import Shellcheck
from pants.backend.shell.shell_setup import ShellSetup
from pants.backend.shell.target_types import ShellSources
from pants.backend.shell.target_types import ShellSourcesField
from pants.base.specs import AddressSpecs, DescendantAddresses
from pants.core.util_rules.external_tool import DownloadedExternalTool, ExternalToolRequest
from pants.engine.addresses import Address
Expand Down Expand Up @@ -52,10 +52,10 @@ class ShellMapping:

@rule(desc="Creating map of Shell file names to Shell targets", level=LogLevel.DEBUG)
async def map_shell_files() -> ShellMapping:
all_expanded_targets = await Get(Targets, AddressSpecs([DescendantAddresses("")]))
shell_tgts = tuple(tgt for tgt in all_expanded_targets if tgt.has_field(ShellSources))
all_targets = await Get(Targets, AddressSpecs([DescendantAddresses("")]))
shell_tgts = tuple(tgt for tgt in all_targets if tgt.has_field(ShellSourcesField))
sources_per_target = await MultiGet(
Get(SourcesPaths, SourcesPathsRequest(tgt[ShellSources])) for tgt in shell_tgts
Get(SourcesPaths, SourcesPathsRequest(tgt[ShellSourcesField])) for tgt in shell_tgts
)

files_to_addresses: dict[str, Address] = {}
Expand Down Expand Up @@ -153,7 +153,7 @@ async def parse_shell_imports(


class InferShellDependencies(InferDependenciesRequest):
infer_from = ShellSources
infer_from = ShellSourcesField


@rule(desc="Inferring Shell dependencies by analyzing imports")
Expand Down
20 changes: 8 additions & 12 deletions src/python/pants/backend/shell/dependency_inference_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
ParseShellImportsRequest,
ShellMapping,
)
from pants.backend.shell.target_types import ShellLibrary, ShellSources
from pants.backend.shell.target_types import ShellSourcesField, ShellSourcesGeneratorTarget
from pants.backend.shell.target_types import rules as target_types_rules
from pants.core.util_rules import external_tool
from pants.engine.addresses import Address
Expand All @@ -34,7 +34,7 @@ def rule_runner() -> RuleRunner:
QueryRule(ParsedShellImports, [ParseShellImportsRequest]),
QueryRule(InferredDependencies, [InferShellDependencies]),
],
target_types=[ShellLibrary],
target_types=[ShellSourcesGeneratorTarget],
)


Expand Down Expand Up @@ -131,24 +131,20 @@ def test_dependency_inference(rule_runner: RuleRunner, caplog) -> None:
def run_dep_inference(address: Address) -> InferredDependencies:
tgt = rule_runner.get_target(address)
return rule_runner.request(
InferredDependencies, [InferShellDependencies(tgt[ShellSources])]
InferredDependencies, [InferShellDependencies(tgt[ShellSourcesField])]
)

build_address = Address("a")
assert run_dep_inference(build_address) == InferredDependencies(
[Address("b", relative_file_path="f.sh"), Address("a", relative_file_path="f1.sh")]
)

file_address = Address("a", relative_file_path="f1.sh")
assert run_dep_inference(file_address) == InferredDependencies(
assert run_dep_inference(Address("a", relative_file_path="f1.sh")) == InferredDependencies(
[Address("b", relative_file_path="f.sh")]
)

caplog.clear()
assert run_dep_inference(Address("ambiguous", target_name="main")) == InferredDependencies(
assert run_dep_inference(
Address("ambiguous", target_name="main", relative_file_path="main.sh")
) == InferredDependencies(
[Address("ambiguous", target_name="dep1", relative_file_path="disambiguated.sh")]
)
assert len(caplog.records) == 1
assert "The target ambiguous:main sources `ambiguous/dep.sh`" in caplog.text
assert "The target ambiguous/main.sh:main sources `ambiguous/dep.sh`" in caplog.text
assert "['ambiguous/dep.sh:dep1', 'ambiguous/dep.sh:dep2']" in caplog.text
assert "disambiguated.sh" not in caplog.text
6 changes: 3 additions & 3 deletions src/python/pants/backend/shell/lint/shell_fmt.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from dataclasses import dataclass
from typing import Iterable

from pants.backend.shell.target_types import ShellSources
from pants.backend.shell.target_types import ShellSourcesField
from pants.core.goals.fmt import FmtResult, LanguageFmtResults, LanguageFmtTargets
from pants.core.goals.style_request import StyleRequest
from pants.core.util_rules.source_files import SourceFiles, SourceFilesRequest
Expand All @@ -17,7 +17,7 @@

@dataclass(frozen=True)
class ShellFmtTargets(LanguageFmtTargets):
required_fields = (ShellSources,)
required_fields = (ShellSourcesField,)


@union
Expand All @@ -31,7 +31,7 @@ async def format_shell_targets(
) -> LanguageFmtResults:
original_sources = await Get(
SourceFiles,
SourceFilesRequest(target[ShellSources] for target in shell_fmt_targets.targets),
SourceFilesRequest(target[ShellSourcesField] for target in shell_fmt_targets.targets),
)
prior_formatter_result = original_sources.snapshot

Expand Down
10 changes: 5 additions & 5 deletions src/python/pants/backend/shell/lint/shellcheck/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

from pants.backend.shell.lint.shellcheck.skip_field import SkipShellcheckField
from pants.backend.shell.lint.shellcheck.subsystem import Shellcheck
from pants.backend.shell.target_types import ShellSources
from pants.backend.shell.target_types import ShellSourcesField
from pants.core.goals.lint import LintRequest, LintResult, LintResults
from pants.core.util_rules.config_files import ConfigFiles, ConfigFilesRequest
from pants.core.util_rules.external_tool import DownloadedExternalTool, ExternalToolRequest
Expand All @@ -29,9 +29,9 @@

@dataclass(frozen=True)
class ShellcheckFieldSet(FieldSet):
required_fields = (ShellSources,)
required_fields = (ShellSourcesField,)

sources: ShellSources
sources: ShellSourcesField
dependencies: Dependencies

@classmethod
Expand All @@ -58,15 +58,15 @@ async def run_shellcheck(request: ShellcheckRequest, shellcheck: Shellcheck) ->
SourceFiles,
SourceFilesRequest(
(field_set.sources for field_set in request.field_sets),
for_sources_types=(ShellSources,),
for_sources_types=(ShellSourcesField,),
enable_codegen=True,
),
)
dependency_sources_get = Get(
SourceFiles,
SourceFilesRequest(
(tgt.get(Sources) for dependencies in all_dependencies for tgt in dependencies),
for_sources_types=(ShellSources,),
for_sources_types=(ShellSourcesField,),
enable_codegen=True,
),
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

from pants.backend.shell.lint.shellcheck.rules import ShellcheckFieldSet, ShellcheckRequest
from pants.backend.shell.lint.shellcheck.rules import rules as shellcheck_rules
from pants.backend.shell.target_types import ShellLibrary
from pants.backend.shell.target_types import ShellSourcesGeneratorTarget
from pants.backend.shell.target_types import rules as target_types_rules
from pants.core.goals.lint import LintResult, LintResults
from pants.core.util_rules import config_files, external_tool, source_files
Expand All @@ -29,7 +29,7 @@ def rule_runner() -> RuleRunner:
*target_types_rules(),
QueryRule(LintResults, [ShellcheckRequest]),
],
target_types=[ShellLibrary],
target_types=[ShellSourcesGeneratorTarget],
)


Expand Down
13 changes: 10 additions & 3 deletions src/python/pants/backend/shell/lint/shellcheck/skip_field.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from pants.backend.shell.target_types import ShellLibrary, Shunit2Tests
from pants.backend.shell.target_types import (
ShellSourcesGeneratorTarget,
ShellSourceTarget,
Shunit2TestsGeneratorTarget,
Shunit2TestTarget,
)
from pants.engine.target import BoolField


Expand All @@ -13,6 +18,8 @@ class SkipShellcheckField(BoolField):

def rules():
return [
ShellLibrary.register_plugin_field(SkipShellcheckField),
Shunit2Tests.register_plugin_field(SkipShellcheckField),
ShellSourceTarget.register_plugin_field(SkipShellcheckField),
ShellSourcesGeneratorTarget.register_plugin_field(SkipShellcheckField),
Shunit2TestTarget.register_plugin_field(SkipShellcheckField),
Shunit2TestsGeneratorTarget.register_plugin_field(SkipShellcheckField),
]
6 changes: 3 additions & 3 deletions src/python/pants/backend/shell/lint/shfmt/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from pants.backend.shell.lint.shell_fmt import ShellFmtRequest
from pants.backend.shell.lint.shfmt.skip_field import SkipShfmtField
from pants.backend.shell.lint.shfmt.subsystem import Shfmt
from pants.backend.shell.target_types import ShellSources
from pants.backend.shell.target_types import ShellSourcesField
from pants.core.goals.fmt import FmtResult
from pants.core.goals.lint import LintRequest, LintResult, LintResults
from pants.core.util_rules.config_files import ConfigFiles, ConfigFilesRequest
Expand All @@ -24,9 +24,9 @@

@dataclass(frozen=True)
class ShfmtFieldSet(FieldSet):
required_fields = (ShellSources,)
required_fields = (ShellSourcesField,)

sources: ShellSources
sources: ShellSourcesField

@classmethod
def opt_out(cls, tgt: Target) -> bool:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

from pants.backend.shell.lint.shfmt.rules import ShfmtFieldSet, ShfmtRequest
from pants.backend.shell.lint.shfmt.rules import rules as shfmt_rules
from pants.backend.shell.target_types import ShellLibrary
from pants.backend.shell.target_types import ShellSourcesGeneratorTarget
from pants.backend.shell.target_types import rules as target_types_rules
from pants.core.goals.fmt import FmtResult
from pants.core.goals.lint import LintResult, LintResults
Expand All @@ -34,7 +34,7 @@ def rule_runner() -> RuleRunner:
QueryRule(FmtResult, [ShfmtRequest]),
QueryRule(SourceFiles, [SourceFilesRequest]),
],
target_types=[ShellLibrary],
target_types=[ShellSourcesGeneratorTarget],
)


Expand Down
13 changes: 10 additions & 3 deletions src/python/pants/backend/shell/lint/shfmt/skip_field.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from pants.backend.shell.target_types import ShellLibrary, Shunit2Tests
from pants.backend.shell.target_types import (
ShellSourcesGeneratorTarget,
ShellSourceTarget,
Shunit2TestsGeneratorTarget,
Shunit2TestTarget,
)
from pants.engine.target import BoolField


Expand All @@ -13,6 +18,8 @@ class SkipShfmtField(BoolField):

def rules():
return [
ShellLibrary.register_plugin_field(SkipShfmtField),
Shunit2Tests.register_plugin_field(SkipShfmtField),
ShellSourceTarget.register_plugin_field(SkipShfmtField),
ShellSourcesGeneratorTarget.register_plugin_field(SkipShfmtField),
Shunit2TestTarget.register_plugin_field(SkipShfmtField),
Shunit2TestsGeneratorTarget.register_plugin_field(SkipShfmtField),
]
8 changes: 6 additions & 2 deletions src/python/pants/backend/shell/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,16 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from pants.backend.shell import dependency_inference, shell_command, shunit2_test_runner, tailor
from pants.backend.shell.target_types import ShellCommand, ShellLibrary, Shunit2Tests
from pants.backend.shell.target_types import (
ShellCommand,
ShellSourcesGeneratorTarget,
Shunit2TestsGeneratorTarget,
)
from pants.backend.shell.target_types import rules as target_types_rules


def target_types():
return [ShellCommand, ShellLibrary, Shunit2Tests]
return [ShellCommand, ShellSourcesGeneratorTarget, Shunit2TestsGeneratorTarget]


def rules():
Expand Down
4 changes: 2 additions & 2 deletions src/python/pants/backend/shell/shell_command_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

from pants.backend.shell.shell_command import GenerateFilesFromShellCommandRequest
from pants.backend.shell.shell_command import rules as shell_command_rules
from pants.backend.shell.target_types import ShellCommand, ShellLibrary
from pants.backend.shell.target_types import ShellCommand, ShellSourcesGeneratorTarget
from pants.core.target_types import Files
from pants.core.target_types import rules as target_type_rules
from pants.core.util_rules.archive import rules as archive_rules
Expand All @@ -34,7 +34,7 @@ def rule_runner() -> RuleRunner:
QueryRule(TransitiveTargets, [TransitiveTargetsRequest]),
QueryRule(SourceFiles, [SourceFilesRequest]),
],
target_types=[ShellCommand, ShellLibrary, Files],
target_types=[ShellCommand, ShellSourcesGeneratorTarget, Files],
)
rule_runner.set_options([], env_inherit={"PATH"})
return rule_runner
Expand Down
34 changes: 13 additions & 21 deletions src/python/pants/backend/shell/shunit2_test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@

from pants.backend.shell.shell_setup import ShellSetup
from pants.backend.shell.target_types import (
ShellSources,
ShellSourcesField,
Shunit2Shell,
Shunit2ShellField,
Shunit2Tests,
Shunit2TestsSources,
Shunit2TestsTimeout,
Shunit2TestsGeneratorTarget,
Shunit2TestSourcesField,
Shunit2TestTimeoutField,
)
from pants.core.goals.test import (
BuildPackageDependenciesRequest,
Expand Down Expand Up @@ -56,10 +56,10 @@

@dataclass(frozen=True)
class Shunit2FieldSet(TestFieldSet):
required_fields = (Shunit2TestsSources,)
required_fields = (Shunit2TestSourcesField,)

sources: Shunit2TestsSources
timeout: Shunit2TestsTimeout
sources: Shunit2TestSourcesField
timeout: Shunit2TestTimeoutField
shell: Shunit2ShellField
runtime_package_dependencies: RuntimePackageDependenciesField

Expand Down Expand Up @@ -127,7 +127,7 @@ async def determine_shunit2_shell(
f"Please either specify the `{Shunit2ShellField.alias}` field or add a "
f"shebang to {request.test_file_content.path} with one of the supported shells in "
f"the format `!#/path/to/shell` or `!#/path/to/env shell`"
f"(run `./pants help {Shunit2Tests.alias}` for valid shells)."
f"(run `./pants help {Shunit2TestsGeneratorTarget.alias}` for valid shells)."
)
tgt_shell = parse_result

Expand Down Expand Up @@ -172,7 +172,7 @@ async def setup_shunit2_for_target(
SourceFiles,
SourceFilesRequest(
(tgt.get(Sources) for tgt in transitive_targets.dependencies),
for_sources_types=(ShellSources, FilesSources, ResourcesSources),
for_sources_types=(ShellSourcesField, FilesSources, ResourcesSources),
enable_codegen=True,
),
)
Expand All @@ -182,24 +182,16 @@ async def setup_shunit2_for_target(
)

field_set_digest_content = await Get(DigestContents, Digest, field_set_sources.snapshot.digest)
# Because a FieldSet corresponds to a file address, there should be exactly 1 file in the
# sources. This assumption allows us to simplify determining which shell to use via inspecting
# the shebang.
if len(field_set_digest_content) != 1:
raise AssertionError(
f"The file address {request.field_set.address} had sources != 1, which is unexpected: "
f"{field_set_sources.snapshot.files}. Please file a bug at "
"https://github.com/pantsbuild/pants/issues/new with this error message copied."
)
original_test_file_content = field_set_digest_content[0]
updated_test_file_content = add_source_shunit2(original_test_file_content)
# `ShellTestSourceField` validates that there's exactly one file.
test_file_content = field_set_digest_content[0]
updated_test_file_content = add_source_shunit2(test_file_content)

updated_test_digest, runner = await MultiGet(
Get(Digest, CreateDigest([updated_test_file_content])),
Get(
Shunit2Runner,
Shunit2RunnerRequest(
request.field_set.address, original_test_file_content, request.field_set.shell
request.field_set.address, test_file_content, request.field_set.shell
),
),
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@
Shunit2RunnerRequest,
)
from pants.backend.shell.target_types import (
ShellLibrary,
ShellSourcesGeneratorTarget,
Shunit2Shell,
Shunit2ShellField,
Shunit2Tests,
Shunit2TestsGeneratorTarget,
)
from pants.backend.shell.target_types import rules as target_types_rules
from pants.core.goals.test import (
Expand Down Expand Up @@ -56,7 +56,12 @@ def rule_runner() -> RuleRunner:
QueryRule(TestDebugRequest, [Shunit2FieldSet]),
QueryRule(Shunit2Runner, [Shunit2RunnerRequest]),
],
target_types=[ShellLibrary, Shunit2Tests, PythonLibrary, PexBinary],
target_types=[
ShellSourcesGeneratorTarget,
Shunit2TestsGeneratorTarget,
PythonLibrary,
PexBinary,
],
)


Expand Down
Loading

0 comments on commit 906099f

Please sign in to comment.