From dac9ad3f21f23db17c9e4d21ec68395350e48611 Mon Sep 17 00:00:00 2001 From: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> Date: Wed, 5 Jan 2022 12:16:26 -0700 Subject: [PATCH] `_python_requirements_file` target uses `source: str` field instead of `sources: list[str]` (#14082) To minimize behavior changes while implementing https://github.com/pantsbuild/pants/pull/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: https://github.com/pantsbuild/pants/issues/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] --- .../python/dependency_inference/rules_test.py | 4 +-- .../python/macros/deprecation_fixers.py | 12 +++---- .../python/macros/deprecation_fixers_test.py | 4 +-- .../python/macros/pipenv_requirements_caof.py | 2 +- .../macros/pipenv_requirements_caof_test.py | 20 +++++------ .../python/macros/poetry_requirements_caof.py | 2 +- .../macros/poetry_requirements_caof_test.py | 34 +++++++++---------- .../python/macros/python_requirements_caof.py | 2 +- .../macros/python_requirements_caof_test.py | 22 ++++++------ src/python/pants/backend/python/register.py | 4 +-- .../pants/backend/python/target_types.py | 12 ++++--- 11 files changed, 62 insertions(+), 56 deletions(-) diff --git a/src/python/pants/backend/python/dependency_inference/rules_test.py b/src/python/pants/backend/python/dependency_inference/rules_test.py index 3b5a31edddf..16501767587 100644 --- a/src/python/pants/backend/python/dependency_inference/rules_test.py +++ b/src/python/pants/backend/python/dependency_inference/rules_test.py @@ -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, @@ -265,7 +265,7 @@ def test_infer_python_strict(caplog) -> None: target_types=[ PythonSourcesGeneratorTarget, PythonRequirementTarget, - PythonRequirementsFile, + PythonRequirementsFileTarget, ], context_aware_object_factories={"python_requirements": PythonRequirementsCAOF}, ) diff --git a/src/python/pants/backend/python/macros/deprecation_fixers.py b/src/python/pants/backend/python/macros/deprecation_fixers.py index 848a5237538..8cc49ae6f14 100644 --- a/src/python/pants/backend/python/macros/deprecation_fixers.py +++ b/src/python/pants/backend/python/macros/deprecation_fixers.py @@ -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, @@ -22,7 +22,7 @@ Dependencies, DependenciesRequest, ExplicitlyProvidedDependencies, - MultipleSourcesField, + SingleSourceField, UnexpandedTargets, ) from pants.engine.unions import UnionRule @@ -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: diff --git a/src/python/pants/backend/python/macros/deprecation_fixers_test.py b/src/python/pants/backend/python/macros/deprecation_fixers_test.py index 0e638068d9d..6eeaa8502a6 100644 --- a/src/python/pants/backend/python/macros/deprecation_fixers_test.py +++ b/src/python/pants/backend/python/macros/deprecation_fixers_test.py @@ -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 @@ -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, diff --git a/src/python/pants/backend/python/macros/pipenv_requirements_caof.py b/src/python/pants/backend/python/macros/pipenv_requirements_caof.py index c4054ab23cb..fa7a2455f3a 100644 --- a/src/python/pants/backend/python/macros/pipenv_requirements_caof.py +++ b/src/python/pants/backend/python/macros/pipenv_requirements_caof.py @@ -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}" diff --git a/src/python/pants/backend/python/macros/pipenv_requirements_caof_test.py b/src/python/pants/backend/python/macros/pipenv_requirements_caof_test.py index 6d53bf1edf8..b4d572b933b 100644 --- a/src/python/pants/backend/python/macros/pipenv_requirements_caof_test.py +++ b/src/python/pants/backend/python/macros/pipenv_requirements_caof_test.py @@ -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 @@ -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}, ) @@ -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: @@ -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( @@ -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( @@ -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=[ diff --git a/src/python/pants/backend/python/macros/poetry_requirements_caof.py b/src/python/pants/backend/python/macros/poetry_requirements_caof.py index c601133e150..5dad9cfe6df 100644 --- a/src/python/pants/backend/python/macros/poetry_requirements_caof.py +++ b/src/python/pants/backend/python/macros/poetry_requirements_caof.py @@ -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}" diff --git a/src/python/pants/backend/python/macros/poetry_requirements_caof_test.py b/src/python/pants/backend/python/macros/poetry_requirements_caof_test.py index 916af95547c..c7a4ef24a7c 100644 --- a/src/python/pants/backend/python/macros/poetry_requirements_caof_test.py +++ b/src/python/pants/backend/python/macros/poetry_requirements_caof_test.py @@ -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 @@ -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}, ) @@ -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: @@ -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=[ @@ -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=[ @@ -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=[], @@ -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=[], @@ -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=[], @@ -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=[], @@ -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=[], diff --git a/src/python/pants/backend/python/macros/python_requirements_caof.py b/src/python/pants/backend/python/macros/python_requirements_caof.py index aab97bca4e5..fa708372e69 100644 --- a/src/python/pants/backend/python/macros/python_requirements_caof.py +++ b/src/python/pants/backend/python/macros/python_requirements_caof.py @@ -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}" diff --git a/src/python/pants/backend/python/macros/python_requirements_caof_test.py b/src/python/pants/backend/python/macros/python_requirements_caof_test.py index 020a0d0656d..68221a9aaa6 100644 --- a/src/python/pants/backend/python/macros/python_requirements_caof_test.py +++ b/src/python/pants/backend/python/macros/python_requirements_caof_test.py @@ -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 @@ -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}, ) @@ -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: @@ -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=[ @@ -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=[ @@ -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, @@ -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=[ diff --git a/src/python/pants/backend/python/register.py b/src/python/pants/backend/python/register.py index 2fc72d0ab0f..7a437dc184f 100644 --- a/src/python/pants/backend/python/register.py +++ b/src/python/pants/backend/python/register.py @@ -28,7 +28,7 @@ PexBinariesGeneratorTarget, PexBinary, PythonDistribution, - PythonRequirementsFile, + PythonRequirementsFileTarget, PythonRequirementTarget, PythonSourcesGeneratorTarget, PythonSourceTarget, @@ -91,7 +91,7 @@ def target_types(): PexBinary, PexBinariesGeneratorTarget, PythonDistribution, - PythonRequirementsFile, + PythonRequirementsFileTarget, PythonRequirementTarget, PythonSourcesGeneratorTarget, PythonSourceTarget, diff --git a/src/python/pants/backend/python/target_types.py b/src/python/pants/backend/python/target_types.py index a17aeb303df..8b0f26207d4 100644 --- a/src/python/pants/backend/python/target_types.py +++ b/src/python/pants/backend/python/target_types.py @@ -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." # -----------------------------------------------------------------------------------------------