Skip to content

Commit

Permalink
Require explicit environment usage (#11641)
Browse files Browse the repository at this point in the history
### Problem

In order to allow concurrent runs of Pants in #11639 (part of #7654), all global mutable singletons must be either 1) thread-local, 2) replaced with explicitly passed values. `os.environ` and `sys.argv` are two of the last cases we're concerned with.

### Solution

Although we could hypothetically redefine `os.environ` and `os.getenv` as thread-local, they seem to be a better fit to be explicitly passed. So this change:
1. Renames (and moves) `PantsEnvironment` to `CompleteEnvironment`
2. Adds `Environment` and `EnvironmentRequest`, which filter the `CompleteEnvironment` to a smaller subset, and should be used more frequently by `@rules`.
3. Fixes a few cases of implicit `os.environ` usage in the `PythonSetup` subsystem by explicitly selecting interpreter-selection env vars (`HOME`/`PATH`/`PYENV_ROOT`)

Finally, we empty the environment for `pantsd` runs to enforce explicit usage of `CompleteEnvironment`/`Environment`. 

### Result

The last (?) global mutable variable blocking #7654 is removed. 

[ci skip-build-wheels]
  • Loading branch information
stuhood authored Mar 9, 2021
1 parent 96be55e commit 73e97c2
Show file tree
Hide file tree
Showing 50 changed files with 470 additions and 227 deletions.
6 changes: 6 additions & 0 deletions pants.toml
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,12 @@ pytest_plugins.add = [
]
timeout_default = 60

[test]
# TODO: These are exposed to tests in order to allow for python interpreter discovery when
# Pants-tests-Pants: in particular, the [python-setup] subsystem consumes them.
# see https://github.com/pantsbuild/pants/issues/11638
extra_env_vars = ["PYENV_ROOT", "HOME", "PATH"]

[coverage-py]
interpreter_constraints = [">=3.7,<3.9"]

Expand Down
3 changes: 2 additions & 1 deletion src/python/pants/backend/awslambda/python/rules_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ def create_python_awslambda(rule_runner: RuleRunner, addr: Address) -> Tuple[str
[
"--backend-packages=pants.backend.awslambda.python",
"--source-root-patterns=src/python",
]
],
env_inherit={"PATH", "PYENV_ROOT", "HOME"},
)
target = rule_runner.get_target(addr)
built_asset = rule_runner.request(BuiltPackage, [PythonAwsLambdaFieldSet.create(target)])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ def assert_files_generated(
]
if mypy:
options.append("--python-protobuf-mypy-plugin")
rule_runner.set_options(options)
rule_runner.set_options(
options,
env_inherit={"PATH", "PYENV_ROOT", "HOME"},
)
tgt = rule_runner.get_target(Address(spec))
protocol_sources = rule_runner.request(
HydratedSources, [HydrateSourcesRequest(tgt[ProtobufSources])]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ def assert_imports_parsed(
):
if content:
rule_runner.create_file(filename, content)
rule_runner.set_options([], env_inherit={"PATH", "PYENV_ROOT", "HOME"})
rule_runner.add_to_build_file("project", "python_library(sources=['**/*.py'])")
tgt = rule_runner.get_target(Address("project"))
imports = rule_runner.request(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def run_dep_inference(
args = ["--backend-packages=pants.backend.python", "--source-root-patterns=src/python"]
if enable_string_imports:
args.append("--python-infer-string-imports")
rule_runner.set_options(args)
rule_runner.set_options(args, env_inherit={"PATH", "PYENV_ROOT", "HOME"})
target = rule_runner.get_target(address)
return rule_runner.request(
InferredDependencies, [InferPythonImportDependencies(target[PythonSources])]
Expand Down Expand Up @@ -129,7 +129,8 @@ def test_infer_python_inits() -> None:
"--backend-packages=pants.backend.python",
"--python-infer-inits",
"--source-root-patterns=src/python",
]
],
env_inherit={"PATH", "PYENV_ROOT", "HOME"},
)

rule_runner.create_file("src/python/root/__init__.py")
Expand Down Expand Up @@ -168,7 +169,8 @@ def test_infer_python_conftests() -> None:
target_types=[PythonTests],
)
rule_runner.set_options(
["--backend-packages=pants.backend.python", "--source-root-patterns=src/python"]
["--backend-packages=pants.backend.python", "--source-root-patterns=src/python"],
env_inherit={"PATH", "PYENV_ROOT", "HOME"},
)

rule_runner.create_file("src/python/root/conftest.py")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def run_pytest(
args.append("--pytest-config=pytest.ini")
if force:
args.append("--test-force")
rule_runner.set_options(args, env=env)
rule_runner.set_options(args, env=env, env_inherit={"PATH", "PYENV_ROOT", "HOME"})

inputs = [PythonTestFieldSet.create(test_target)]
test_result = rule_runner.request(TestResult, inputs)
Expand Down
7 changes: 5 additions & 2 deletions src/python/pants/backend/python/goals/setup_py_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@


def create_setup_py_rule_runner(*, rules: Iterable) -> RuleRunner:
return RuleRunner(
rule_runner = RuleRunner(
rules=rules,
target_types=[
PexBinary,
Expand All @@ -68,6 +68,8 @@ def create_setup_py_rule_runner(*, rules: Iterable) -> RuleRunner:
],
objects={"setup_py": PythonArtifact},
)
rule_runner.set_options([], env_inherit={"PATH", "PYENV_ROOT", "HOME"})
return rule_runner


# We use a trivial test that our SetupKwargs plugin hook works.
Expand Down Expand Up @@ -501,7 +503,8 @@ def assert_requirements(
version_scheme: FirstPartyDependencyVersionScheme = FirstPartyDependencyVersionScheme.EXACT,
):
rule_runner.set_options(
[f"--setup-py-generation-first-party-dependency-version-scheme={version_scheme.value}"]
[f"--setup-py-generation-first-party-dependency-version-scheme={version_scheme.value}"],
env_inherit={"PATH", "PYENV_ROOT", "HOME"},
)
tgt = rule_runner.get_target(addr)
reqs = rule_runner.request(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def run_bandit(
args.append("--bandit-skip")
if additional_args:
args.extend(additional_args)
rule_runner.set_options(args)
rule_runner.set_options(args, env_inherit={"PATH", "PYENV_ROOT", "HOME"})
results = rule_runner.request(
LintResults,
[BanditRequest(BanditFieldSet.create(tgt) for tgt in targets)],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ def run_black(
args.append("--black-skip")
if version:
args.append(f"--black-version={version}")
rule_runner.set_options(args)
rule_runner.set_options(args, env_inherit={"PATH", "PYENV_ROOT", "HOME"})
field_sets = [BlackFieldSet.create(tgt) for tgt in targets]
lint_results = rule_runner.request(LintResults, [BlackRequest(field_sets)])
input_sources = rule_runner.request(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def run_docformatter(
args.append(f"--docformatter-args='{passthrough_args}'")
if skip:
args.append("--docformatter-skip")
rule_runner.set_options(args)
rule_runner.set_options(args, env_inherit={"PATH", "PYENV_ROOT", "HOME"})
field_sets = [DocformatterFieldSet.create(tgt) for tgt in targets]
lint_results = rule_runner.request(LintResults, [DocformatterRequest(field_sets)])
input_sources = rule_runner.request(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def run_flake8(
args.append("--flake8-skip")
if additional_args:
args.extend(additional_args)
rule_runner.set_options(args)
rule_runner.set_options(args, env_inherit={"PATH", "PYENV_ROOT", "HOME"})
results = rule_runner.request(
LintResults,
[
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def run_isort(
args.append(f"--isort-args='{passthrough_args}'")
if skip:
args.append("--isort-skip")
rule_runner.set_options(args)
rule_runner.set_options(args, env_inherit={"PATH", "PYENV_ROOT", "HOME"})
field_sets = [IsortFieldSet.create(tgt) for tgt in targets]
lint_results = rule_runner.request(LintResults, [IsortRequest(field_sets)])
input_sources = rule_runner.request(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ def run_pylint(
args.append("--pylint-skip")
if additional_args:
args.extend(additional_args)
rule_runner.set_options(args)
rule_runner.set_options(args, env_inherit={"PATH", "PYENV_ROOT", "HOME"})
results = rule_runner.request(
LintResults,
[PylintRequest(PylintFieldSet.create(tgt) for tgt in targets)],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ def run_black_and_isort(
[
"--backend-packages=['pants.backend.python.lint.black', 'pants.backend.python.lint.isort']",
*(extra_args or []),
]
],
env_inherit={"PATH", "PYENV_ROOT", "HOME"},
)
results = rule_runner.request(LanguageFmtResults, [targets])
return results
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ def run_mypy(
args.append("--mypy-skip")
if additional_args:
args.extend(additional_args)
rule_runner.set_options(args)
rule_runner.set_options(args, env_inherit={"PATH", "PYENV_ROOT", "HOME"})
result = rule_runner.request(
TypecheckResults,
[MyPyRequest(MyPyFieldSet.create(tgt) for tgt in targets)],
Expand Down
5 changes: 4 additions & 1 deletion src/python/pants/backend/python/util_rules/pex_cli_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ def test_custom_ca_certs(rule_runner: RuleRunner) -> None:
with temporary_dir() as tmpdir:
certs_file = Path(tmpdir) / "certsfile"
certs_file.write_text("Some fake cert")
rule_runner.set_options([f"--ca-certs-path={certs_file}"])
rule_runner.set_options(
[f"--ca-certs-path={certs_file}"],
env_inherit={"PATH", "PYENV_ROOT", "HOME"},
)
proc = rule_runner.request(
Process,
[PexCliProcess(argv=["some", "--args"], description="")],
Expand Down
20 changes: 13 additions & 7 deletions src/python/pants/backend/python/util_rules/pex_environment.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@
from pants.core.util_rules.subprocess_environment import SubprocessEnvironmentVars
from pants.engine import process
from pants.engine.engine_aware import EngineAwareReturnType
from pants.engine.environment import Environment, EnvironmentRequest
from pants.engine.process import BinaryPath, BinaryPathRequest, BinaryPaths, BinaryPathTest
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.option.global_options import GlobalOptions
from pants.option.subsystem import Subsystem
from pants.python.python_setup import PythonSetup
from pants.util.frozendict import FrozenDict
from pants.util.logging import LogLevel
from pants.util.memo import memoized_property
from pants.util.memo import memoized_method
from pants.util.ordered_set import OrderedSet
from pants.util.strutil import create_path_env_var

Expand Down Expand Up @@ -67,12 +68,12 @@ def register_options(cls, register):
),
)

@memoized_property
def path(self) -> Tuple[str, ...]:
@memoized_method
def path(self, env: Environment) -> Tuple[str, ...]:
def iter_path_entries():
for entry in self.options.executable_search_paths:
if entry == "<PATH>":
path = os.environ.get("PATH")
path = env.get("PATH")
if path:
for path_entry in path.split(os.pathsep):
yield path_entry
Expand Down Expand Up @@ -158,6 +159,9 @@ async def find_pex_python(
subprocess_env_vars: SubprocessEnvironmentVars,
global_options: GlobalOptions,
) -> PexEnvironment:
pex_relevant_environment = await Get(
Environment, EnvironmentRequest(["PATH", "HOME", "PYENV_ROOT"])
)
# PEX files are compatible with bootstrapping via Python 2.7 or Python 3.5+. The bootstrap
# code will then re-exec itself if the underlying PEX user code needs a more specific python
# interpreter. As such, we look for many Pythons usable by the PEX bootstrap code here for
Expand All @@ -166,7 +170,7 @@ async def find_pex_python(
Get(
BinaryPaths,
BinaryPathRequest(
search_path=python_setup.interpreter_search_paths,
search_path=python_setup.interpreter_search_paths(pex_relevant_environment),
binary_name=binary_name,
test=BinaryPathTest(
args=[
Expand Down Expand Up @@ -218,8 +222,10 @@ def first_python_binary() -> Optional[PythonExecutable]:
return None

return PexEnvironment(
path=pex_runtime_env.path,
interpreter_search_paths=tuple(python_setup.interpreter_search_paths),
path=pex_runtime_env.path(pex_relevant_environment),
interpreter_search_paths=tuple(
python_setup.interpreter_search_paths(pex_relevant_environment)
),
subprocess_environment_dict=subprocess_env_vars.vars,
# TODO: This path normalization is duplicated with `engine_initializer.py`. How can we do
# the normalization only once, via the options system?
Expand Down
5 changes: 4 additions & 1 deletion src/python/pants/backend/python/util_rules/pex_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,9 @@ def create_pex_and_get_all_data(
additional_args=additional_pex_args,
)
rule_runner.set_options(
["--backend-packages=pants.backend.python", *additional_pants_args], env=env
["--backend-packages=pants.backend.python", *additional_pants_args],
env=env,
env_inherit={"PATH", "PYENV_ROOT", "HOME"},
)
pex = rule_runner.request(pex_type, [request])
if isinstance(pex, Pex):
Expand Down Expand Up @@ -468,6 +470,7 @@ def test_pex_environment(rule_runner: RuleRunner, pex_type: type[Pex | VenvPex])
"--subprocess-environment-env-vars=LANG", # Value should come from environment.
"--subprocess-environment-env-vars=ftp_proxy=dummyproxy",
),
interpreter_constraints=PexInterpreterConstraints(["CPython>=3.6"]),
env={"LANG": "es_PY.UTF-8"},
)

Expand Down
26 changes: 13 additions & 13 deletions src/python/pants/bin/daemon_pants_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@

from pants.base.exiter import PANTS_FAILED_EXIT_CODE, ExitCode
from pants.bin.local_pants_runner import LocalPantsRunner
from pants.engine.environment import CompleteEnvironment
from pants.engine.internals.native import RawFdRunner
from pants.engine.internals.native_engine import PySessionCancellationLatch
from pants.init.logging import stdio_destination
from pants.option.options_bootstrapper import OptionsBootstrapper
from pants.pantsd.pants_daemon_core import PantsDaemonCore
from pants.util.contextutil import argv_as, hermetic_environment_as

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -97,7 +97,11 @@ def should_keep_polling(now):
)

def single_daemonized_run(
self, working_dir: str, cancellation_latch: PySessionCancellationLatch
self,
args: Tuple[str, ...],
env: Dict[str, str],
working_dir: str,
cancellation_latch: PySessionCancellationLatch,
) -> ExitCode:
"""Run a single daemonized run of Pants.
Expand All @@ -110,22 +114,18 @@ def single_daemonized_run(
logger.debug("Connected to pantsd")
# Capture the client's start time, which we propagate here in order to get an accurate
# view of total time.
env_start_time = os.environ.get("PANTSD_RUNTRACKER_CLIENT_START_TIME", None)
env_start_time = env.get("PANTSD_RUNTRACKER_CLIENT_START_TIME", None)
start_time = float(env_start_time) if env_start_time else time.time()

# Clear global mutable state before entering `LocalPantsRunner`. Note that we use
# `sys.argv` and `os.environ`, since they have been mutated to maintain the illusion
# of a local run: once we allow for concurrent runs, this information should be
# propagated down from the caller.
# see https://github.com/pantsbuild/pants/issues/7654
options_bootstrapper = OptionsBootstrapper.create(
env=os.environ, args=sys.argv, allow_pantsrc=True
env=env, args=args, allow_pantsrc=True
)

# Run using the pre-warmed Session.
scheduler, options_initializer = self._core.prepare(options_bootstrapper)
complete_env = CompleteEnvironment(env)
scheduler, options_initializer = self._core.prepare(options_bootstrapper, complete_env)
runner = LocalPantsRunner.create(
os.environ,
complete_env,
options_bootstrapper,
scheduler=scheduler,
options_initializer=options_initializer,
Expand Down Expand Up @@ -165,9 +165,9 @@ def __call__(
stdin_fileno=stdin_fileno,
stdout_fileno=stdout_fileno,
stderr_fileno=stderr_fileno,
), hermetic_environment_as(**env), argv_as((command,) + args):
):
return self.single_daemonized_run(
working_directory.decode(), cancellation_latch
((command,) + args), env, working_directory.decode(), cancellation_latch
)
finally:
logger.info(f"request completed: `{' '.join(args)}`")
Loading

0 comments on commit 73e97c2

Please sign in to comment.