Skip to content

Commit

Permalink
_python_requirements_file target uses source: str field instead o…
Browse files Browse the repository at this point in the history
…f `sources: list[str]` (pantsbuild#14082)

To minimize behavior changes while implementing pantsbuild#14075, we want to make sure that `--changed-since=HEAD --changed-dependees=direct` will still claim that all generated `python_requirement` targets are impacted if the `requirements.txt` changed.

This would happen naturally if a generated target depended on its target generator: pantsbuild#13118. But that will require a lot more design work and is a big change we're not ready to make.

So we can work around this by still having `_python_requirements_file` even with target generators. But before doing that, we should change to `source: str` to actually represent the expected API of the target.

Because this is a private API, we make a breaking change.

[ci skip-rust]
[ci skip-build-wheels]
  • Loading branch information
Eric-Arellano authored Jan 5, 2022
1 parent 593948a commit dac9ad3
Show file tree
Hide file tree
Showing 11 changed files with 62 additions and 56 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
)
from pants.backend.python.macros.python_requirements_caof import PythonRequirementsCAOF
from pants.backend.python.target_types import (
PythonRequirementsFile,
PythonRequirementsFileTarget,
PythonRequirementTarget,
PythonSourceField,
PythonSourcesGeneratorTarget,
Expand Down Expand Up @@ -265,7 +265,7 @@ def test_infer_python_strict(caplog) -> None:
target_types=[
PythonSourcesGeneratorTarget,
PythonRequirementTarget,
PythonRequirementsFile,
PythonRequirementsFileTarget,
],
context_aware_object_factories={"python_requirements": PythonRequirementsCAOF},
)
Expand Down
12 changes: 6 additions & 6 deletions src/python/pants/backend/python/macros/deprecation_fixers.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import tokenize
from dataclasses import dataclass

from pants.backend.python.target_types import PythonRequirementsFile, PythonRequirementTarget
from pants.backend.python.target_types import PythonRequirementsFileTarget, PythonRequirementTarget
from pants.build_graph.address import InvalidAddress
from pants.core.goals.update_build_files import (
DeprecationFixerRequest,
Expand All @@ -22,7 +22,7 @@
Dependencies,
DependenciesRequest,
ExplicitlyProvidedDependencies,
MultipleSourcesField,
SingleSourceField,
UnexpandedTargets,
)
from pants.engine.unions import UnionRule
Expand Down Expand Up @@ -83,13 +83,13 @@ async def determine_macro_changes(all_targets: AllTargets) -> MacroRenames:
for python_req_deps_field, build_file_addr, deps in zip(
python_requirement_dependencies_fields, build_file_addresses_per_tgt, deps_per_tgt
):
generator_tgt = next((tgt for tgt in deps if isinstance(tgt, PythonRequirementsFile)), None)
generator_tgt = next(
(tgt for tgt in deps if isinstance(tgt, PythonRequirementsFileTarget)), None
)
if generator_tgt is None:
continue

# Assume there is a single source. We should have changed this target to use
# `SingleSourceField`, alas.
generator_source = generator_tgt[MultipleSourcesField].value[0] # type: ignore[index]
generator_source = generator_tgt[SingleSourceField].value
if "Pipfile" in generator_source:
generator_alias = "pipenv_requirements"
elif "pyproject.toml" in generator_source:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from pants.backend.python.macros.pipenv_requirements_caof import PipenvRequirementsCAOF
from pants.backend.python.macros.poetry_requirements_caof import PoetryRequirementsCAOF
from pants.backend.python.macros.python_requirements_caof import PythonRequirementsCAOF
from pants.backend.python.target_types import PythonRequirementsFile, PythonRequirementTarget
from pants.backend.python.target_types import PythonRequirementsFileTarget, PythonRequirementTarget
from pants.core.goals.update_build_files import RewrittenBuildFile
from pants.core.target_types import GenericTarget
from pants.engine.addresses import Address
Expand All @@ -33,7 +33,7 @@ def rule_runner() -> RuleRunner:
QueryRule(MacroRenames, []),
QueryRule(RewrittenBuildFile, [UpdatePythonMacrosRequest]),
),
target_types=[GenericTarget, PythonRequirementsFile, PythonRequirementTarget],
target_types=[GenericTarget, PythonRequirementsFileTarget, PythonRequirementTarget],
context_aware_object_factories={
"python_requirements": PythonRequirementsCAOF,
"poetry_requirements": PoetryRequirementsCAOF,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def __call__(
self._parse_context.create_object(
"_python_requirements_file",
name=requirements_file_target_name,
sources=[source],
source=source,
)
requirements_dep = f":{requirements_file_target_name}"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

from pants.backend.python.macros.pipenv_requirements_caof import PipenvRequirementsCAOF
from pants.backend.python.pip_requirement import PipRequirement
from pants.backend.python.target_types import PythonRequirementsFile, PythonRequirementTarget
from pants.backend.python.target_types import PythonRequirementsFileTarget, PythonRequirementTarget
from pants.engine.addresses import Address
from pants.engine.target import AllTargets
from pants.testutil.rule_runner import RuleRunner
Expand All @@ -18,7 +18,7 @@
@pytest.fixture
def rule_runner() -> RuleRunner:
return RuleRunner(
target_types=[PythonRequirementTarget, PythonRequirementsFile],
target_types=[PythonRequirementTarget, PythonRequirementsFileTarget],
context_aware_object_factories={"pipenv_requirements": PipenvRequirementsCAOF},
)

Expand All @@ -28,7 +28,7 @@ def assert_pipenv_requirements(
build_file_entry: str,
pipfile_lock: dict,
*,
expected_file_dep: PythonRequirementsFile,
expected_file_dep: PythonRequirementsFileTarget,
expected_targets: Iterable[PythonRequirementTarget],
pipfile_lock_relpath: str = "Pipfile.lock",
) -> None:
Expand All @@ -53,8 +53,8 @@ def test_pipfile_lock(rule_runner: RuleRunner) -> None:
"default": {"ansicolors": {"version": ">=1.18.0"}},
"develop": {"cachetools": {"markers": "python_version ~= '3.5'", "version": "==4.1.1"}},
},
expected_file_dep=PythonRequirementsFile(
{"sources": ["Pipfile.lock"]}, Address("", target_name="Pipfile.lock")
expected_file_dep=PythonRequirementsFileTarget(
{"source": "Pipfile.lock"}, Address("", target_name="Pipfile.lock")
),
expected_targets=[
PythonRequirementTarget(
Expand Down Expand Up @@ -93,8 +93,8 @@ def test_properly_creates_extras_requirements(rule_runner: RuleRunner) -> None:
}
},
},
expected_file_dep=PythonRequirementsFile(
{"sources": ["Pipfile.lock"]}, Address("", target_name="Pipfile.lock")
expected_file_dep=PythonRequirementsFileTarget(
{"source": "Pipfile.lock"}, Address("", target_name="Pipfile.lock")
),
expected_targets=[
PythonRequirementTarget(
Expand Down Expand Up @@ -132,13 +132,13 @@ def test_supply_python_requirements_file(rule_runner: RuleRunner) -> None:
_python_requirements_file(
name='custom_pipfile_target',
sources=['custom/pipfile/Pipfile.lock']
source='custom/pipfile/Pipfile.lock'
)
"""
),
{"default": {"ansicolors": {"version": ">=1.18.0"}}},
expected_file_dep=PythonRequirementsFile(
{"sources": ["custom/pipfile/Pipfile.lock"]},
expected_file_dep=PythonRequirementsFileTarget(
{"source": "custom/pipfile/Pipfile.lock"},
Address("", target_name="custom_pipfile_target"),
),
expected_targets=[
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ def __call__(
req_file_tgt = self._parse_context.create_object(
"_python_requirements_file",
name=source.replace(os.path.sep, "_"),
sources=[source],
source=source,
)
requirements_dep = f":{req_file_tgt.name}"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
parse_str_version,
)
from pants.backend.python.pip_requirement import PipRequirement
from pants.backend.python.target_types import PythonRequirementsFile, PythonRequirementTarget
from pants.backend.python.target_types import PythonRequirementsFileTarget, PythonRequirementTarget
from pants.engine.addresses import Address
from pants.engine.internals.scheduler import ExecutionError
from pants.engine.target import AllTargets
Expand Down Expand Up @@ -391,7 +391,7 @@ def test_parse_multi_reqs() -> None:
@pytest.fixture
def rule_runner() -> RuleRunner:
return RuleRunner(
target_types=[PythonRequirementTarget, PythonRequirementsFile],
target_types=[PythonRequirementTarget, PythonRequirementsFileTarget],
context_aware_object_factories={"poetry_requirements": PoetryRequirementsCAOF},
)

Expand All @@ -401,7 +401,7 @@ def assert_poetry_requirements(
build_file_entry: str,
pyproject_toml: str,
*,
expected_file_dep: PythonRequirementsFile,
expected_file_dep: PythonRequirementsFileTarget,
expected_targets: Iterable[PythonRequirementTarget],
pyproject_toml_relpath: str = "pyproject.toml",
) -> None:
Expand Down Expand Up @@ -438,8 +438,8 @@ def test_pyproject_toml(rule_runner: RuleRunner) -> None:
ansicolors = ">=1.18.0"
"""
),
expected_file_dep=PythonRequirementsFile(
{"sources": ["pyproject.toml"]},
expected_file_dep=PythonRequirementsFileTarget(
{"source": "pyproject.toml"},
address=Address("", target_name="pyproject.toml"),
),
expected_targets=[
Expand Down Expand Up @@ -489,8 +489,8 @@ def test_source_override(rule_runner: RuleRunner) -> None:
"""
),
pyproject_toml_relpath="subdir/pyproject.toml",
expected_file_dep=PythonRequirementsFile(
{"sources": ["subdir/pyproject.toml"]},
expected_file_dep=PythonRequirementsFileTarget(
{"source": "subdir/pyproject.toml"},
address=Address("", target_name="subdir_pyproject.toml"),
),
expected_targets=[
Expand All @@ -515,8 +515,8 @@ def test_non_pep440_error(rule_runner: RuleRunner, caplog: Any) -> None:
foo = "~r62b"
[tool.poetry.dev-dependencies]
""",
expected_file_dep=PythonRequirementsFile(
{"sources": ["pyproject.toml"]},
expected_file_dep=PythonRequirementsFileTarget(
{"source": "pyproject.toml"},
address=Address("", target_name="pyproject.toml"),
),
expected_targets=[],
Expand All @@ -532,8 +532,8 @@ def test_no_req_defined_warning(rule_runner: RuleRunner, caplog: Any) -> None:
[tool.poetry.dependencies]
[tool.poetry.dev-dependencies]
""",
expected_file_dep=PythonRequirementsFile(
{"sources": ["pyproject.toml"]},
expected_file_dep=PythonRequirementsFileTarget(
{"source": "pyproject.toml"},
address=Address("", target_name="pyproject.toml"),
),
expected_targets=[],
Expand All @@ -551,8 +551,8 @@ def test_bad_dict_format(rule_runner: RuleRunner) -> None:
foo = {bad_req = "test"}
[tool.poetry.dev-dependencies]
""",
expected_file_dep=PythonRequirementsFile(
{"sources": ["pyproject.toml"]},
expected_file_dep=PythonRequirementsFileTarget(
{"source": "pyproject.toml"},
address=Address("", target_name="pyproject.toml"),
),
expected_targets=[],
Expand All @@ -570,8 +570,8 @@ def test_bad_req_type(rule_runner: RuleRunner) -> None:
foo = 4
[tool.poetry.dev-dependencies]
""",
expected_file_dep=PythonRequirementsFile(
{"sources": ["pyproject.toml"]},
expected_file_dep=PythonRequirementsFileTarget(
{"source": "pyproject.toml"},
address=Address("", target_name="pyproject.toml"),
),
expected_targets=[],
Expand All @@ -587,8 +587,8 @@ def test_no_tool_poetry(rule_runner: RuleRunner) -> None:
"""
foo = 4
""",
expected_file_dep=PythonRequirementsFile(
{"sources": ["pyproject.toml"]},
expected_file_dep=PythonRequirementsFileTarget(
{"source": "pyproject.toml"},
address=Address("", target_name="pyproject.toml"),
),
expected_targets=[],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def __call__(
req_file_tgt = self._parse_context.create_object(
"_python_requirements_file",
name=source.replace(os.path.sep, "_"),
sources=[source],
source=source,
)
requirements_dep = f":{req_file_tgt.name}"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

from pants.backend.python.macros.python_requirements_caof import PythonRequirementsCAOF
from pants.backend.python.pip_requirement import PipRequirement
from pants.backend.python.target_types import PythonRequirementsFile, PythonRequirementTarget
from pants.backend.python.target_types import PythonRequirementsFileTarget, PythonRequirementTarget
from pants.engine.addresses import Address
from pants.engine.internals.scheduler import ExecutionError
from pants.engine.target import AllTargets
Expand All @@ -18,7 +18,7 @@
@pytest.fixture
def rule_runner() -> RuleRunner:
return RuleRunner(
target_types=[PythonRequirementTarget, PythonRequirementsFile],
target_types=[PythonRequirementTarget, PythonRequirementsFileTarget],
context_aware_object_factories={"python_requirements": PythonRequirementsCAOF},
)

Expand All @@ -28,7 +28,7 @@ def assert_python_requirements(
build_file_entry: str,
requirements_txt: str,
*,
expected_file_dep: PythonRequirementsFile,
expected_file_dep: PythonRequirementsFileTarget,
expected_targets: Iterable[PythonRequirementTarget],
requirements_txt_relpath: str = "requirements.txt",
) -> None:
Expand Down Expand Up @@ -68,8 +68,8 @@ def test_requirements_txt(rule_runner: RuleRunner) -> None:
pip@ git+https://github.com/pypa/pip.git
"""
),
expected_file_dep=PythonRequirementsFile(
{"sources": ["requirements.txt"]},
expected_file_dep=PythonRequirementsFileTarget(
{"source": "requirements.txt"},
Address("", target_name="requirements.txt"),
),
expected_targets=[
Expand Down Expand Up @@ -132,8 +132,8 @@ def test_multiple_versions(rule_runner: RuleRunner) -> None:
repletewateringcan>=7
"""
),
expected_file_dep=PythonRequirementsFile(
{"sources": ["requirements.txt"]},
expected_file_dep=PythonRequirementsFileTarget(
{"source": "requirements.txt"},
Address("", target_name="requirements.txt"),
),
expected_targets=[
Expand Down Expand Up @@ -167,7 +167,9 @@ def test_multiple_versions(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("doesnt_matter"))
fake_file_tgt = PythonRequirementsFileTarget(
{"source": "doesnt matter"}, Address("doesnt_matter")
)
with pytest.raises(ExecutionError) as exc:
assert_python_requirements(
rule_runner,
Expand Down Expand Up @@ -198,8 +200,8 @@ def test_source_override(rule_runner: RuleRunner) -> None:
"python_requirements(source='subdir/requirements.txt')",
"ansicolors>=1.18.0",
requirements_txt_relpath="subdir/requirements.txt",
expected_file_dep=PythonRequirementsFile(
{"sources": ["subdir/requirements.txt"]},
expected_file_dep=PythonRequirementsFileTarget(
{"source": "subdir/requirements.txt"},
Address("", target_name="subdir_requirements.txt"),
),
expected_targets=[
Expand Down
4 changes: 2 additions & 2 deletions src/python/pants/backend/python/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
PexBinariesGeneratorTarget,
PexBinary,
PythonDistribution,
PythonRequirementsFile,
PythonRequirementsFileTarget,
PythonRequirementTarget,
PythonSourcesGeneratorTarget,
PythonSourceTarget,
Expand Down Expand Up @@ -91,7 +91,7 @@ def target_types():
PexBinary,
PexBinariesGeneratorTarget,
PythonDistribution,
PythonRequirementsFile,
PythonRequirementsFileTarget,
PythonRequirementTarget,
PythonSourcesGeneratorTarget,
PythonSourceTarget,
Expand Down
12 changes: 8 additions & 4 deletions src/python/pants/backend/python/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -1073,15 +1073,19 @@ def parse_requirements_file(content: str, *, rel_path: str) -> Iterator[PipRequi
)


class PythonRequirementsFileSourcesField(MultipleSourcesField):
required = True
class PythonRequirementsFileSourcesField(SingleSourceField):
uses_source_roots = False


class PythonRequirementsFile(Target):
# This allows us to work around https://github.com/pantsbuild/pants/issues/13118. Because a
# generated target does not depend on its target generator, `--changed-since --changed-dependees`
# would not mark the generated targets as changing when the `requirements.txt` changes, even though
# it may be impacted. Fixing that will be A Thing and requires design work, so instead we can
# depend on this private target type that owns `requirements.txt` to get `--changed-since` working.
class PythonRequirementsFileTarget(Target):
alias = "_python_requirements_file"
core_fields = (*COMMON_TARGET_FIELDS, PythonRequirementsFileSourcesField)
help = "A private helper target type for requirements.txt files."
help = "A private helper target type used by `python_requirement` target generators."


# -----------------------------------------------------------------------------------------------
Expand Down

0 comments on commit dac9ad3

Please sign in to comment.