From 8c910d1d7c52a71d09b2e9265ef9aeeec7464443 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Thu, 4 Aug 2022 16:53:35 -0400 Subject: [PATCH] Replace `--[no-]process-cleanup` with `--keep-sandboxes={always,never,on_failure}` (#16415) For the purposes of debugging an intermittent issue (such as a crash which occurs only in certain conditions in CI), `--no-process-cleanup` is too big of a hammer. To support capturing the sandboxes for only failing processes, this change replaces the `--[no-]process-cleanup` option with `--keep-sandboxes={always,never,on_failure}`. [ci skip-rust] [ci skip-build-wheels] --- docs/markdown/Using Pants/troubleshooting.md | 8 ++- .../rules-api/rules-api-process.md | 2 +- .../rules-api/rules-api-tips.md | 4 +- .../backend/docker/goals/package_image.py | 6 +- .../docker/goals/package_image_test.py | 4 +- .../java/dependency_inference/java_parser.py | 6 +- .../dependency_inference/kotlin_parser.py | 6 +- .../pants/backend/python/goals/coverage_py.py | 6 +- .../dependency_inference/scala_parser.py | 6 +- src/python/pants/core/goals/run.py | 20 ++++-- src/python/pants/core/goals/run_test.py | 6 +- .../pants/engine/internals/options_parsing.py | 16 ++++- .../pants/engine/internals/scheduler.py | 2 +- src/python/pants/engine/process.py | 31 ++++++--- src/python/pants/jvm/strip_jar/strip_jar.py | 17 +---- src/python/pants/option/global_options.py | 56 ++++++++++++++-- src/python/pants/option/option_types.py | 18 +++++ src/python/pants/testutil/rule_runner.py | 2 +- .../engine/process_execution/src/local.rs | 67 ++++++++++++------- src/rust/engine/src/context.rs | 4 +- src/rust/engine/src/externs/interface.rs | 5 +- src/rust/engine/src/intrinsics.rs | 20 ++++-- 22 files changed, 215 insertions(+), 97 deletions(-) diff --git a/docs/markdown/Using Pants/troubleshooting.md b/docs/markdown/Using Pants/troubleshooting.md index fb117bf6895..91553ca91b9 100644 --- a/docs/markdown/Using Pants/troubleshooting.md +++ b/docs/markdown/Using Pants/troubleshooting.md @@ -27,20 +27,22 @@ Setting the option `--pex-verbosity=9` can help debug exceptions that occur when Once you have this stack trace, we recommend copying it into Pastebin or a GitHub Gist, then opening a GitHub issue or posting on Slack. Someone from the Pants team would be happy to help. See [Getting Help](doc:getting-help). -Debug tip: inspect the sandbox with `--no-process-cleanup` +Debug tip: inspect the sandbox with `--keep-sandboxes` ---------------------------------------------------------- Pants runs most processes in a hermetic sandbox (temporary directory), which allows for safely caching and running multiple processes in parallel. -Use the option `--no-process-cleanup` for Pants to log the paths to these sandboxes, and to keep them around after the run. You can then inspect them to check if the files you are expecting are present. +Use the option `--keep-sandboxes=always` for Pants to log the paths to these sandboxes, and to keep them around after the run. You can then inspect them to check if the files you are expecting are present. ```bash -./pants --no-process-cleanup lint src/project/app.py +./pants --keep-sandboxes=always lint src/project/app.py ... 21:26:13.55 [INFO] preserving local process execution dir `"/private/var/folders/hm/qjjq4w3n0fsb07kp5bxbn8rw0000gn/T/process-executionQgIOjb"` for "Run isort on 1 file." ... ``` +You can also pass `--keep-sandboxes=on_failure`, to preserve only the sandboxes of failing processes. + There is even a `__run.sh` script in the directory that will run the process using the same argv and environment that Pants would use. Cache or pantsd invalidation issues diff --git a/docs/markdown/Writing Plugins/rules-api/rules-api-process.md b/docs/markdown/Writing Plugins/rules-api/rules-api-process.md index d275f1d0f95..b1afa13a0e0 100644 --- a/docs/markdown/Writing Plugins/rules-api/rules-api-process.md +++ b/docs/markdown/Writing Plugins/rules-api/rules-api-process.md @@ -38,7 +38,7 @@ The process will run in a temporary directory and is hermetic, meaning that it c > 📘 Debugging a `Process` > -> Setting the [`--no-process-cleanup`](docs:rules-api-tips#debugging-look-inside-the-chroot) flag will cause the sandboxes of `Process`es to be preserved and logged to the console for inspection. +> Setting the [`--keep-sandboxes=always`](docs:rules-api-tips#debugging-look-inside-the-chroot) flag will cause the sandboxes of `Process`es to be preserved and logged to the console for inspection. > > It can be very helpful while editing `Process` definitions! diff --git a/docs/markdown/Writing Plugins/rules-api/rules-api-tips.md b/docs/markdown/Writing Plugins/rules-api/rules-api-tips.md index a6d39b2cc04..c9b1e49c1f8 100644 --- a/docs/markdown/Writing Plugins/rules-api/rules-api-tips.md +++ b/docs/markdown/Writing Plugins/rules-api/rules-api-tips.md @@ -61,12 +61,12 @@ Pants will also memoize in-memory the evaluation of all `@rule`s. This means tha Debugging: Look inside the chroot --------------------------------- -When Pants runs most processes, it runs in a `chroot` (temporary directory). Usually, this gets cleaned up after the `Process` finishes. You can instead run `./pants --no-process-cleanup`, which will keep around the folder. +When Pants runs most processes, it runs in a `chroot` (temporary directory). Usually, this gets cleaned up after the `Process` finishes. You can instead pass `--keep-sandboxes=always` to keep those directories for all processes, or `--keep-sandboxes=on_failure` to keep those directories for only processes which have failed. Pants will log the path to the chroot, e.g.: ``` -▶ ./pants --no-process-cleanup test src/python/pants/util/strutil_test.py +▶ ./pants --keep-sandboxes=always test src/python/pants/util/strutil_test.py ... 12:29:45.08 [INFO] preserving local process execution dir `"/private/var/folders/sx/pdpbqz4x5cscn9hhfpbsbqvm0000gn/T/process-executionN9Kdk0"` for "Test binary /Users/pantsbuild/.pyenv/shims/python3." ... diff --git a/src/python/pants/backend/docker/goals/package_image.py b/src/python/pants/backend/docker/goals/package_image.py index dbc9ca0ee31..6c615be5edb 100644 --- a/src/python/pants/backend/docker/goals/package_image.py +++ b/src/python/pants/backend/docker/goals/package_image.py @@ -40,7 +40,7 @@ from pants.engine.rules import Get, MultiGet, collect_rules, rule from pants.engine.target import Target, WrappedTarget, WrappedTargetRequest from pants.engine.unions import UnionRule -from pants.option.global_options import GlobalOptions, ProcessCleanupOption +from pants.option.global_options import GlobalOptions, KeepSandboxes from pants.util.strutil import bullet_list logger = logging.getLogger(__name__) @@ -233,7 +233,7 @@ async def build_docker_image( options: DockerOptions, global_options: GlobalOptions, docker: DockerBinary, - process_cleanup: ProcessCleanupOption, + keep_sandboxes: KeepSandboxes, ) -> BuiltPackage: """Build a Docker image using `docker build`.""" context, wrapped_target = await MultiGet( @@ -290,7 +290,7 @@ async def build_docker_image( result.stdout, result.stderr, process.description, - process_cleanup=process_cleanup.val, + keep_sandboxes=keep_sandboxes, ) image_id = parse_image_id_from_docker_build_output(result.stdout, result.stderr) diff --git a/src/python/pants/backend/docker/goals/package_image_test.py b/src/python/pants/backend/docker/goals/package_image_test.py index e9a0f997c08..53fead81b7d 100644 --- a/src/python/pants/backend/docker/goals/package_image_test.py +++ b/src/python/pants/backend/docker/goals/package_image_test.py @@ -53,7 +53,7 @@ ProcessResultMetadata, ) from pants.engine.target import InvalidFieldException, WrappedTarget, WrappedTargetRequest -from pants.option.global_options import GlobalOptions, ProcessCleanupOption +from pants.option.global_options import GlobalOptions, KeepSandboxes from pants.testutil.option_util import create_subsystem from pants.testutil.pytest_util import assert_logged, no_exception from pants.testutil.rule_runner import MockGet, QueryRule, RuleRunner, run_rule_with_mocks @@ -146,7 +146,7 @@ def run_process_mock(process: Process) -> FallibleProcessResult: docker_options, global_options, DockerBinary("/dummy/docker"), - ProcessCleanupOption(True), + KeepSandboxes.never, ], mock_gets=[ MockGet( diff --git a/src/python/pants/backend/java/dependency_inference/java_parser.py b/src/python/pants/backend/java/dependency_inference/java_parser.py index fadd1ec01ea..c68916bfb0e 100644 --- a/src/python/pants/backend/java/dependency_inference/java_parser.py +++ b/src/python/pants/backend/java/dependency_inference/java_parser.py @@ -21,7 +21,7 @@ from pants.jvm.jdk_rules import InternalJdk, JvmProcess from pants.jvm.resolve.coursier_fetch import ToolClasspath, ToolClasspathRequest from pants.jvm.resolve.jvm_tool import GenerateJvmLockfileFromTool -from pants.option.global_options import ProcessCleanupOption +from pants.option.global_options import KeepSandboxes from pants.util.logging import LogLevel from pants.util.ordered_set import FrozenOrderedSet @@ -53,7 +53,7 @@ class JavaParserCompiledClassfiles: @rule(level=LogLevel.DEBUG) async def resolve_fallible_result_to_analysis( fallible_result: FallibleJavaSourceDependencyAnalysisResult, - process_cleanup: ProcessCleanupOption, + keep_sandboxes: KeepSandboxes, ) -> JavaSourceDependencyAnalysis: # TODO(#12725): Just convert directly to a ProcessResult like this: # result = await Get(ProcessResult, FallibleProcessResult, fallible_result.process_result) @@ -68,7 +68,7 @@ async def resolve_fallible_result_to_analysis( fallible_result.process_result.stdout, fallible_result.process_result.stderr, "Java source dependency analysis failed.", - process_cleanup=process_cleanup.val, + keep_sandboxes=keep_sandboxes, ) diff --git a/src/python/pants/backend/kotlin/dependency_inference/kotlin_parser.py b/src/python/pants/backend/kotlin/dependency_inference/kotlin_parser.py index b7925c3cdc5..a86270654e2 100644 --- a/src/python/pants/backend/kotlin/dependency_inference/kotlin_parser.py +++ b/src/python/pants/backend/kotlin/dependency_inference/kotlin_parser.py @@ -20,7 +20,7 @@ from pants.jvm.resolve.common import ArtifactRequirements, Coordinate from pants.jvm.resolve.coursier_fetch import ToolClasspath, ToolClasspathRequest from pants.jvm.resolve.jvm_tool import GenerateJvmLockfileFromTool -from pants.option.global_options import ProcessCleanupOption +from pants.option.global_options import KeepSandboxes from pants.util.frozendict import FrozenDict from pants.util.logging import LogLevel from pants.util.ordered_set import FrozenOrderedSet @@ -207,7 +207,7 @@ async def analyze_kotlin_source_dependencies( @rule(level=LogLevel.DEBUG) async def resolve_fallible_result_to_analysis( fallible_result: FallibleKotlinSourceDependencyAnalysisResult, - process_cleanup: ProcessCleanupOption, + keep_sandboxes: KeepSandboxes, ) -> KotlinSourceDependencyAnalysis: # TODO(#12725): Just convert directly to a ProcessResult like this: # result = await Get(ProcessResult, FallibleProcessResult, fallible_result.process_result) @@ -222,7 +222,7 @@ async def resolve_fallible_result_to_analysis( fallible_result.process_result.stdout, fallible_result.process_result.stderr, "Kotlin source dependency analysis failed.", - process_cleanup=process_cleanup.val, + keep_sandboxes=keep_sandboxes, ) diff --git a/src/python/pants/backend/python/goals/coverage_py.py b/src/python/pants/backend/python/goals/coverage_py.py index 4aebc2c2fa5..179ff4ed279 100644 --- a/src/python/pants/backend/python/goals/coverage_py.py +++ b/src/python/pants/backend/python/goals/coverage_py.py @@ -49,7 +49,7 @@ from pants.engine.rules import Get, MultiGet, collect_rules, rule from pants.engine.target import TransitiveTargets, TransitiveTargetsRequest from pants.engine.unions import UnionRule -from pants.option.global_options import ProcessCleanupOption +from pants.option.global_options import KeepSandboxes from pants.option.option_types import ( BoolOption, EnumListOption, @@ -487,7 +487,7 @@ async def generate_coverage_reports( coverage_setup: CoverageSetup, coverage_config: CoverageConfig, coverage_subsystem: CoverageSubsystem, - process_cleanup: ProcessCleanupOption, + keep_sandboxes: KeepSandboxes, distdir: DistDir, ) -> CoverageReports: """Takes all Python test results and generates a single coverage report.""" @@ -566,7 +566,7 @@ async def generate_coverage_reports( res.stdout, res.stderr, proc.description, - process_cleanup=process_cleanup.val, + keep_sandboxes=keep_sandboxes, ) # In practice if one result triggers --fail-under, they all will, but no need to rely on that. diff --git a/src/python/pants/backend/scala/dependency_inference/scala_parser.py b/src/python/pants/backend/scala/dependency_inference/scala_parser.py index c415d678034..41f1c0bf923 100644 --- a/src/python/pants/backend/scala/dependency_inference/scala_parser.py +++ b/src/python/pants/backend/scala/dependency_inference/scala_parser.py @@ -34,7 +34,7 @@ from pants.jvm.resolve.jvm_tool import GenerateJvmLockfileFromTool from pants.jvm.subsystems import JvmSubsystem from pants.jvm.target_types import JvmResolveField -from pants.option.global_options import ProcessCleanupOption +from pants.option.global_options import KeepSandboxes from pants.util.frozendict import FrozenDict from pants.util.logging import LogLevel from pants.util.ordered_set import FrozenOrderedSet @@ -292,7 +292,7 @@ async def analyze_scala_source_dependencies( @rule(level=LogLevel.DEBUG) async def resolve_fallible_result_to_analysis( fallible_result: FallibleScalaSourceDependencyAnalysisResult, - process_cleanup: ProcessCleanupOption, + keep_sandboxes: KeepSandboxes, ) -> ScalaSourceDependencyAnalysis: # TODO(#12725): Just convert directly to a ProcessResult like this: # result = await Get(ProcessResult, FallibleProcessResult, fallible_result.process_result) @@ -307,7 +307,7 @@ async def resolve_fallible_result_to_analysis( fallible_result.process_result.stdout, fallible_result.process_result.stderr, "Scala source dependency analysis failed.", - process_cleanup=process_cleanup.val, + keep_sandboxes=keep_sandboxes, ) diff --git a/src/python/pants/core/goals/run.py b/src/python/pants/core/goals/run.py index dcfb5c77b7d..44f6ba0eee8 100644 --- a/src/python/pants/core/goals/run.py +++ b/src/python/pants/core/goals/run.py @@ -25,7 +25,7 @@ WrappedTargetRequest, ) from pants.engine.unions import UnionMembership, union -from pants.option.global_options import GlobalOptions +from pants.option.global_options import GlobalOptions, KeepSandboxes from pants.option.option_types import ArgsListOption, BoolOption from pants.util.frozendict import FrozenDict from pants.util.meta import frozen_after_init @@ -110,14 +110,17 @@ def activated(cls, union_membership: UnionMembership) -> bool: ) cleanup = BoolOption( default=True, + deprecation_start_version="2.15.0.dev1", + removal_version="2.16.0.dev1", + removal_hint="Use the global `keep_sandboxes` option instead.", help=softwrap( """ Whether to clean up the temporary directory in which the binary is chrooted. Set this to false to retain the directory, e.g., for debugging. - Note that setting the global --process-cleanup option to false will also conserve - this directory, along with those of all other processes that Pants executes. - This option is more selective and controls just the target binary's directory. + Note that setting the global --keep-sandboxes option may also conserve this directory, + along with those of all other processes that Pants executes. This option is more + selective and controls just the target binary's directory. """ ), ) @@ -168,8 +171,11 @@ async def run( WrappedTarget, WrappedTargetRequest(field_set.address, description_of_origin="") ) restartable = wrapped_target.target.get(RestartableField).value - # 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 + keep_sandboxes = ( + global_options.keep_sandboxes + if run_subsystem.options.is_default("cleanup") + else (KeepSandboxes.never if run_subsystem.cleanup else KeepSandboxes.always) + ) if run_subsystem.debug_adapter: logger.info( @@ -189,7 +195,7 @@ async def run( input_digest=request.digest, run_in_workspace=True, restartable=restartable, - cleanup=cleanup, + keep_sandboxes=keep_sandboxes, immutable_input_digests=request.immutable_input_digests, append_only_caches=request.append_only_caches, ), diff --git a/src/python/pants/core/goals/run_test.py b/src/python/pants/core/goals/run_test.py index 9e85c83071b..56afafe00d0 100644 --- a/src/python/pants/core/goals/run_test.py +++ b/src/python/pants/core/goals/run_test.py @@ -27,7 +27,7 @@ WrappedTarget, WrappedTargetRequest, ) -from pants.option.global_options import GlobalOptions +from pants.option.global_options import GlobalOptions, KeepSandboxes from pants.testutil.option_util import create_goal_subsystem, create_subsystem from pants.testutil.rule_runner import ( MockEffect, @@ -86,7 +86,9 @@ class TestBinaryTarget(Target): port="5678", ), create_subsystem( - GlobalOptions, pants_workdir=rule_runner.pants_workdir, process_cleanup=True + GlobalOptions, + pants_workdir=rule_runner.pants_workdir, + keep_sandboxes=KeepSandboxes.never, ), workspace, BuildRoot(), diff --git a/src/python/pants/engine/internals/options_parsing.py b/src/python/pants/engine/internals/options_parsing.py index b580cc7b23c..4d29f2f1687 100644 --- a/src/python/pants/engine/internals/options_parsing.py +++ b/src/python/pants/engine/internals/options_parsing.py @@ -3,11 +3,13 @@ from dataclasses import dataclass +from pants.base.deprecated import warn_or_error from pants.build_graph.build_configuration import BuildConfiguration from pants.engine.internals.session import SessionValues from pants.engine.rules import collect_rules, rule from pants.option.global_options import ( GlobalOptions, + KeepSandboxes, NamedCachesDirOption, ProcessCleanupOption, UseDeprecatedPexBinaryRunSemanticsOption, @@ -59,8 +61,18 @@ def log_level(global_options: GlobalOptions) -> LogLevel: @rule -def extract_process_cleanup_option(global_options: GlobalOptions) -> ProcessCleanupOption: - return ProcessCleanupOption(global_options.process_cleanup) +def extract_process_cleanup_option(keep_sandboxes: KeepSandboxes) -> ProcessCleanupOption: + warn_or_error( + removal_version="2.15.0.dev1", + entity="ProcessCleanupOption", + hint="Instead, use `KeepSandboxes`.", + ) + return ProcessCleanupOption(keep_sandboxes == KeepSandboxes.never) + + +@rule +def extract_keep_sandboxes(global_options: GlobalOptions) -> KeepSandboxes: + return GlobalOptions.resolve_keep_sandboxes(global_options.options) @rule diff --git a/src/python/pants/engine/internals/scheduler.py b/src/python/pants/engine/internals/scheduler.py index e1315812e94..702961b15d6 100644 --- a/src/python/pants/engine/internals/scheduler.py +++ b/src/python/pants/engine/internals/scheduler.py @@ -203,7 +203,7 @@ def __init__( local_cache=execution_options.local_cache, remote_cache_read=execution_options.remote_cache_read, remote_cache_write=execution_options.remote_cache_write, - local_cleanup=execution_options.process_cleanup, + local_keep_sandboxes=execution_options.keep_sandboxes.value, local_parallelism=execution_options.process_execution_local_parallelism, local_enable_nailgun=execution_options.process_execution_local_enable_nailgun, remote_parallelism=execution_options.process_execution_remote_parallelism, diff --git a/src/python/pants/engine/process.py b/src/python/pants/engine/process.py index 6ba14e8e11c..78b2d906239 100644 --- a/src/python/pants/engine/process.py +++ b/src/python/pants/engine/process.py @@ -15,7 +15,7 @@ from pants.engine.internals.session import RunId from pants.engine.platform import Platform from pants.engine.rules import collect_rules, rule -from pants.option.global_options import ProcessCleanupOption +from pants.option.global_options import KeepSandboxes from pants.util.frozendict import FrozenDict from pants.util.logging import LogLevel from pants.util.meta import frozen_after_init @@ -216,7 +216,7 @@ def __init__( stderr: bytes, process_description: str, *, - process_cleanup: bool, + keep_sandboxes: KeepSandboxes, ) -> None: # These are intentionally "public" members. self.exit_code = exit_code @@ -239,9 +239,9 @@ def try_decode(content: bytes) -> str: "stderr:", try_decode(stderr), ] - if process_cleanup: + if keep_sandboxes == KeepSandboxes.never: err_strings.append( - "\n\nUse `--no-process-cleanup` to preserve process chroots for inspection." + "\n\nUse `--keep-sandboxes=on_failure` to preserve the process chroot for inspection." ) super().__init__("\n".join(err_strings)) @@ -255,7 +255,7 @@ def get_multi_platform_request_description(req: Process) -> ProductDescription: def fallible_to_exec_result_or_raise( fallible_result: FallibleProcessResult, description: ProductDescription, - process_cleanup: ProcessCleanupOption, + keep_sandboxes: KeepSandboxes, ) -> ProcessResult: """Converts a FallibleProcessResult to a ProcessResult or raises an error.""" @@ -274,7 +274,7 @@ def fallible_to_exec_result_or_raise( fallible_result.stdout, fallible_result.stderr, description.value, - process_cleanup=process_cleanup.val, + keep_sandboxes=keep_sandboxes, ) @@ -292,7 +292,7 @@ class InteractiveProcess(SideEffecting): run_in_workspace: bool forward_signals_to_process: bool restartable: bool - cleanup: bool + keep_sandboxes: KeepSandboxes def __init__( self, @@ -303,9 +303,10 @@ def __init__( run_in_workspace: bool = False, forward_signals_to_process: bool = True, restartable: bool = False, - cleanup: bool = True, + cleanup: bool | None = None, append_only_caches: Mapping[str, str] | None = None, immutable_input_digests: Mapping[str, Digest] | None = None, + keep_sandboxes: KeepSandboxes | None = None, ) -> None: """Request to run a subprocess in the foreground, similar to subprocess.run(). @@ -329,7 +330,19 @@ def __init__( self.run_in_workspace = run_in_workspace self.forward_signals_to_process = forward_signals_to_process self.restartable = restartable - self.cleanup = cleanup + if cleanup is not None: + warn_or_error( + removal_version="2.15.0.dev1", + entity="InteractiveProcess.cleanup", + hint="Use `InteractiveProcess.keep_sandboxes` instead.", + ) + if keep_sandboxes is not None: + raise ValueError("Only one of `cleanup` and `keep_sandboxes` may be specified.") + self.keep_sandboxes = KeepSandboxes.never if cleanup else KeepSandboxes.always + elif keep_sandboxes is not None: + self.keep_sandboxes = keep_sandboxes + else: + self.keep_sandboxes = KeepSandboxes.never @classmethod def from_process( diff --git a/src/python/pants/jvm/strip_jar/strip_jar.py b/src/python/pants/jvm/strip_jar/strip_jar.py index 934df1a723e..19cfe5ff1de 100644 --- a/src/python/pants/jvm/strip_jar/strip_jar.py +++ b/src/python/pants/jvm/strip_jar/strip_jar.py @@ -9,13 +9,12 @@ from pants.core.goals.generate_lockfiles import DEFAULT_TOOL_LOCKFILE, GenerateToolLockfileSentinel from pants.engine.fs import AddPrefix, CreateDigest, Digest, Directory, FileContent from pants.engine.internals.native_engine import MergeDigests, RemovePrefix -from pants.engine.process import FallibleProcessResult, ProcessExecutionFailure, ProcessResult +from pants.engine.process import FallibleProcessResult, ProcessResult from pants.engine.rules import Get, MultiGet, collect_rules, rule from pants.engine.unions import UnionRule from pants.jvm.jdk_rules import InternalJdk, JvmProcess from pants.jvm.resolve.coursier_fetch import ToolClasspath, ToolClasspathRequest from pants.jvm.resolve.jvm_tool import GenerateJvmLockfileFromTool -from pants.option.global_options import ProcessCleanupOption from pants.util.logging import LogLevel from pants.util.ordered_set import FrozenOrderedSet @@ -45,7 +44,6 @@ class StripJarCompiledClassfiles: @rule(level=LogLevel.DEBUG) async def strip_jar( - process_cleanup: ProcessCleanupOption, processor_classfiles: StripJarCompiledClassfiles, jdk: InternalJdk, request: StripJarRequest, @@ -76,7 +74,7 @@ async def strip_jar( } process_result = await Get( - FallibleProcessResult, + ProcessResult, JvmProcess( jdk=jdk, classpath_entries=[ @@ -93,16 +91,7 @@ async def strip_jar( ), ) - if process_result.exit_code == 0: - digest = await Get(Digest, RemovePrefix(process_result.output_digest, _OUTPUT_PATH)) - return digest - raise ProcessExecutionFailure( - process_result.exit_code, - process_result.stdout, - process_result.stderr, - "Strip jar failed.", - process_cleanup=process_cleanup.val, - ) + return await Get(Digest, RemovePrefix(process_result.output_digest, _OUTPUT_PATH)) def _load_strip_jar_source() -> bytes: diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index f6b7fe9f4f0..bdaf16a8c2f 100644 --- a/src/python/pants/option/global_options.py +++ b/src/python/pants/option/global_options.py @@ -119,6 +119,17 @@ class CacheContentBehavior(Enum): defer = "defer" +class KeepSandboxes(Enum): + """An enum for the global option `keep_sandboxes`. + + Prefer to use this rather than requesting `GlobalOptions` for more precise invalidation. + """ + + always = "always" + on_failure = "on_failure" + never = "never" + + @enum.unique class AuthPluginState(Enum): OK = "ok" @@ -473,7 +484,7 @@ class ExecutionOptions: remote_instance_name: str | None remote_ca_certs_path: str | None - process_cleanup: bool + keep_sandboxes: KeepSandboxes local_cache: bool process_execution_local_parallelism: int process_execution_local_enable_nailgun: bool @@ -518,7 +529,7 @@ def from_options( remote_instance_name=dynamic_remote_options.instance_name, remote_ca_certs_path=bootstrap_options.remote_ca_certs_path, # Process execution setup. - process_cleanup=bootstrap_options.process_cleanup, + keep_sandboxes=GlobalOptions.resolve_keep_sandboxes(bootstrap_options), local_cache=bootstrap_options.local_cache, process_execution_local_parallelism=bootstrap_options.process_execution_local_parallelism, process_execution_remote_parallelism=dynamic_remote_options.parallelism, @@ -607,7 +618,7 @@ def from_options(cls, options: OptionValueContainer) -> LocalStoreOptions: process_execution_local_parallelism=CPU_COUNT, process_execution_remote_parallelism=128, process_execution_cache_namespace=None, - process_cleanup=True, + keep_sandboxes=KeepSandboxes.never, local_cache=True, cache_content_behavior=CacheContentBehavior.fetch, process_execution_local_enable_nailgun=True, @@ -1145,7 +1156,10 @@ class BootstrapOptions: ), ) process_cleanup = BoolOption( - default=DEFAULT_EXECUTION_OPTIONS.process_cleanup, + default=(DEFAULT_EXECUTION_OPTIONS.keep_sandboxes == KeepSandboxes.never), + deprecation_start_version="2.15.0.dev1", + removal_version="2.16.0.dev1", + removal_hint="Use the `keep_sandboxes` option instead.", help=softwrap( """ If false, Pants will not clean up local directories used as chroots for running @@ -1155,6 +1169,19 @@ class BootstrapOptions: """ ), ) + keep_sandboxes = EnumOption( + default=DEFAULT_EXECUTION_OPTIONS.keep_sandboxes, + help=softwrap( + """ + Controls whether Pants will clean up local directories used as chroots for running + processes. + + Pants will log their location so that you can inspect the chroot, and run the + `__run.sh` script to recreate the process using the same argv and environment variables + used by Pants. This option is useful for debugging. + """ + ), + ) cache_content_behavior = EnumOption( advanced=True, default=DEFAULT_EXECUTION_OPTIONS.cache_content_behavior, @@ -1893,6 +1920,27 @@ def resolve_cache_content_behavior( f"Unexpected option value for `cache_content_behavior`: {resolved_value}" ) + @staticmethod + def resolve_keep_sandboxes( + bootstrap_options: OptionValueContainer, + ) -> KeepSandboxes: + resolved_value = resolve_conflicting_options( + old_option="process_cleanup", + new_option="keep_sandboxes", + old_scope="", + new_scope="", + old_container=bootstrap_options, + new_container=bootstrap_options, + ) + + if isinstance(resolved_value, bool): + # Is `process_cleanup`. + return KeepSandboxes.always if resolved_value else KeepSandboxes.on_failure + elif isinstance(resolved_value, KeepSandboxes): + return resolved_value + else: + raise TypeError(f"Unexpected option value for `keep_sandboxes`: {resolved_value}") + @staticmethod def compute_pants_ignore(buildroot, global_options): """Computes the merged value of the `--pants-ignore` flag. diff --git a/src/python/pants/option/option_types.py b/src/python/pants/option/option_types.py index 8d5a7d82d4a..e4579737e8a 100644 --- a/src/python/pants/option/option_types.py +++ b/src/python/pants/option/option_types.py @@ -92,6 +92,7 @@ def __new__( mutually_exclusive_group: str | None = None, removal_version: str | None = None, removal_hint: str | None = None, + deprecation_start_version: str | None = None, # Internal bells/whistles daemon: bool | None = None, fingerprint: bool | None = None, @@ -128,6 +129,8 @@ def __new__( be removed in. You must also set `removal_hint`. :param removal_hint: If the option is deprecated, provides a message to display to the user when running `help`. + :param deprecation_start_version: If the option is deprecated, sets the version at which the + deprecation will begin. Must be less than the `removal_version`. """ self = super().__new__(cls) self._flag_names = (flag_name,) if flag_name else None @@ -146,6 +149,7 @@ def __new__( "mutually_exclusive_group": mutually_exclusive_group, "removal_hint": removal_hint, "removal_version": removal_version, + "deprecation_start_version": deprecation_start_version, }.items() if v is not None } @@ -227,6 +231,7 @@ def __new__( mutually_exclusive_group: str | None = None, removal_version: str | None = None, removal_hint: str | None = None, + deprecation_start_version: str | None = None, # Internal bells/whistles daemon: bool | None = None, fingerprint: bool | None = None, @@ -247,6 +252,7 @@ def __new__( mutually_exclusive_group=mutually_exclusive_group, removal_hint=removal_hint, removal_version=removal_version, + deprecation_start_version=deprecation_start_version, ) return instance @@ -442,6 +448,7 @@ def __new__( mutually_exclusive_group: str | None = None, removal_version: str | None = None, removal_hint: str | None = None, + deprecation_start_version: str | None = None, # Internal bells/whistles daemon: bool | None = None, fingerprint: bool | None = None, @@ -466,6 +473,7 @@ def __new__( mutually_exclusive_group: str | None = None, removal_version: str | None = None, removal_hint: str | None = None, + deprecation_start_version: str | None = None, # Internal bells/whistles daemon: bool | None = None, fingerprint: bool | None = None, @@ -490,6 +498,7 @@ def __new__( mutually_exclusive_group: str | None = None, removal_version: str | None = None, removal_hint: str | None = None, + deprecation_start_version: str | None = None, # Internal bells/whistles daemon: bool | None = None, fingerprint: bool | None = None, @@ -512,6 +521,7 @@ def __new__( mutually_exclusive_group=None, removal_version=None, removal_hint=None, + deprecation_start_version=None, # Internal bells/whistles daemon=None, fingerprint=None, @@ -529,6 +539,7 @@ def __new__( mutually_exclusive_group=mutually_exclusive_group, removal_version=removal_version, removal_hint=removal_hint, + deprecation_start_version=deprecation_start_version, daemon=daemon, fingerprint=fingerprint, ) @@ -580,6 +591,7 @@ def __new__( mutually_exclusive_group: str | None = None, removal_version: str | None = None, removal_hint: str | None = None, + deprecation_start_version: str | None = None, # Internal bells/whistles daemon: bool | None = None, fingerprint: bool | None = None, @@ -604,6 +616,7 @@ def __new__( mutually_exclusive_group: str | None = None, removal_version: str | None = None, removal_hint: str | None = None, + deprecation_start_version: str | None = None, # Internal bells/whistles daemon: bool | None = None, fingerprint: bool | None = None, @@ -627,6 +640,7 @@ def __new__( mutually_exclusive_group: str | None = None, removal_version: str | None = None, removal_hint: str | None = None, + deprecation_start_version: str | None = None, # Internal bells/whistles daemon: bool | None = None, fingerprint: bool | None = None, @@ -649,6 +663,7 @@ def __new__( mutually_exclusive_group=None, removal_version=None, removal_hint=None, + deprecation_start_version=None, # Internal bells/whistles daemon=None, fingerprint=None, @@ -666,6 +681,7 @@ def __new__( mutually_exclusive_group=mutually_exclusive_group, removal_version=removal_version, removal_hint=removal_hint, + deprecation_start_version=deprecation_start_version, daemon=daemon, fingerprint=fingerprint, ) @@ -730,6 +746,7 @@ def __new__( mutually_exclusive_group: str | None = None, removal_version: str | None = None, removal_hint: str | None = None, + deprecation_start_version: str | None = None, # Internal bells/whistles daemon: bool | None = None, fingerprint: bool | None = None, @@ -749,6 +766,7 @@ def __new__( mutually_exclusive_group=mutually_exclusive_group, removal_hint=removal_hint, removal_version=removal_version, + deprecation_start_version=deprecation_start_version, ) def _convert_(self, val: Any) -> dict[str, _ValueT]: diff --git a/src/python/pants/testutil/rule_runner.py b/src/python/pants/testutil/rule_runner.py index fdec63c2a09..804e358e43c 100644 --- a/src/python/pants/testutil/rule_runner.py +++ b/src/python/pants/testutil/rule_runner.py @@ -206,7 +206,7 @@ def __init__( root_dir = Path(mkdtemp(prefix="RuleRunner.")) print(f"Preserving rule runner temporary directories at {root_dir}.", file=sys.stderr) bootstrap_args.extend( - ["--no-process-cleanup", f"--local-execution-root-dir={root_dir}"] + ["--keep-sandboxes=always", f"--local-execution-root-dir={root_dir}"] ) build_root = (root_dir / "BUILD_ROOT").resolve() build_root.mkdir() diff --git a/src/rust/engine/process_execution/src/local.rs b/src/rust/engine/process_execution/src/local.rs index 93026b36fd1..0235dbb19ff 100644 --- a/src/rust/engine/process_execution/src/local.rs +++ b/src/rust/engine/process_execution/src/local.rs @@ -42,13 +42,21 @@ use crate::{ pub const USER_EXECUTABLE_MODE: u32 = 0o100755; +#[derive(Clone, Copy, Debug, PartialEq, strum_macros::EnumString)] +#[strum(serialize_all = "snake_case")] +pub enum KeepSandboxes { + Always, + Never, + OnFailure, +} + pub struct CommandRunner { pub store: Store, executor: Executor, work_dir_base: PathBuf, named_caches: NamedCaches, immutable_inputs: ImmutableInputs, - cleanup_local_dirs: bool, + keep_sandboxes: KeepSandboxes, platform: Platform, spawn_lock: RwLock<()>, } @@ -60,7 +68,7 @@ impl CommandRunner { work_dir_base: PathBuf, named_caches: NamedCaches, immutable_inputs: ImmutableInputs, - cleanup_local_dirs: bool, + keep_sandboxes: KeepSandboxes, ) -> CommandRunner { CommandRunner { store, @@ -68,7 +76,7 @@ impl CommandRunner { work_dir_base, named_caches, immutable_inputs, - cleanup_local_dirs, + keep_sandboxes, platform: Platform::current().unwrap(), spawn_lock: RwLock::new(()), } @@ -271,11 +279,11 @@ impl super::CommandRunner for CommandRunner { // renders at the Process's level. desc = Some(req.description.clone()), |workunit| async move { - let workdir = create_sandbox( + let mut workdir = create_sandbox( self.executor.clone(), &self.work_dir_base, &req.description, - self.cleanup_local_dirs, + self.keep_sandboxes, )?; // Start working on a mutable version of the process. @@ -320,7 +328,11 @@ impl super::CommandRunner for CommandRunner { }) .await; - if !self.cleanup_local_dirs { + if self.keep_sandboxes == KeepSandboxes::Always + || self.keep_sandboxes == KeepSandboxes::OnFailure + && res.as_ref().map(|r| r.exit_code).unwrap_or(1) != 0 + { + workdir.keep(&req.description); setup_run_sh_script(&req.env, &req.working_directory, &req.argv, workdir.path())?; } @@ -726,12 +738,17 @@ pub async fn prepare_workdir( Ok(exclusive_spawn) } +/// /// Creates an optionally-cleaned-up sandbox in the given base path. +/// +/// If KeepSandboxes::Always, it is immediately marked preserved: otherwise, the caller should +/// decide whether to preserve it. +/// pub fn create_sandbox( executor: Executor, base_directory: &Path, description: &str, - cleanup: bool, + keep_sandboxes: KeepSandboxes, ) -> Result { let workdir = tempfile::Builder::new() .prefix("pants-sandbox-") @@ -743,22 +760,11 @@ pub fn create_sandbox( ) })?; - let (workdir_path, maybe_workdir) = if cleanup { - // Hold on to the workdir so that we can drop it explicitly after we've finished using it. - (workdir.path().to_owned(), Some(workdir)) - } else { - // This consumes the `TempDir` without deleting directory on the filesystem, meaning - // that the temporary directory will no longer be automatically deleted when dropped. - let preserved_path = workdir.into_path(); - info!( - "Preserving local process execution dir {} for {}", - preserved_path.display(), - description, - ); - (preserved_path, None) - }; - - Ok(AsyncDropSandbox(executor, workdir_path, maybe_workdir)) + let mut sandbox = AsyncDropSandbox(executor, workdir.path().to_owned(), Some(workdir)); + if keep_sandboxes == KeepSandboxes::Always { + sandbox.keep(description); + } + Ok(sandbox) } /// Dropping sandboxes can involve a lot of IO, so it is spawned to the background as a blocking @@ -770,6 +776,21 @@ impl AsyncDropSandbox { pub fn path(&self) -> &Path { &self.1 } + + /// + /// Consume the `TempDir` without deleting directory on the filesystem, meaning that the + /// temporary directory will no longer be automatically deleted when dropped. + /// + pub fn keep(&mut self, description: &str) { + if let Some(workdir) = self.2.take() { + let preserved_path = workdir.into_path(); + info!( + "Preserving local process execution dir {} for {}", + preserved_path.display(), + description, + ); + } + } } impl Drop for AsyncDropSandbox { diff --git a/src/rust/engine/src/context.rs b/src/rust/engine/src/context.rs index a814eb99e87..810fb0eb002 100644 --- a/src/rust/engine/src/context.rs +++ b/src/rust/engine/src/context.rs @@ -106,7 +106,7 @@ pub struct RemotingOptions { pub struct ExecutionStrategyOptions { pub local_parallelism: usize, pub remote_parallelism: usize, - pub local_cleanup: bool, + pub local_keep_sandboxes: local::KeepSandboxes, pub local_cache: bool, pub local_enable_nailgun: bool, pub remote_cache_read: bool, @@ -216,7 +216,7 @@ impl Core { local_execution_root_dir.to_path_buf(), named_caches.clone(), immutable_inputs.clone(), - exec_strategy_opts.local_cleanup, + exec_strategy_opts.local_keep_sandboxes, ); let runner: Box = if exec_strategy_opts.local_enable_nailgun { diff --git a/src/rust/engine/src/externs/interface.rs b/src/rust/engine/src/externs/interface.rs index b22ce5b1462..0ec95d6df84 100644 --- a/src/rust/engine/src/externs/interface.rs +++ b/src/rust/engine/src/externs/interface.rs @@ -244,7 +244,7 @@ impl PyExecutionStrategyOptions { fn __new__( local_parallelism: usize, remote_parallelism: usize, - local_cleanup: bool, + local_keep_sandboxes: String, local_cache: bool, local_enable_nailgun: bool, remote_cache_read: bool, @@ -256,7 +256,8 @@ impl PyExecutionStrategyOptions { Self(ExecutionStrategyOptions { local_parallelism, remote_parallelism, - local_cleanup, + local_keep_sandboxes: process_execution::local::KeepSandboxes::from_str(&local_keep_sandboxes) + .unwrap(), local_cache, local_enable_nailgun, remote_cache_read, diff --git a/src/rust/engine/src/intrinsics.rs b/src/rust/engine/src/intrinsics.rs index ea23dced4bb..bb3c4d37c8e 100644 --- a/src/rust/engine/src/intrinsics.rs +++ b/src/rust/engine/src/intrinsics.rs @@ -3,6 +3,7 @@ use std::path::{Path, PathBuf}; use std::process::Stdio; +use std::str::FromStr; use std::time::Duration; use crate::context::Context; @@ -20,12 +21,12 @@ use crate::Failure; use futures::future::{self, BoxFuture, FutureExt, TryFutureExt}; use futures::try_join; use indexmap::IndexMap; -use pyo3::{PyRef, Python, ToPyObject}; +use pyo3::{PyAny, PyRef, Python, ToPyObject}; use tokio::process; use fs::{DirectoryDigest, RelativePath}; use hashing::Digest; -use process_execution::local::{apply_chroot, create_sandbox, prepare_workdir}; +use process_execution::local::{apply_chroot, create_sandbox, prepare_workdir, KeepSandboxes}; use process_execution::ManagedChild; use stdio::TryCloneAsFile; use store::{SnapshotOps, SubsetParams}; @@ -521,22 +522,23 @@ fn interactive_process( (py_interactive_process.extract().unwrap(), py_process) }); let mut process = ExecuteProcess::lift_process(&context.core.store(), py_process).await?; - let (run_in_workspace, restartable, cleanup) = Python::with_gil(|py| { + let (run_in_workspace, restartable, keep_sandboxes) = Python::with_gil(|py| { let py_interactive_process_obj = py_interactive_process.to_object(py); let py_interactive_process = py_interactive_process_obj.as_ref(py); let run_in_workspace: bool = externs::getattr(py_interactive_process, "run_in_workspace").unwrap(); let restartable: bool = externs::getattr(py_interactive_process, "restartable").unwrap(); - let cleanup: bool = externs::getattr(py_interactive_process, "cleanup").unwrap(); - (run_in_workspace, restartable, cleanup) + let keep_sandboxes_value: &PyAny = externs::getattr(py_interactive_process, "keep_sandboxes").unwrap(); + let keep_sandboxes = KeepSandboxes::from_str(externs::getattr(keep_sandboxes_value, "value").unwrap()).unwrap(); + (run_in_workspace, restartable, keep_sandboxes) }); let session = context.session; - let tempdir = create_sandbox( + let mut tempdir = create_sandbox( context.core.executor.clone(), &context.core.local_execution_root_dir, "interactive process", - cleanup, + keep_sandboxes, )?; prepare_workdir( tempdir.path().to_owned(), @@ -626,6 +628,10 @@ fn interactive_process( .await?; let code = exit_status.code().unwrap_or(-1); + if keep_sandboxes == KeepSandboxes::OnFailure && code != 0 { + tempdir.keep("interactive process"); + } + let result = { let gil = Python::acquire_gil(); let py = gil.python();