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]` (#14082)

To minimize behavior changes while implementing #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: #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 committed Jan 5, 2022
1 parent 593948a commit dac9ad3
Show file tree
Hide file tree
Showing 11 changed files with 62 additions and 56 deletions.
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
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
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
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
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
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
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
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
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
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
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.