From e2f06fa49f094a9bc752e279f02ff2e26e8de1df Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 28 Sep 2022 09:00:13 -0700 Subject: [PATCH 1/4] Adds `_preprocessed_interpreter_search_paths` # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- .../pants/core/subsystems/python_bootstrap.py | 43 ++++++++++++++++++- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/src/python/pants/core/subsystems/python_bootstrap.py b/src/python/pants/core/subsystems/python_bootstrap.py index ab3748b9bd0..664c3e4c5e0 100644 --- a/src/python/pants/core/subsystems/python_bootstrap.py +++ b/src/python/pants/core/subsystems/python_bootstrap.py @@ -14,6 +14,7 @@ from pants.base.build_environment import get_buildroot from pants.core.util_rules import asdf from pants.core.util_rules.asdf import AsdfToolPathsRequest, AsdfToolPathsResult +from pants.core.util_rules.environments import LocalEnvironmentTarget from pants.engine.env_vars import EnvironmentVars from pants.engine.rules import Get, collect_rules, rule from pants.option.option_types import StrListOption @@ -35,7 +36,7 @@ class PythonBootstrapSubsystem(Subsystem): """ ) - class EnvironmentAware: + class EnvironmentAware(Subsystem.EnvironmentAware): search_path = StrListOption( default=["", ""], help=softwrap( @@ -52,7 +53,12 @@ class EnvironmentAware: The following special strings are supported: + For all runtime environment types: + * ``, the contents of the PATH env var + + For only local runtime environments: + * ``, all Python versions currently configured by ASDF \ `(asdf shell, ${HOME}/.tool-versions)`, with a fallback to all installed versions * ``, the ASDF interpreter with the version in BUILD_ROOT/.tool-versions @@ -214,12 +220,45 @@ def get_pyenv_root(env: EnvironmentVars) -> str | None: return None +def _preprocessed_interpreter_search_paths( + python_bootstrap_subsystem: PythonBootstrapSubsystem.EnvironmentAware, +) -> tuple[str, ...]: + + env = python_bootstrap_subsystem.env_tgt.val + + if isinstance(env, LocalEnvironmentTarget): + return python_bootstrap_subsystem.search_path + if env is None: + return python_bootstrap_subsystem.search_path + + if python_bootstrap_subsystem.options.is_default( + "search_path" + ) and python_bootstrap_subsystem.search_path == tuple( + python_bootstrap_subsystem.options.get("search_path") + ): + return ("",) # Just skip `PyEnv` + + search_paths = python_bootstrap_subsystem.search_path + not_allowed = {"", "", "", ""} + any_not_allowed = set(search_paths) & not_allowed + if any_not_allowed: + env_type = type(env) + raise ValueError( + f"`[python-bootstrap].search_paths` is configured to use local Python discovery " + f"tools, which do not work in {env_type.__name__} runtime environments. To fix this, " + f"set the value of `python_bootstrap_search_path` in the {env.alias} defined at " + f"`{env.address}` to contain only hardcoded paths or the `` special string." + ) + + return search_paths + + @rule async def python_bootstrap( python_bootstrap_subsystem: PythonBootstrapSubsystem.EnvironmentAware, ) -> PythonBootstrap: - interpreter_search_paths = python_bootstrap_subsystem.search_path + interpreter_search_paths = _preprocessed_interpreter_search_paths(python_bootstrap_subsystem) interpreter_names = python_bootstrap_subsystem.names has_standard_path_token, has_local_path_token = _contains_asdf_path_tokens( From c24bdaaad862121d8af526f74bf3df992586ffb6 Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 28 Sep 2022 09:54:24 -0700 Subject: [PATCH 2/4] make `_preprocessed_interpreter_search_paths` testable # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- .../pants/core/subsystems/python_bootstrap.py | 31 ++++++++++--------- src/python/pants/option/subsystem.py | 11 +++++++ 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/python/pants/core/subsystems/python_bootstrap.py b/src/python/pants/core/subsystems/python_bootstrap.py index 664c3e4c5e0..1a0ea7a14fc 100644 --- a/src/python/pants/core/subsystems/python_bootstrap.py +++ b/src/python/pants/core/subsystems/python_bootstrap.py @@ -14,7 +14,7 @@ from pants.base.build_environment import get_buildroot from pants.core.util_rules import asdf from pants.core.util_rules.asdf import AsdfToolPathsRequest, AsdfToolPathsResult -from pants.core.util_rules.environments import LocalEnvironmentTarget +from pants.core.util_rules.environments import EnvironmentTarget, LocalEnvironmentTarget from pants.engine.env_vars import EnvironmentVars from pants.engine.rules import Get, collect_rules, rule from pants.option.option_types import StrListOption @@ -221,25 +221,24 @@ def get_pyenv_root(env: EnvironmentVars) -> str | None: def _preprocessed_interpreter_search_paths( - python_bootstrap_subsystem: PythonBootstrapSubsystem.EnvironmentAware, + env_tgt: EnvironmentTarget, + _search_paths: Iterable[str], + is_default: bool, ) -> tuple[str, ...]: - env = python_bootstrap_subsystem.env_tgt.val + env = env_tgt.val + search_paths = tuple(_search_paths) if isinstance(env, LocalEnvironmentTarget): - return python_bootstrap_subsystem.search_path + return search_paths if env is None: - return python_bootstrap_subsystem.search_path - - if python_bootstrap_subsystem.options.is_default( - "search_path" - ) and python_bootstrap_subsystem.search_path == tuple( - python_bootstrap_subsystem.options.get("search_path") - ): - return ("",) # Just skip `PyEnv` + return search_paths - search_paths = python_bootstrap_subsystem.search_path not_allowed = {"", "", "", ""} + + if is_default: + return tuple(path for path in search_paths if path not in not_allowed) + any_not_allowed = set(search_paths) & not_allowed if any_not_allowed: env_type = type(env) @@ -258,7 +257,11 @@ async def python_bootstrap( python_bootstrap_subsystem: PythonBootstrapSubsystem.EnvironmentAware, ) -> PythonBootstrap: - interpreter_search_paths = _preprocessed_interpreter_search_paths(python_bootstrap_subsystem) + interpreter_search_paths = _preprocessed_interpreter_search_paths( + python_bootstrap_subsystem.env_tgt, + python_bootstrap_subsystem.search_path, + python_bootstrap_subsystem._is_default("search_path"), + ) interpreter_names = python_bootstrap_subsystem.names has_standard_path_token, has_local_path_token = _contains_asdf_path_tokens( diff --git a/src/python/pants/option/subsystem.py b/src/python/pants/option/subsystem.py index 3c4213ff8fe..58800bfa536 100644 --- a/src/python/pants/option/subsystem.py +++ b/src/python/pants/option/subsystem.py @@ -124,6 +124,17 @@ def __getattribute__(self, __name: str) -> Any: # We should just return the default at this point. return default + def _is_default(self, __name: str) -> bool: + from pants.core.util_rules.environments import resolve_environment_sensitive_option + + v = getattr(type(self), __name) + assert isinstance(v, OptionsInfo) + + return ( + self.options.is_default(__name) + and resolve_environment_sensitive_option(v.flag_names[0], self) is None + ) + @memoized_classmethod def rules(cls: Any) -> Iterable[Rule]: from pants.core.util_rules.environments import add_option_fields_for From 1206c1501292465f8dfea649a04753540c52b58d Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 28 Sep 2022 10:25:35 -0700 Subject: [PATCH 3/4] Adds test for search path preproecessor # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- .../pants/core/subsystems/python_bootstrap.py | 18 +++- .../core/subsystems/python_bootstrap_test.py | 87 +++++++++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-) diff --git a/src/python/pants/core/subsystems/python_bootstrap.py b/src/python/pants/core/subsystems/python_bootstrap.py index 1a0ea7a14fc..ba15c6c22cf 100644 --- a/src/python/pants/core/subsystems/python_bootstrap.py +++ b/src/python/pants/core/subsystems/python_bootstrap.py @@ -225,6 +225,19 @@ def _preprocessed_interpreter_search_paths( _search_paths: Iterable[str], is_default: bool, ) -> tuple[str, ...]: + """Checks for special search path strings, and errors if any are invalid for the environment. + + This will return: + * The search paths, unaltered, for local/undefined environments, OR + * The search paths, with invalid tokens removed, if the provided value was unaltered from the + default value in the options system + (see `PythonBootstrapSubsystem.EnvironmentAware.search_paths`) + * The search paths unaltered, if the search paths do not contain tokens invalid for this + environment + + If the environment is non-local and there are invalid tokens for those environments, raise + `ValueError`. + """ env = env_tgt.val search_paths = tuple(_search_paths) @@ -234,9 +247,12 @@ def _preprocessed_interpreter_search_paths( if env is None: return search_paths - not_allowed = {"", "", "", ""} + not_allowed = {"", "", "", "", ""} if is_default: + # Strip out the not-allowed special strings from search_paths. + # An error will occur on the off chance the non-local environment expects pyenv + # but there's nothing we can do here to detect it. return tuple(path for path in search_paths if path not in not_allowed) any_not_allowed = set(search_paths) & not_allowed diff --git a/src/python/pants/core/subsystems/python_bootstrap_test.py b/src/python/pants/core/subsystems/python_bootstrap_test.py index eeaec5e183d..1e3d78c18e7 100644 --- a/src/python/pants/core/subsystems/python_bootstrap_test.py +++ b/src/python/pants/core/subsystems/python_bootstrap_test.py @@ -1,10 +1,14 @@ # Copyright 2018 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). +from __future__ import annotations + import os from contextlib import contextmanager from typing import Iterable, List, Sequence, TypeVar +import pytest + from pants.base.build_environment import get_pants_cachedir from pants.core.subsystems.python_bootstrap import ( PythonBootstrap, @@ -12,11 +16,20 @@ _get_environment_paths, _get_pex_python_paths, _get_pyenv_paths, + _preprocessed_interpreter_search_paths, get_pyenv_root, ) from pants.core.util_rules import asdf from pants.core.util_rules.asdf import AsdfToolPathsRequest, AsdfToolPathsResult from pants.core.util_rules.asdf_test import fake_asdf_root, get_asdf_paths +from pants.core.util_rules.environments import ( + DockerEnvironmentTarget, + DockerImageField, + EnvironmentTarget, + LocalEnvironmentTarget, + RemoteEnvironmentTarget, +) +from pants.engine.addresses import Address from pants.engine.env_vars import EnvironmentVars from pants.engine.rules import QueryRule from pants.testutil.rule_runner import RuleRunner @@ -192,3 +205,77 @@ def test_expand_interpreter_search_paths() -> None: "/qux", ) assert expected == expanded_paths + + +@pytest.mark.parametrize( + ("env_tgt_type", "search_paths", "is_default", "expected"), + ( + (LocalEnvironmentTarget, ("",), False, ("",)), + (LocalEnvironmentTarget, ("",), False, ("",)), + ( + LocalEnvironmentTarget, + ("", "/home/derryn/pythons"), + False, + ("", "/home/derryn/pythons"), + ), + (DockerEnvironmentTarget, ("", ""), True, ("",)), + (DockerEnvironmentTarget, ("", ""), False, ValueError), + (DockerEnvironmentTarget, ("", ""), False, ValueError), + ( + DockerEnvironmentTarget, + ("", "/home/derryn/pythons"), + False, + ValueError, + ), # Contains a banned special-string + (DockerEnvironmentTarget, ("",), False, ValueError), + (DockerEnvironmentTarget, ("",), False, ValueError), + (DockerEnvironmentTarget, ("",), False, ValueError), + (DockerEnvironmentTarget, ("",), False, ("",)), + ( + DockerEnvironmentTarget, + ("", "/home/derryn/pythons"), + False, + ("", "/home/derryn/pythons"), + ), + (RemoteEnvironmentTarget, ("", ""), True, ("",)), + (RemoteEnvironmentTarget, ("", ""), False, ValueError), + (RemoteEnvironmentTarget, ("", ""), False, ValueError), + ( + RemoteEnvironmentTarget, + ("", "/home/derryn/pythons"), + False, + ValueError, + ), # Contains a banned special-string + (RemoteEnvironmentTarget, ("",), False, ValueError), + (RemoteEnvironmentTarget, ("",), False, ValueError), + (RemoteEnvironmentTarget, ("",), False, ValueError), + (RemoteEnvironmentTarget, ("",), False, ("",)), + ( + RemoteEnvironmentTarget, + ("", "/home/derryn/pythons"), + False, + ("", "/home/derryn/pythons"), + ), + ), +) +def test_preprocessed_interpreter_search_paths( + env_tgt_type: type[LocalEnvironmentTarget] + | type[DockerEnvironmentTarget] + | type[RemoteEnvironmentTarget], + search_paths: Iterable[str], + is_default: bool, + expected: tuple[str] | type[ValueError], +): + + extra_kwargs: dict = {} + if env_tgt_type is DockerEnvironmentTarget: + extra_kwargs = { + DockerImageField.alias: "my_img", + } + + env_tgt = EnvironmentTarget(env_tgt_type(extra_kwargs, address=Address("flem"))) + if expected is not ValueError: + assert expected == _preprocessed_interpreter_search_paths(env_tgt, search_paths, is_default) + else: + with pytest.raises(ValueError): + _preprocessed_interpreter_search_paths(env_tgt, search_paths, is_default) From b9908c3c13789a02ee8c5bdb281456d14513df33 Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 28 Sep 2022 14:34:51 -0700 Subject: [PATCH 4/4] Address code review feedback # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- .../pants/core/subsystems/python_bootstrap.py | 20 +++++++++---------- src/python/pants/option/subsystem.py | 1 + 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/python/pants/core/subsystems/python_bootstrap.py b/src/python/pants/core/subsystems/python_bootstrap.py index ba15c6c22cf..40d736a3528 100644 --- a/src/python/pants/core/subsystems/python_bootstrap.py +++ b/src/python/pants/core/subsystems/python_bootstrap.py @@ -57,7 +57,7 @@ class EnvironmentAware(Subsystem.EnvironmentAware): * ``, the contents of the PATH env var - For only local runtime environments: + When the environment is a `local_environment` target: * ``, all Python versions currently configured by ASDF \ `(asdf shell, ${HOME}/.tool-versions)`, with a fallback to all installed versions @@ -232,8 +232,7 @@ def _preprocessed_interpreter_search_paths( * The search paths, with invalid tokens removed, if the provided value was unaltered from the default value in the options system (see `PythonBootstrapSubsystem.EnvironmentAware.search_paths`) - * The search paths unaltered, if the search paths do not contain tokens invalid for this - environment + * The search paths unaltered, if the search paths are all valid tokens for this environment If the environment is non-local and there are invalid tokens for those environments, raise `ValueError`. @@ -242,9 +241,7 @@ def _preprocessed_interpreter_search_paths( env = env_tgt.val search_paths = tuple(_search_paths) - if isinstance(env, LocalEnvironmentTarget): - return search_paths - if env is None: + if env is None or isinstance(env, LocalEnvironmentTarget): return search_paths not_allowed = {"", "", "", "", ""} @@ -259,10 +256,13 @@ def _preprocessed_interpreter_search_paths( if any_not_allowed: env_type = type(env) raise ValueError( - f"`[python-bootstrap].search_paths` is configured to use local Python discovery " - f"tools, which do not work in {env_type.__name__} runtime environments. To fix this, " - f"set the value of `python_bootstrap_search_path` in the {env.alias} defined at " - f"`{env.address}` to contain only hardcoded paths or the `` special string." + softwrap( + f"`[python-bootstrap].search_paths` is configured to use local Python discovery " + f"tools, which do not work in {env_type.__name__} runtime environments. To fix " + f"this, set the value of `python_bootstrap_search_path` in the `{env.alias}` " + f"defined at `{env.address}` to contain only hardcoded paths or the `` " + "special string." + ) ) return search_paths diff --git a/src/python/pants/option/subsystem.py b/src/python/pants/option/subsystem.py index 58800bfa536..abad434f9d9 100644 --- a/src/python/pants/option/subsystem.py +++ b/src/python/pants/option/subsystem.py @@ -125,6 +125,7 @@ def __getattribute__(self, __name: str) -> Any: return default def _is_default(self, __name: str) -> bool: + """Returns true if the value of the named option is unchanged from the default.""" from pants.core.util_rules.environments import resolve_environment_sensitive_option v = getattr(type(self), __name)