Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose getattr method for Python-related APIs #10953

Merged
merged 2 commits into from
Oct 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/rust/engine/src/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/rust/engine/src/externs/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
}
Expand Down
42 changes: 12 additions & 30 deletions src/rust/engine/src/externs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>(value: &PyObject, field: &str) -> Result<T, String>
pub fn getattr<T>(value: &PyObject, field: &str) -> Result<T, String>
where
for<'a> T: FromPyObject<'a>,
{
Expand Down Expand Up @@ -227,28 +227,24 @@ fn collect_iterable(value: &Value) -> Result<Vec<Value>, 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<Value> {
collect_iterable(value).unwrap()
}

pub fn project_multi(value: &Value, field: &str) -> Vec<Value> {
getattr(value.as_ref(), field).unwrap()
pub fn project_multi(value: &PyObject, field: &str) -> Vec<Value> {
getattr(value, field).unwrap()
}

pub fn project_bool(value: &Value, field: &str) -> bool {
getattr(value.as_ref(), field).unwrap()
pub fn project_multi_strs(value: &PyObject, field: &str) -> Vec<String> {
getattr(value, field).unwrap()
}

pub fn project_multi_strs(value: &Value, field: &str) -> Vec<String> {
getattr(value.as_ref(), field).unwrap()
}

pub fn project_frozendict(value: &Value, field: &str) -> BTreeMap<String, String> {
let frozendict = getattr(value.as_ref(), field).unwrap();
pub fn project_frozendict(value: &PyObject, field: &str) -> BTreeMap<String, String> {
let frozendict = getattr(value, field).unwrap();
let pydict: PyDict = getattr(&frozendict, "_data").unwrap();
let gil = Python::acquire_gil();
let py = gil.python();
Expand All @@ -259,34 +255,20 @@ pub fn project_frozendict(value: &Value, field: &str) -> BTreeMap<String, String
.collect()
}

pub fn project_str(value: &Value, field: &str) -> String {
pub fn project_str(value: &PyObject, field: &str) -> String {
// TODO: It's possible to view a python string as a `Cow<str>`, 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<u8> {
// 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 {
val_to_str(&val_for(key).as_ref())
}

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 {
Expand Down
10 changes: 5 additions & 5 deletions src/rust/engine/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,11 +356,11 @@ 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 is_executable = externs::project_bool(&file_content_or_directory, "is_executable");
let bytes = bytes::Bytes::from(
externs::getattr::<Vec<u8>>(&file_content_or_directory, "content").unwrap(),
);
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
Expand Down
10 changes: 5 additions & 5 deletions src/rust/engine/src/nodes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ pub fn lift_directory_digest(types: &Types, digest: &Value) -> Result<hashing::D
));
}
let fingerprint = externs::project_str(&digest, "fingerprint");
let digest_length = externs::project_u64(&digest, "serialized_bytes_length") as usize;
let digest_length: usize = externs::getattr(&digest, "serialized_bytes_length").unwrap();
Ok(hashing::Digest(
hashing::Fingerprint::from_hex_string(&fingerprint)?,
digest_length,
Expand All @@ -226,7 +226,7 @@ pub fn lift_file_digest(types: &Types, digest: &Value) -> Result<hashing::Digest
return Err(format!("{} is not of type {}.", digest, types.file_digest));
}
let fingerprint = externs::project_str(&digest, "fingerprint");
let digest_length = externs::project_u64(&digest, "serialized_bytes_length") as usize;
let digest_length: usize = externs::getattr(&digest, "serialized_bytes_length").unwrap();
Ok(hashing::Digest(
hashing::Fingerprint::from_hex_string(&fingerprint)?,
digest_length,
Expand Down Expand Up @@ -274,7 +274,7 @@ impl MultiPlatformExecuteProcess {
.map(RelativePath::new)
.collect::<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
Expand All @@ -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");
Expand All @@ -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"),
Expand Down