From 530bd42ee09839eec3b19daf3854b7a951844f41 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Fri, 8 Jul 2022 12:06:33 -0700 Subject: [PATCH] Remove the restriction on InteractiveProcess inputs, and use native chroots. [ci skip-rust] [ci skip-build-wheels] --- src/python/pants/backend/scala/goals/repl.py | 15 ++---- src/python/pants/core/goals/repl.py | 52 ++++++++----------- src/python/pants/core/goals/run.py | 54 ++++++++------------ src/python/pants/engine/process.py | 15 ------ src/python/pants/engine/process_test.py | 27 ++++------ src/python/pants/jvm/jdk_rules.py | 13 +++-- 6 files changed, 66 insertions(+), 110 deletions(-) diff --git a/src/python/pants/backend/scala/goals/repl.py b/src/python/pants/backend/scala/goals/repl.py index 6ffebe8d0cad..3c10604871aa 100644 --- a/src/python/pants/backend/scala/goals/repl.py +++ b/src/python/pants/backend/scala/goals/repl.py @@ -1,5 +1,6 @@ # Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). + from __future__ import annotations from pants.backend.scala.subsystems.scala import ScalaSubsystem @@ -66,32 +67,26 @@ async def create_scala_repl_request( Get(Digest, AddPrefix(d, user_classpath_prefix)) for d in user_classpath.digests() ) - # TODO: Manually merging the `immutable_input_digests` since InteractiveProcess doesn't - # support them yet. See https://github.com/pantsbuild/pants/issues/13852. - jdk_digests = await MultiGet( - Get(Digest, AddPrefix(digest, relpath)) - for relpath, digest in jdk.immutable_input_digests.items() - ) - repl_digest = await Get( Digest, - MergeDigests([*prefixed_user_classpath, tool_classpath.content.digest, *jdk_digests]), + MergeDigests([*prefixed_user_classpath, tool_classpath.content.digest]), ) return ReplRequest( digest=repl_digest, args=[ - *jdk.args(bash, tool_classpath.classpath_entries()), + *jdk.args(bash, tool_classpath.classpath_entries(), chroot="{chroot}"), "-Dscala.usejavacp=true", "scala.tools.nsc.MainGenericRunner", "-classpath", ":".join(user_classpath.args(prefix=user_classpath_prefix)), ], + run_in_workspace=False, extra_env={ **jdk.env, "PANTS_INTERNAL_ABSOLUTE_PREFIX": "", }, - run_in_workspace=False, + immutable_input_digests=jdk.immutable_input_digests, append_only_caches=jdk.append_only_caches, ) diff --git a/src/python/pants/core/goals/repl.py b/src/python/pants/core/goals/repl.py index ded7ebf9fcd1..6b04b62ffda0 100644 --- a/src/python/pants/core/goals/repl.py +++ b/src/python/pants/core/goals/repl.py @@ -5,7 +5,6 @@ import os from abc import ABC from dataclasses import dataclass -from pathlib import PurePath from typing import ClassVar, Iterable, Mapping, Optional, Sequence, Tuple from pants.base.build_root import BuildRoot @@ -14,14 +13,12 @@ from pants.engine.environment import CompleteEnvironment from pants.engine.fs import Digest, Workspace from pants.engine.goal import Goal, GoalSubsystem -from pants.engine.internals.native_engine import EMPTY_DIGEST from pants.engine.process import InteractiveProcess, InteractiveProcessResult from pants.engine.rules import Effect, Get, collect_rules, goal_rule from pants.engine.target import FilteredTargets, Target from pants.engine.unions import UnionMembership, union from pants.option.global_options import GlobalOptions from pants.option.option_types import BoolOption, StrOption -from pants.util.contextutil import temporary_dir from pants.util.frozendict import FrozenDict from pants.util.memo import memoized_property from pants.util.meta import frozen_after_init @@ -38,10 +35,9 @@ class ReplImplementation(ABC): name: ClassVar[str] targets: Sequence[Target] - chroot: str # Absolute path of the chroot the sources will be materialized to. def in_chroot(self, relpath: str) -> str: - return os.path.join(self.chroot, relpath) + return os.path.join("{chroot}", relpath) @memoized_property def addresses(self) -> Addresses: @@ -76,6 +72,7 @@ class ReplRequest: digest: Digest args: Tuple[str, ...] extra_env: FrozenDict[str, str] + immutable_input_digests: FrozenDict[str, Digest] append_only_caches: FrozenDict[str, str] run_in_workspace: bool @@ -85,12 +82,14 @@ def __init__( digest: Digest, args: Iterable[str], extra_env: Optional[Mapping[str, str]] = None, + immutable_input_digests: Mapping[str, Digest] | None = None, append_only_caches: Mapping[str, str] | None = None, run_in_workspace: bool = True, ) -> None: self.digest = digest self.args = tuple(args) self.extra_env = FrozenDict(extra_env or {}) + self.immutable_input_digests = FrozenDict(immutable_input_digests or {}) self.append_only_caches = FrozenDict(append_only_caches or {}) self.run_in_workspace = run_in_workspace @@ -119,33 +118,22 @@ async def run_repl( ) return Repl(-1) - with temporary_dir(root_dir=global_options.pants_workdir, cleanup=False) as tmpdir: - repl_impl = repl_implementation_cls(targets=specified_targets, chroot=tmpdir) - request = await Get(ReplRequest, ReplImplementation, repl_impl) - - input_digest = request.digest - if request.run_in_workspace: - workspace.write_digest( - request.digest, - path_prefix=PurePath(tmpdir).relative_to(build_root.path).as_posix(), - # We don't want to influence whether the InteractiveProcess is able to restart. Because - # we're writing into a temp directory, we can safely mark this side_effecting=False. - side_effecting=False, - ) - input_digest = EMPTY_DIGEST - - env = {**complete_env, **request.extra_env} - result = await Effect( - InteractiveProcessResult, - InteractiveProcess( - argv=request.args, - env=env, - input_digest=input_digest, - run_in_workspace=request.run_in_workspace, - restartable=repl_subsystem.restartable, - append_only_caches=request.append_only_caches, - ), - ) + repl_impl = repl_implementation_cls(targets=specified_targets) + request = await Get(ReplRequest, ReplImplementation, repl_impl) + + env = {**complete_env, **request.extra_env} + result = await Effect( + InteractiveProcessResult, + InteractiveProcess( + argv=request.args, + env=env, + input_digest=request.digest, + run_in_workspace=request.run_in_workspace, + restartable=repl_subsystem.restartable, + immutable_input_digests=request.immutable_input_digests, + append_only_caches=request.append_only_caches, + ), + ) return Repl(result.exit_code) diff --git a/src/python/pants/core/goals/run.py b/src/python/pants/core/goals/run.py index d0cffc9c9e38..7f0b65d05e73 100644 --- a/src/python/pants/core/goals/run.py +++ b/src/python/pants/core/goals/run.py @@ -3,7 +3,6 @@ import logging from abc import ABCMeta from dataclasses import dataclass -from pathlib import PurePath from typing import Iterable, Mapping, Optional, Tuple from pants.base.build_root import BuildRoot @@ -25,7 +24,6 @@ from pants.engine.unions import UnionMembership, union from pants.option.global_options import GlobalOptions from pants.option.option_types import ArgsListOption, BoolOption -from pants.util.contextutil import temporary_dir from pants.util.frozendict import FrozenDict from pants.util.meta import frozen_after_init from pants.util.strutil import softwrap @@ -164,41 +162,29 @@ async def run( # Cleanup is the default, so we want to preserve the chroot if either option is off. cleanup = run_subsystem.cleanup and global_options.process_cleanup - with temporary_dir(root_dir=global_options.pants_workdir, cleanup=cleanup) as tmpdir: - if not cleanup: - logger.info(f"Preserving running binary chroot {tmpdir}") - workspace.write_digest( - request.digest, - path_prefix=PurePath(tmpdir).relative_to(build_root.path).as_posix(), - # We don't want to influence whether the InteractiveProcess is able to restart. Because - # we're writing into a temp directory, we can safely mark this side_effecting=False. - side_effecting=False, - ) - - args = (arg.format(chroot=tmpdir) for arg in request.args) - env = {**complete_env, **{k: v.format(chroot=tmpdir) for k, v in request.extra_env.items()}} - if run_subsystem.debug_adapter: - logger.info( - softwrap( - f""" - Launching debug adapter at '{debug_adapter.host}:{debug_adapter.port}', - which will wait for a client connection... - """ - ) + if run_subsystem.debug_adapter: + logger.info( + softwrap( + f""" + Launching debug adapter at '{debug_adapter.host}:{debug_adapter.port}', + which will wait for a client connection... + """ ) - - result = await Effect( - InteractiveProcessResult, - InteractiveProcess( - argv=(*args, *run_subsystem.args), - env=env, - run_in_workspace=True, - restartable=restartable, - ), ) - exit_code = result.exit_code - return Run(exit_code) + result = await Effect( + InteractiveProcessResult, + InteractiveProcess( + argv=(*request.args, *run_subsystem.args), + env={**complete_env, **request.extra_env}, + input_digest=request.digest, + run_in_workspace=True, + restartable=restartable, + cleanup=cleanup, + ), + ) + + return Run(result.exit_code) def rules(): diff --git a/src/python/pants/engine/process.py b/src/python/pants/engine/process.py index 70941a093737..6ba14e8e11c6 100644 --- a/src/python/pants/engine/process.py +++ b/src/python/pants/engine/process.py @@ -331,21 +331,6 @@ def __init__( self.restartable = restartable self.cleanup = cleanup - self.__post_init__() - - def __post_init__(self): - if self.process.input_digest != EMPTY_DIGEST and self.run_in_workspace: - raise ValueError( - "InteractiveProcess should use the Workspace API to materialize any needed " - "files when it runs in the workspace" - ) - if self.process.append_only_caches and self.run_in_workspace: - raise ValueError( - "InteractiveProcess requested setup of append-only caches and also requested to run" - " in the workspace. These options are incompatible since setting up append-only" - " caches would modify the workspace." - ) - @classmethod def from_process( cls, diff --git a/src/python/pants/engine/process_test.py b/src/python/pants/engine/process_test.py index efa841dab8cc..be420b2aca79 100644 --- a/src/python/pants/engine/process_test.py +++ b/src/python/pants/engine/process_test.py @@ -252,32 +252,27 @@ def test_create_files(rule_runner: RuleRunner) -> None: assert result.stdout == b"hellogoodbye" -def test_interactive_process_cannot_have_input_files_and_workspace() -> None: - mock_digest = Digest(EMPTY_DIGEST.fingerprint, 1) - with pytest.raises(ValueError): - InteractiveProcess(argv=["/bin/echo"], input_digest=mock_digest, run_in_workspace=True) - - -def test_interactive_process_cannot_have_append_only_caches_and_workspace() -> None: - with pytest.raises(ValueError): - InteractiveProcess( - argv=["/bin/echo"], append_only_caches={"foo": "bar"}, run_in_workspace=True - ) - - -def test_interactive_process_immutable_input_digests(rule_runner: RuleRunner) -> None: +@pytest.mark.parametrize("run_in_workspace", [True, False]) +def test_interactive_process_inputs(rule_runner: RuleRunner, run_in_workspace: bool) -> None: digest0 = rule_runner.request(Digest, [CreateDigest([FileContent("file0", b"")])]) digest1 = rule_runner.request(Digest, [CreateDigest([FileContent("file1", b"")])]) digest2 = rule_runner.request( Digest, [CreateDigest([FileContent("file2", b""), FileContent("file3", b"")])] ) process = InteractiveProcess( - argv=["/bin/bash", "-c", "ls -1"], + argv=["/bin/bash", "-c", "ls -1 '{chroot}'"], env={"BAZ": "QUX"}, input_digest=digest0, immutable_input_digests={"prefix1": digest1, "prefix2": digest2}, + append_only_caches={"cache_name": "append_only0"}, + run_in_workspace=run_in_workspace, ) with mock_console(rule_runner.options_bootstrapper) as (_, stdio_reader): result = rule_runner.run_interactive_process(process) assert result.exit_code == 0 - assert set(stdio_reader.get_stdout().splitlines()) == {"file0", "prefix1", "prefix2"} + assert set(stdio_reader.get_stdout().splitlines()) == { + "append_only0", + "file0", + "prefix1", + "prefix2", + } diff --git a/src/python/pants/jvm/jdk_rules.py b/src/python/pants/jvm/jdk_rules.py index 1776e86c2725..b451b007db1e 100644 --- a/src/python/pants/jvm/jdk_rules.py +++ b/src/python/pants/jvm/jdk_rules.py @@ -101,13 +101,20 @@ class JdkEnvironment: jdk_preparation_script: ClassVar[str] = f"{bin_dir}/jdk.sh" java_home: ClassVar[str] = "__java_home" - def args(self, bash: BashBinary, classpath_entries: Iterable[str]) -> tuple[str, ...]: + def args( + self, bash: BashBinary, classpath_entries: Iterable[str], chroot: str | None = None + ) -> tuple[str, ...]: + def in_chroot(path: str) -> str: + if not chroot: + return path + return os.path.join(chroot, path) + return ( bash.path, - self.jdk_preparation_script, + in_chroot(self.jdk_preparation_script), f"{self.java_home}/bin/java", "-cp", - ":".join([self.nailgun_jar, *classpath_entries]), + ":".join([in_chroot(self.nailgun_jar), *classpath_entries]), ) @property