From a0043328490f6ba14535b11a2e563117245f27d7 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Wed, 14 Sep 2022 18:33:32 -0500 Subject: [PATCH] Every `Process` is now platform-specific # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- .../common-plugin-tasks/plugin-upgrade-guide.md | 15 +++++++++++++++ .../pants/backend/codegen/protobuf/go/rules.py | 4 +--- src/python/pants/backend/go/util_rules/sdk.py | 5 ----- .../pants/backend/python/util_rules/pex.py | 11 ++--------- src/python/pants/core/goals/check_test.py | 2 +- src/python/pants/core/util_rules/environments.py | 6 ++++-- .../pants/engine/internals/native_engine.pyi | 2 +- src/python/pants/engine/process.py | 16 ++++++++++++++-- src/python/pants/jvm/jdk_rules.py | 5 ----- src/python/pants/testutil/rule_runner.py | 7 ++++++- src/rust/engine/src/externs/process.rs | 16 +++++++++++++--- src/rust/engine/src/nodes.rs | 11 ++--------- 12 files changed, 59 insertions(+), 41 deletions(-) diff --git a/docs/markdown/Writing Plugins/common-plugin-tasks/plugin-upgrade-guide.md b/docs/markdown/Writing Plugins/common-plugin-tasks/plugin-upgrade-guide.md index ed3502bb32a..7466b11a4ad 100644 --- a/docs/markdown/Writing Plugins/common-plugin-tasks/plugin-upgrade-guide.md +++ b/docs/markdown/Writing Plugins/common-plugin-tasks/plugin-upgrade-guide.md @@ -9,6 +9,21 @@ updatedAt: "2022-07-25T20:02:17.695Z" 2.15 ---- +### `platform` kwarg for `Process` deprecated + +Previously, we assumed processes were platform-agnostic, i.e. they had identical output on all +platforms (OS x CPU architecture). You had to opt into platform awareness by setting the kwarg +`platform` on the `Process`; otherwise, remote caching could incorrectly use results from a +different platform. + +This was not a safe default, and this behavior also breaks the new Docker support. So, now all +processes automatically are marked as platform-specific. + +https://github.com/pantsbuild/pants/issues/16873 proposes how you will eventually be able to mark +a `Process` as platform-agnostic. + +To fix this deprecation, simply delete the `platform` kwarg. + ### `Environment`, `EnvironmentRequest`, and `CompleteEnvironment` renamed and moved The types were moved from `pants.engine.environment` to `pants.engine.env_vars`, and now have diff --git a/src/python/pants/backend/codegen/protobuf/go/rules.py b/src/python/pants/backend/codegen/protobuf/go/rules.py index f79baaad331..fbc1227eba6 100644 --- a/src/python/pants/backend/codegen/protobuf/go/rules.py +++ b/src/python/pants/backend/codegen/protobuf/go/rules.py @@ -557,7 +557,7 @@ async def generate_go_from_protobuf( @rule -async def setup_go_protoc_plugin(platform: Platform) -> _SetupGoProtocPlugin: +async def setup_go_protoc_plugin() -> _SetupGoProtocPlugin: go_mod_digest = await Get( Digest, CreateDigest( @@ -587,7 +587,6 @@ async def setup_go_protoc_plugin(platform: Platform) -> _SetupGoProtocPlugin: input_digest=download_sources_result.output_digest, output_files=["gopath/bin/protoc-gen-go"], description="Build Go protobuf plugin for `protoc`.", - platform=platform, ), ), Get( @@ -600,7 +599,6 @@ async def setup_go_protoc_plugin(platform: Platform) -> _SetupGoProtocPlugin: input_digest=download_sources_result.output_digest, output_files=["gopath/bin/protoc-gen-go-grpc"], description="Build Go gRPC protobuf plugin for `protoc`.", - platform=platform, ), ), ) diff --git a/src/python/pants/backend/go/util_rules/sdk.py b/src/python/pants/backend/go/util_rules/sdk.py index bace4616b4c..c1362bb0e53 100644 --- a/src/python/pants/backend/go/util_rules/sdk.py +++ b/src/python/pants/backend/go/util_rules/sdk.py @@ -14,7 +14,6 @@ from pants.engine.env_vars import EnvironmentVars, EnvironmentVarsRequest from pants.engine.fs import EMPTY_DIGEST, CreateDigest, Digest, FileContent, MergeDigests from pants.engine.internals.selectors import Get, MultiGet -from pants.engine.platform import Platform from pants.engine.process import Process, ProcessResult from pants.engine.rules import collect_rules, rule from pants.util.frozendict import FrozenDict @@ -32,7 +31,6 @@ class GoSdkProcess: working_dir: str | None output_files: tuple[str, ...] output_directories: tuple[str, ...] - platform: Platform | None replace_sandbox_root_in_args: bool def __init__( @@ -46,7 +44,6 @@ def __init__( output_files: Iterable[str] = (), output_directories: Iterable[str] = (), allow_downloads: bool = False, - platform: Platform | None = None, replace_sandbox_root_in_args: bool = False, ) -> None: self.command = tuple(command) @@ -60,7 +57,6 @@ def __init__( self.working_dir = working_dir self.output_files = tuple(output_files) self.output_directories = tuple(output_directories) - self.platform = platform self.replace_sandbox_root_in_args = replace_sandbox_root_in_args @@ -136,7 +132,6 @@ async def setup_go_sdk_process( output_files=request.output_files, output_directories=request.output_directories, level=LogLevel.DEBUG, - platform=request.platform, ) diff --git a/src/python/pants/backend/python/util_rules/pex.py b/src/python/pants/backend/python/util_rules/pex.py index 406ce5bfae0..7edf34e9da7 100644 --- a/src/python/pants/backend/python/util_rules/pex.py +++ b/src/python/pants/backend/python/util_rules/pex.py @@ -555,8 +555,8 @@ async def build_pex( else: output_directories = [request.output_filename] - process = await Get( - Process, + result = await Get( + ProcessResult, PexCliProcess( python=pex_python_setup.python, subcommand=(), @@ -569,13 +569,6 @@ async def build_pex( ), ) - process = dataclasses.replace(process, platform=platform) - - # NB: Building a Pex is platform dependent, so in order to get a PEX that we can use locally - # without cross-building, we specify that our PEX command should be run on the current local - # platform. - result = await Get(ProcessResult, Process, process) - if pex_subsystem.verbosity > 0: log_output = result.stderr.decode() if log_output: diff --git a/src/python/pants/core/goals/check_test.py b/src/python/pants/core/goals/check_test.py index acb56a5531e..3cc39ac7a2b 100644 --- a/src/python/pants/core/goals/check_test.py +++ b/src/python/pants/core/goals/check_test.py @@ -268,7 +268,7 @@ def test_from_fallible_process_result_output_prepping( stderr=b"stderr \033[0;31m/var/pants-sandbox-123/red/path.py\033[0m \033[1mbold\033[0m", stderr_digest=EMPTY_FILE_DIGEST, output_digest=EMPTY_DIGEST, - platform=Platform.current, + platform=Platform.create_for_localhost(), metadata=ProcessResultMetadata(0, "ran_locally", 0), ), strip_chroot_path=strip_chroot_path, diff --git a/src/python/pants/core/util_rules/environments.py b/src/python/pants/core/util_rules/environments.py index 36015cf4714..f9a60156400 100644 --- a/src/python/pants/core/util_rules/environments.py +++ b/src/python/pants/core/util_rules/environments.py @@ -356,11 +356,13 @@ async def get_target_for_environment_name( @rule -def extract_process_config_from_environment(tgt: EnvironmentTarget) -> ProcessConfigFromEnvironment: +def extract_process_config_from_environment( + tgt: EnvironmentTarget, platform: Platform +) -> ProcessConfigFromEnvironment: docker_image = ( tgt.val[DockerImageField].value if tgt.val and tgt.val.has_field(DockerImageField) else None ) - return ProcessConfigFromEnvironment(docker_image=docker_image) + return ProcessConfigFromEnvironment(platform=platform.value, docker_image=docker_image) class EnvironmentSensitiveOptionFieldMixin: diff --git a/src/python/pants/engine/internals/native_engine.pyi b/src/python/pants/engine/internals/native_engine.pyi index e6d0beceacc..58cbc94849b 100644 --- a/src/python/pants/engine/internals/native_engine.pyi +++ b/src/python/pants/engine/internals/native_engine.pyi @@ -172,7 +172,7 @@ class ProcessConfigFromEnvironment: `env` in the `Process` constructor. """ - def __init__(self, *, docker_image: str | None) -> None: ... + def __init__(self, *, platform: str, docker_image: str | None) -> None: ... def __eq__(self, other: ProcessConfigFromEnvironment | Any) -> bool: ... def __hash__(self) -> int: ... def __repr__(self) -> str: ... diff --git a/src/python/pants/engine/process.py b/src/python/pants/engine/process.py index 2ffef223965..aefb690db58 100644 --- a/src/python/pants/engine/process.py +++ b/src/python/pants/engine/process.py @@ -9,6 +9,7 @@ from enum import Enum from typing import Iterable, Mapping +from pants.base.deprecated import warn_or_error from pants.engine.engine_aware import SideEffecting from pants.engine.fs import EMPTY_DIGEST, Digest, FileDigest from pants.engine.internals.native_engine import ( # noqa: F401 @@ -21,6 +22,7 @@ from pants.util.frozendict import FrozenDict from pants.util.logging import LogLevel from pants.util.meta import frozen_after_init +from pants.util.strutil import softwrap logger = logging.getLogger(__name__) @@ -65,7 +67,6 @@ class Process: execution_slot_variable: str | None concurrency_available: int cache_scope: ProcessCacheScope - platform: str | None def __init__( self, @@ -133,7 +134,18 @@ def __init__( self.execution_slot_variable = execution_slot_variable self.concurrency_available = concurrency_available self.cache_scope = cache_scope - self.platform = platform.value if platform is not None else None + + if platform is not None: + warn_or_error( + "2.16.0.dev0", + "the `platform` kwarg for `Process`", + softwrap( + """ + The `platform` kwarg no longer does anything because the `platform` is always + automatically set. To fix this deprecation, delete the kwarg. + """ + ), + ) @dataclass(frozen=True) diff --git a/src/python/pants/jvm/jdk_rules.py b/src/python/pants/jvm/jdk_rules.py index 6b1bcfb0d18..71a10316a67 100644 --- a/src/python/pants/jvm/jdk_rules.py +++ b/src/python/pants/jvm/jdk_rules.py @@ -16,7 +16,6 @@ from pants.core.util_rules.system_binaries import BashBinary from pants.engine.fs import CreateDigest, Digest, FileContent, FileDigest, MergeDigests from pants.engine.internals.selectors import Get -from pants.engine.platform import Platform from pants.engine.process import FallibleProcessResult, Process, ProcessCacheScope from pants.engine.rules import collect_rules, rule from pants.engine.target import CoarsenedTarget @@ -320,7 +319,6 @@ class JvmProcess: output_files: tuple[str, ...] output_directories: tuple[str, ...] timeout_seconds: int | float | None - platform: Platform | None extra_immutable_input_digests: FrozenDict[str, Digest] extra_env: FrozenDict[str, str] cache_scope: ProcessCacheScope | None @@ -341,7 +339,6 @@ def __init__( extra_immutable_input_digests: Mapping[str, Digest] | None = None, extra_env: Mapping[str, str] | None = None, timeout_seconds: int | float | None = None, - platform: Platform | None = None, cache_scope: ProcessCacheScope | None = None, use_nailgun: bool = True, ): @@ -356,7 +353,6 @@ def __init__( self.output_files = tuple(output_files or ()) self.output_directories = tuple(output_directories or ()) self.timeout_seconds = timeout_seconds - self.platform = platform self.cache_scope = cache_scope self.extra_immutable_input_digests = FrozenDict(extra_immutable_input_digests or {}) self.extra_env = FrozenDict(extra_env or {}) @@ -427,7 +423,6 @@ def valid_jvm_opt(opt: str) -> str: level=request.level, output_directories=request.output_directories, env=env, - platform=request.platform, timeout_seconds=request.timeout_seconds, append_only_caches=jdk.append_only_caches, output_files=request.output_files, diff --git a/src/python/pants/testutil/rule_runner.py b/src/python/pants/testutil/rule_runner.py index 791583ae784..96528d9634a 100644 --- a/src/python/pants/testutil/rule_runner.py +++ b/src/python/pants/testutil/rule_runner.py @@ -42,6 +42,7 @@ from pants.engine.internals.scheduler import ExecutionError, Scheduler, SchedulerSession from pants.engine.internals.selectors import Effect, Get, Params from pants.engine.internals.session import SessionValues +from pants.engine.platform import Platform from pants.engine.process import InteractiveProcess, InteractiveProcessResult from pants.engine.rules import QueryRule as QueryRule from pants.engine.rules import rule @@ -517,7 +518,11 @@ def write_digest(self, digest: Digest, *, path_prefix: str | None = None) -> Non def run_interactive_process(self, request: InteractiveProcess) -> InteractiveProcessResult: return native_engine.session_run_interactive_process( - self.scheduler.py_session, request, ProcessConfigFromEnvironment(docker_image=None) + self.scheduler.py_session, + request, + ProcessConfigFromEnvironment( + platform=Platform.create_for_localhost().value, docker_image=None + ), ) diff --git a/src/rust/engine/src/externs/process.rs b/src/rust/engine/src/externs/process.rs index e40a472aabd..5f7e8557701 100644 --- a/src/rust/engine/src/externs/process.rs +++ b/src/rust/engine/src/externs/process.rs @@ -5,8 +5,11 @@ use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; use pyo3::basic::CompareOp; +use pyo3::exceptions::PyValueError; use pyo3::prelude::*; +use process_execution::Platform; + pub(crate) fn register(m: &PyModule) -> PyResult<()> { m.add_class::()?; @@ -16,25 +19,32 @@ pub(crate) fn register(m: &PyModule) -> PyResult<()> { #[pyclass(name = "ProcessConfigFromEnvironment")] #[derive(Clone, Debug, PartialEq, Eq)] pub struct PyProcessConfigFromEnvironment { + pub platform: Platform, pub docker_image: Option, } #[pymethods] impl PyProcessConfigFromEnvironment { #[new] - fn __new__(docker_image: Option) -> Self { - Self { docker_image } + fn __new__(platform: String, docker_image: Option) -> PyResult { + let platform = Platform::try_from(platform).map_err(PyValueError::new_err)?; + Ok(Self { + platform, + docker_image, + }) } fn __hash__(&self) -> u64 { let mut s = DefaultHasher::new(); + self.platform.hash(&mut s); self.docker_image.hash(&mut s); s.finish() } fn __repr__(&self) -> String { format!( - "ProcessConfigFromEnvironment(docker_image={})", + "ProcessConfigFromEnvironment(platform={}, docker_image={})", + String::from(self.platform), self.docker_image.as_ref().unwrap_or(&"None".to_owned()) ) } diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index b3dd328df4a..1bd083094f4 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -32,7 +32,7 @@ use fs::{ RelativePath, StrictGlobMatching, Vfs, }; use process_execution::{ - self, CacheName, InputDigests, Platform, Process, ProcessCacheScope, ProcessResultSource, + self, CacheName, InputDigests, Process, ProcessCacheScope, ProcessResultSource, }; use crate::externs::engine_aware::{EngineAwareParameter, EngineAwareReturnType}; @@ -386,13 +386,6 @@ impl ExecuteProcess { .try_into()? }; - let platform_constraint = - if let Some(p) = externs::getattr_as_optional_string(value, "platform") { - Some(Platform::try_from(p)?) - } else { - None - }; - Ok(process_execution::Process { argv: externs::getattr(value, "argv").unwrap(), env, @@ -405,7 +398,7 @@ impl ExecuteProcess { level, append_only_caches, jdk_home, - platform_constraint, + platform_constraint: Some(process_config.platform), execution_slot_variable, concurrency_available, cache_scope,