From c76058da74df3648690a7f53072e0c7f08b7fa58 Mon Sep 17 00:00:00 2001 From: Greg Shuflin Date: Mon, 12 Oct 2020 12:11:46 -0700 Subject: [PATCH 1/2] Expose getattr and replace project_bool [ci skip-build-wheels] --- src/rust/engine/src/externs/interface.rs | 4 ++-- src/rust/engine/src/externs/mod.rs | 6 +----- src/rust/engine/src/intrinsics.rs | 3 ++- src/rust/engine/src/nodes.rs | 4 ++-- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/rust/engine/src/externs/interface.rs b/src/rust/engine/src/externs/interface.rs index 87aa89d31fb..183430f5184 100644 --- a/src/rust/engine/src/externs/interface.rs +++ b/src/rust/engine/src/externs/interface.rs @@ -1513,7 +1513,7 @@ fn run_local_interactive_process( return Err("Empty argv list not permitted".to_string()); } - let run_in_workspace = externs::project_bool(&value, "run_in_workspace"); + let run_in_workspace: bool = externs::getattr(&value, "run_in_workspace").unwrap(); let maybe_tempdir = if run_in_workspace { None } else { @@ -1558,7 +1558,7 @@ fn run_local_interactive_process( command.current_dir(tempdir.path()); } - let hermetic_env = externs::project_bool(&value, "hermetic_env"); + let hermetic_env: bool = externs::getattr(&value, "hermetic_env").unwrap(); if hermetic_env { command.env_clear(); } diff --git a/src/rust/engine/src/externs/mod.rs b/src/rust/engine/src/externs/mod.rs index ee5970ef610..319cec1e067 100644 --- a/src/rust/engine/src/externs/mod.rs +++ b/src/rust/engine/src/externs/mod.rs @@ -176,7 +176,7 @@ pub fn hasattr(value: &PyObject, field: &str) -> bool { /// /// Gets an attribute of the given value as the given type. /// -fn getattr(value: &PyObject, field: &str) -> Result +pub fn getattr(value: &PyObject, field: &str) -> Result where for<'a> T: FromPyObject<'a>, { @@ -239,10 +239,6 @@ pub fn project_multi(value: &Value, field: &str) -> Vec { getattr(value.as_ref(), field).unwrap() } -pub fn project_bool(value: &Value, field: &str) -> bool { - getattr(value.as_ref(), field).unwrap() -} - pub fn project_multi_strs(value: &Value, field: &str) -> Vec { getattr(value.as_ref(), field).unwrap() } diff --git a/src/rust/engine/src/intrinsics.rs b/src/rust/engine/src/intrinsics.rs index d3539ebc0d8..1e3ed314a78 100644 --- a/src/rust/engine/src/intrinsics.rs +++ b/src/rust/engine/src/intrinsics.rs @@ -360,7 +360,8 @@ fn create_digest_to_digest( &file_content_or_directory, "content", )); - let is_executable = externs::project_bool(&file_content_or_directory, "is_executable"); + let is_executable: bool = + externs::getattr(&file_content_or_directory, "is_executable").unwrap(); let digest = store.store_file_bytes(bytes, true).await?; let snapshot = store diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index 4fbfa2ffdb8..711af1659c1 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -300,7 +300,7 @@ impl MultiPlatformExecuteProcess { } }; - let is_nailgunnable = externs::project_bool(&value, "is_nailgunnable"); + let is_nailgunnable: bool = externs::getattr(&value, "is_nailgunnable").unwrap(); let execution_slot_variable = { let s = externs::project_str(&value, "execution_slot_variable"); @@ -311,7 +311,7 @@ impl MultiPlatformExecuteProcess { } }; - let cache_failures = externs::project_bool(&value, "cache_failures"); + let cache_failures: bool = externs::getattr(&value, "cache_failures").unwrap(); Ok(process_execution::Process { argv: externs::project_multi_strs(&value, "argv"), From 66134605c33d67c39469e1edaa39173fe72dcaf4 Mon Sep 17 00:00:00 2001 From: Greg Shuflin Date: Mon, 12 Oct 2020 16:21:19 -0700 Subject: [PATCH 2/2] Remove more project_ methods And use &PyObject in what's left [ci skip-build-wheels] --- src/rust/engine/src/core.rs | 2 +- src/rust/engine/src/externs/mod.rs | 36 +++++++++--------------------- src/rust/engine/src/intrinsics.rs | 7 +++--- src/rust/engine/src/nodes.rs | 6 ++--- 4 files changed, 18 insertions(+), 33 deletions(-) diff --git a/src/rust/engine/src/core.rs b/src/rust/engine/src/core.rs index 97e0d231aad..a82f928c89e 100644 --- a/src/rust/engine/src/core.rs +++ b/src/rust/engine/src/core.rs @@ -161,7 +161,7 @@ impl Function { let name = externs::project_str(&val, "__name__"); // NB: this is a custom dunder method that Python code should populate before sending the // function (e.g. an `@rule`) through FFI. - let line_number = externs::project_u64(&val, "__line_number__"); + let line_number: u64 = externs::getattr(&val, "__line_number__").unwrap(); format!("{}:{}:{}", module, line_number, name) } } diff --git a/src/rust/engine/src/externs/mod.rs b/src/rust/engine/src/externs/mod.rs index 319cec1e067..6f0ef1f3753 100644 --- a/src/rust/engine/src/externs/mod.rs +++ b/src/rust/engine/src/externs/mod.rs @@ -227,24 +227,24 @@ fn collect_iterable(value: &Value) -> Result, String> { /// /// Pulls out the value specified by the field name from a given Value /// -pub fn project_ignoring_type(value: &Value, field: &str) -> Value { - getattr(value.as_ref(), field).unwrap() +pub fn project_ignoring_type(value: &PyObject, field: &str) -> Value { + getattr(value, field).unwrap() } pub fn project_iterable(value: &Value) -> Vec { collect_iterable(value).unwrap() } -pub fn project_multi(value: &Value, field: &str) -> Vec { - getattr(value.as_ref(), field).unwrap() +pub fn project_multi(value: &PyObject, field: &str) -> Vec { + getattr(value, field).unwrap() } -pub fn project_multi_strs(value: &Value, field: &str) -> Vec { - getattr(value.as_ref(), field).unwrap() +pub fn project_multi_strs(value: &PyObject, field: &str) -> Vec { + getattr(value, field).unwrap() } -pub fn project_frozendict(value: &Value, field: &str) -> BTreeMap { - let frozendict = getattr(value.as_ref(), field).unwrap(); +pub fn project_frozendict(value: &PyObject, field: &str) -> BTreeMap { + let frozendict = getattr(value, field).unwrap(); let pydict: PyDict = getattr(&frozendict, "_data").unwrap(); let gil = Python::acquire_gil(); let py = gil.python(); @@ -255,26 +255,12 @@ pub fn project_frozendict(value: &Value, field: &str) -> BTreeMap String { +pub fn project_str(value: &PyObject, field: &str) -> String { // TODO: It's possible to view a python string as a `Cow`, so we could avoid actually // cloning in some cases. // TODO: We can't directly extract as a string here, because val_to_str defaults to empty string // for None. - val_to_str(&getattr(value.as_ref(), field).unwrap()) -} - -pub fn project_u64(value: &Value, field: &str) -> u64 { - getattr(value.as_ref(), field).unwrap() -} - -pub fn project_f64(value: &Value, field: &str) -> f64 { - getattr(value.as_ref(), field).unwrap() -} - -pub fn project_bytes(value: &Value, field: &str) -> Vec { - // TODO: It's possible to view a python bytes as a `&[u8]`, so we could avoid actually - // cloning in some cases. - getattr(value.as_ref(), field).unwrap() + val_to_str(&getattr(value, field).unwrap()) } pub fn key_to_str(key: &Key) -> String { @@ -282,7 +268,7 @@ pub fn key_to_str(key: &Key) -> String { } pub fn type_to_str(type_id: TypeId) -> String { - project_str(&type_for_type_id(type_id).into_object().into(), "__name__") + project_str(&type_for_type_id(type_id).into_object(), "__name__") } pub fn val_to_str(obj: &PyObject) -> String { diff --git a/src/rust/engine/src/intrinsics.rs b/src/rust/engine/src/intrinsics.rs index 1e3ed314a78..434b32b2ebe 100644 --- a/src/rust/engine/src/intrinsics.rs +++ b/src/rust/engine/src/intrinsics.rs @@ -356,10 +356,9 @@ fn create_digest_to_digest( .map_err(|e| format!("The `path` must be relative: {:?}", e))?; if externs::hasattr(file_content_or_directory.as_ref(), "content") { - let bytes = bytes::Bytes::from(externs::project_bytes( - &file_content_or_directory, - "content", - )); + let bytes = bytes::Bytes::from( + externs::getattr::>(&file_content_or_directory, "content").unwrap(), + ); let is_executable: bool = externs::getattr(&file_content_or_directory, "is_executable").unwrap(); diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index 711af1659c1..17165a8e25e 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -214,7 +214,7 @@ pub fn lift_directory_digest(types: &Types, digest: &Value) -> Result Result>()?; - let timeout_in_seconds = externs::project_f64(&value, "timeout_seconds"); + let timeout_in_seconds: f64 = externs::getattr(&value, "timeout_seconds").unwrap(); let timeout = if timeout_in_seconds < 0.0 { None