Skip to content

Commit

Permalink
Fix source root stripping to not strip requirements.txt files in pl…
Browse files Browse the repository at this point in the history
…ugins (#11712)

The `PythonRequirementsFileSources` is like a `files()` target in that its source roots should not be stripped. However, in #9640, we "fixed" it to not subclass `FilesSources` because this was causing lots of issues with the file being unintentionally included in chroots. Yet, as a result, using `await Get(StrippedSourceFiles)` will try to strip the file, unless the plugin author uses `for_sources_type` and leaves off the type in the allowlist.

This PR allows us to properly handle both use cases by generifying the mechanism for "unrooted sources". Now, any `Sources` subclass can express whether it uses source roots, i.e. it's no longer hardcoded to `FilesSources`.

[ci skip-rust]
[ci skip-build-wheels]
  • Loading branch information
Eric-Arellano committed Mar 17, 2021
1 parent 030c99f commit 305d62f
Show file tree
Hide file tree
Showing 6 changed files with 23 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,15 @@ def test_requirements_txt(rule_runner: RuleRunner) -> None:

def test_invalid_req(rule_runner: RuleRunner) -> None:
"""Test that we give a nice error message."""
fake_file_tgt = PythonRequirementsFile(
{"sources": ["doesnt matter"]}, address=Address("doesnt_matter")
)
with pytest.raises(ExecutionError) as exc:
assert_python_requirements(
rule_runner,
"python_requirements()",
"\n\nNot A Valid Req == 3.7",
expected_file_dep=PythonRequirementsFile({}, address=Address("doesnt_matter")),
expected_file_dep=fake_file_tgt,
expected_targets=[],
)
assert "Invalid requirement 'Not A Valid Req == 3.7' in requirements.txt at line 3" in str(
Expand All @@ -124,7 +127,7 @@ def test_invalid_req(rule_runner: RuleRunner) -> None:
rule_runner,
"python_requirements()",
"git+https://github.com/pypa/pip.git#egg=pip",
expected_file_dep=PythonRequirementsFile({}, address=Address("doesnt_matter")),
expected_file_dep=fake_file_tgt,
expected_targets=[],
)
assert "It looks like you're trying to use a pip VCS-style requirement?" in str(exc.value)
Expand Down
3 changes: 2 additions & 1 deletion src/python/pants/backend/python/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,8 @@ def parse_requirements_file(content: str, *, rel_path: str) -> Iterator[Requirem


class PythonRequirementsFileSources(Sources):
pass
required = True
uses_source_roots = False


class PythonRequirementsFile(Target):
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/core/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@

class FilesSources(Sources):
required = True
uses_source_roots = False


class Files(Target):
Expand Down
3 changes: 1 addition & 2 deletions src/python/pants/core/util_rules/source_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
from dataclasses import dataclass
from typing import Iterable, Set, Tuple, Type

from pants.core.target_types import FilesSources
from pants.engine.fs import MergeDigests, Snapshot
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.target import HydratedSources, HydrateSourcesRequest
Expand Down Expand Up @@ -63,7 +62,7 @@ async def determine_source_files(request: SourceFilesRequest) -> SourceFiles:
)

for hydrated_sources, sources_field in zip(all_hydrated_sources, request.sources_fields):
if isinstance(sources_field, FilesSources):
if not sources_field.uses_source_roots:
unrooted_files.update(hydrated_sources.snapshot.files)

digests_to_merge = tuple(
Expand Down
12 changes: 11 additions & 1 deletion src/python/pants/core/util_rules/source_files_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,23 @@ def test_address_specs(rule_runner: RuleRunner) -> None:
)


def test_file_sources(rule_runner: RuleRunner) -> None:
def test_unrooted_sources(rule_runner: RuleRunner) -> None:
"""Any Sources field with `uses_source_roots=False`, such as `FilesSources`, should be marked as
unrooted sources."""
sources = TargetSources("src/python", ["README.md"])
field = mock_sources_field(rule_runner, sources, sources_field_cls=FilesSources)
assert_sources_resolved(
rule_runner, [field], expected=[sources], expected_unrooted=sources.full_paths
)

class CustomSources(SourcesField):
uses_source_roots = False

field = mock_sources_field(rule_runner, sources, sources_field_cls=CustomSources)
assert_sources_resolved(
rule_runner, [field], expected=[sources], expected_unrooted=sources.full_paths
)


def test_gracefully_handle_no_sources(rule_runner: RuleRunner) -> None:
sources_field = mock_sources_field(rule_runner, SOURCES1, include_sources=False)
Expand Down
5 changes: 3 additions & 2 deletions src/python/pants/engine/target.py
Original file line number Diff line number Diff line change
Expand Up @@ -1212,8 +1212,9 @@ def compute_value(

class Sources(StringSequenceField, AsyncFieldMixin):
alias = "sources"
expected_file_extensions: ClassVar[Optional[Tuple[str, ...]]] = None
expected_num_files: ClassVar[Optional[Union[int, range]]] = None
expected_file_extensions: ClassVar[tuple[str, ...] | None] = None
expected_num_files: ClassVar[int | range | None] = None
uses_source_roots: ClassVar[bool] = True
help = (
"A list of files and globs that belong to this target.\n\nPaths are relative to the BUILD "
"file's directory. You can ignore files/globs by prefixing them with `!`.\n\nExample: "
Expand Down

0 comments on commit 305d62f

Please sign in to comment.