From 3bdacf3e979bb014b25875fbd9619de959badafd Mon Sep 17 00:00:00 2001 From: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> Date: Mon, 22 Aug 2022 23:25:44 -0500 Subject: [PATCH] Mark `[python].requirement_constraints` as deprecated (but not planned for removal) (#16606) We decided in Slack to not plan to remove this mechanism until Pants 3.0. While we strongly encourage using `[python].resolves` instead, we don't want to _force_ users to migrate. We only want to _encourage_ migration. This PR also refactors `pex_from_targets.py` so that the impact of the old code is even better isolated. Note that we will always need to keep around a lot of the relevant code with how to subset via `--repository-pex` because we decided to support manually managed `requirements.txt`-style locks. --- .../pants/backend/python/subsystems/setup.py | 40 ++++++++++-- .../python/util_rules/pex_from_targets.py | 62 +++++++------------ 2 files changed, 60 insertions(+), 42 deletions(-) diff --git a/src/python/pants/backend/python/subsystems/setup.py b/src/python/pants/backend/python/subsystems/setup.py index 73c41bded45..174045f61f1 100644 --- a/src/python/pants/backend/python/subsystems/setup.py +++ b/src/python/pants/backend/python/subsystems/setup.py @@ -428,6 +428,20 @@ class PythonSetup(Subsystem): ), advanced=True, ) + + __constraints_deprecation_msg = softwrap( + f""" + We encourage instead migrating to `[python].enable_resolves` and `[python].resolves`, + which is an improvement over this option. The `[python].resolves` feature ensures that + your lockfiles are fully comprehensive, i.e. include all transitive dependencies; + uses hashes for better supply chain security; and supports advanced features like VCS + and local requirements, along with options `[python].resolves_to_only_binary`. + + To migrate, stop setting `[python].requirement_constraints` and + `[python].resolve_all_constraints`, and instead set `[python].enable_resolves` to + `true`. Then, run `{bin_name()} generate-lockfiles`. + """ + ) requirement_constraints = FileOption( default=None, help=softwrap( @@ -449,8 +463,10 @@ class PythonSetup(Subsystem): ), advanced=True, mutually_exclusive_group="lockfile", + removal_version="3.0.0.dev0", + removal_hint=__constraints_deprecation_msg, ) - resolve_all_constraints = BoolOption( + _resolve_all_constraints = BoolOption( default=True, help=softwrap( """ @@ -466,6 +482,8 @@ class PythonSetup(Subsystem): """ ), advanced=True, + removal_version="3.0.0.dev0", + removal_hint=__constraints_deprecation_msg, ) no_binary = StrListOption( help=softwrap( @@ -755,9 +773,6 @@ def resolves_to_only_binary( ).items() } - def resolve_all_constraints_was_set_explicitly(self) -> bool: - return not self.options.is_default("resolve_all_constraints") - @property def manylinux(self) -> str | None: manylinux = cast(Optional[str], self.resolver_manylinux) @@ -765,6 +780,23 @@ def manylinux(self) -> str | None: return None return manylinux + @property + def resolve_all_constraints(self) -> bool: + if ( + self._resolve_all_constraints + and not self.options.is_default("resolve_all_constraints") + and not self.requirement_constraints + ): + raise ValueError( + softwrap( + """ + `[python].resolve_all_constraints` is enabled, so + `[python].requirement_constraints` must also be set. + """ + ) + ) + return self._resolve_all_constraints + @property def scratch_dir(self): return os.path.join(self.options.pants_workdir, *self.options_scope.split(".")) diff --git a/src/python/pants/backend/python/util_rules/pex_from_targets.py b/src/python/pants/backend/python/util_rules/pex_from_targets.py index b0c4701d192..054e708410a 100644 --- a/src/python/pants/backend/python/util_rules/pex_from_targets.py +++ b/src/python/pants/backend/python/util_rules/pex_from_targets.py @@ -325,6 +325,8 @@ async def determine_requirement_strings_in_closure( for tgt in transitive_targets.closure if tgt.has_field(PythonRequirementsField) ), + # This is only set if `[python].requirement_constraints` is configured, which is mutually + # exclusive with resolves. constraints_strings=(str(constraint) for constraint in global_requirement_constraints), ) @@ -398,13 +400,8 @@ async def _determine_requirements_for_pex_from_targets( should_request_repository_pex = ( # The entire lockfile was explicitly requested. should_return_entire_lockfile - # The legacy `resolve_all_constraints`+`requirement_constraints` options were used. - or ( - # TODO: The constraints.txt resolve for `resolve_all_constraints` will be removed as - # part of #12314. - python_setup.resolve_all_constraints - and python_setup.requirement_constraints - ) + # The legacy `resolve_all_constraints` + or (python_setup.resolve_all_constraints and python_setup.requirement_constraints) # A non-PEX-native lockfile was used, and so we cannot directly subset it from a # LoadedLockfile. or not pex_native_subsetting_supported @@ -534,40 +531,29 @@ async def get_repository_pex( if request.platforms or request.complete_platforms: return OptionalPexRequest(None) - interpreter_constraints = await Get( - InterpreterConstraints, - InterpreterConstraintsRequest, - request.to_interpreter_constraints_request(), - ) - - repository_pex_request: PexRequest | None = None if python_setup.requirement_constraints: constraints_repository_pex_request = await Get( - OptionalPexRequest, - _ConstraintsRepositoryPexRequest(request), + OptionalPexRequest, _ConstraintsRepositoryPexRequest(request) ) - repository_pex_request = constraints_repository_pex_request.maybe_pex_request - elif ( - python_setup.resolve_all_constraints - and python_setup.resolve_all_constraints_was_set_explicitly() - ): - raise ValueError( - softwrap( - """ - `[python].resolve_all_constraints` is enabled, so - `[python].requirement_constraints` must also be set. - """ - ) - ) - elif python_setup.enable_resolves: - chosen_resolve = await Get( - ChosenPythonResolve, ChosenPythonResolveRequest(request.addresses) - ) - repository_pex_request = PexRequest( + return OptionalPexRequest(constraints_repository_pex_request.maybe_pex_request) + + if not python_setup.enable_resolves: + return OptionalPexRequest(None) + + chosen_resolve, interpreter_constraints = await MultiGet( + Get(ChosenPythonResolve, ChosenPythonResolveRequest(request.addresses)), + Get( + InterpreterConstraints, + InterpreterConstraintsRequest, + request.to_interpreter_constraints_request(), + ), + ) + return OptionalPexRequest( + PexRequest( description=softwrap( f""" - Installing {chosen_resolve.lockfile.file_path} - for the resolve `{chosen_resolve.name}` + Installing {chosen_resolve.lockfile.file_path} for the resolve + `{chosen_resolve.name}` """ ), output_filename=f"{path_safe(chosen_resolve.name)}_lockfile.pex", @@ -579,7 +565,7 @@ async def get_repository_pex( complete_platforms=request.complete_platforms, additional_args=request.additional_lockfile_args, ) - return OptionalPexRequest(repository_pex_request) + ) @rule @@ -694,7 +680,7 @@ def __init__( @rule -async def generalize_requirementspexrequest( +async def generalize_requirements_pex_request( request: RequirementsPexRequest, ) -> PexFromTargetsRequest: return PexFromTargetsRequest(