From b9f54d56eec57190006cba45377a93da3c17c6fa Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Wed, 28 Jul 2021 18:33:14 -0700 Subject: [PATCH 1/2] WIP [internal] Allow going from Rust-CPython pointers to PyO3 pyclass structs --- src/rust/engine/Cargo.lock | 1 + src/rust/engine/Cargo.toml | 1 + src/rust/engine/src/externs/mod.rs | 10 ++++++++++ 3 files changed, 12 insertions(+) diff --git a/src/rust/engine/Cargo.lock b/src/rust/engine/Cargo.lock index aed335bea10..0ba64266639 100644 --- a/src/rust/engine/Cargo.lock +++ b/src/rust/engine/Cargo.lock @@ -687,6 +687,7 @@ dependencies = [ "parking_lot", "petgraph", "process_execution", + "pyo3", "rand 0.8.2", "regex", "reqwest", diff --git a/src/rust/engine/Cargo.toml b/src/rust/engine/Cargo.toml index 62bf9e35bfa..1c755e5414a 100644 --- a/src/rust/engine/Cargo.toml +++ b/src/rust/engine/Cargo.toml @@ -126,6 +126,7 @@ num_enum = "0.4" parking_lot = "0.11" petgraph = "0.5" process_execution = { path = "process_execution" } +pyo3 = "0.14.1" rand = "0.8" regex = "1" reqwest = { version = "0.11", default_features = false, features = ["stream", "rustls-tls"] } diff --git a/src/rust/engine/src/externs/mod.rs b/src/rust/engine/src/externs/mod.rs index f2be13cebf6..afc5af4238c 100644 --- a/src/rust/engine/src/externs/mod.rs +++ b/src/rust/engine/src/externs/mod.rs @@ -28,6 +28,7 @@ use cpython::{ PyObject, PyResult as CPyResult, PyTuple, PyType, Python, PythonObject, ToPyObject, }; use lazy_static::lazy_static; +use pyo3::FromPyPointer; use logging::PythonLogLevel; @@ -420,3 +421,12 @@ pub enum GeneratorResponse { Get(Get), GetMulti(Vec), } + +pub(crate) fn from_rust_cpython_to_pyo3(value: &PyObject) -> &pyo3::PyAny { + let pyo3_val = pyo3::Python::with_gil(|py| unsafe { + pyo3::PyAny::from_owned_ptr(py, value.as_ptr() as *mut pyo3::ffi::PyObject) + }); + // Forget the first instance, since the second instance has taken ownership. + std::mem::forget(value); + pyo3_val +} From 868cca353fa7d6097e4c8528b42feed7779ad3bb Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Fri, 30 Jul 2021 13:31:15 -0700 Subject: [PATCH 2/2] wip to reorg files # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/rust/engine/Cargo.lock | 8 ++++ src/rust/engine/Cargo.toml | 2 + src/rust/engine/engine_pyo3/Cargo.toml | 12 ------ .../engine/engine_pyo3/src/externs/mod.rs | 4 -- .../src/{externs/interface => }/fs.rs | 2 +- src/rust/engine/engine_pyo3/src/lib.rs | 6 ++- .../src/{externs/interface => }/nailgun.rs | 5 ++- .../{externs/interface.rs => scheduler.rs} | 16 +------- .../src/{externs/interface => }/testutil.rs | 5 ++- .../src/{externs/interface => }/workunits.rs | 3 +- .../engine/engine_pyo3_extension/Cargo.toml | 22 ++++++++++ .../engine/engine_pyo3_extension/build.rs | 34 ++++++++++++++++ .../engine/engine_pyo3_extension/src/lib.rs | 40 +++++++++++++++++++ src/rust/engine/src/externs/mod.rs | 12 +++--- 14 files changed, 127 insertions(+), 44 deletions(-) delete mode 100644 src/rust/engine/engine_pyo3/src/externs/mod.rs rename src/rust/engine/engine_pyo3/src/{externs/interface => }/fs.rs (99%) rename src/rust/engine/engine_pyo3/src/{externs/interface => }/nailgun.rs (95%) rename src/rust/engine/engine_pyo3/src/{externs/interface.rs => scheduler.rs} (63%) rename src/rust/engine/engine_pyo3/src/{externs/interface => }/testutil.rs (94%) rename src/rust/engine/engine_pyo3/src/{externs/interface => }/workunits.rs (85%) create mode 100644 src/rust/engine/engine_pyo3_extension/Cargo.toml create mode 100644 src/rust/engine/engine_pyo3_extension/build.rs create mode 100644 src/rust/engine/engine_pyo3_extension/src/lib.rs diff --git a/src/rust/engine/Cargo.lock b/src/rust/engine/Cargo.lock index 0ba64266639..8d1be1a02d3 100644 --- a/src/rust/engine/Cargo.lock +++ b/src/rust/engine/Cargo.lock @@ -725,6 +725,14 @@ dependencies = [ "workunit_store", ] +[[package]] +name = "engine_pyo3_extension" +version = "0.0.1" +dependencies = [ + "engine_pyo3", + "pyo3", +] + [[package]] name = "env_logger" version = "0.5.13" diff --git a/src/rust/engine/Cargo.toml b/src/rust/engine/Cargo.toml index 1c755e5414a..37447669c11 100644 --- a/src/rust/engine/Cargo.toml +++ b/src/rust/engine/Cargo.toml @@ -25,6 +25,7 @@ members = [ "client", "concrete_time", "engine_pyo3", + "engine_pyo3_extension", "fs", "fs/brfs", "fs/fs_util", @@ -65,6 +66,7 @@ default-members = [ "async_value", "concrete_time", "engine_pyo3", + "engine_pyo3_extension", "fs", "fs/fs_util", "fs/store", diff --git a/src/rust/engine/engine_pyo3/Cargo.toml b/src/rust/engine/engine_pyo3/Cargo.toml index 37b20544f12..da651f94019 100644 --- a/src/rust/engine/engine_pyo3/Cargo.toml +++ b/src/rust/engine/engine_pyo3/Cargo.toml @@ -5,18 +5,6 @@ name = "engine_pyo3" authors = [ "Pants Build " ] publish = false -[lib] -crate-type = ["cdylib"] - -[features] -# NB: To actually load this crate from python, the `extension-module` feature must be enabled. But -# unfortunately, enabling `extension-module` causes tests linked against `pyo3` to fail. We -# define a feature to enable that, but we do not enable it by default: someone building this module -# in order to extract `libengine_pyo3.so` should pass `cargo build .. --features=engine_pyo3/extension-module`. -# see https://github.com/PyO3/pyo3/issues/340 -extension-module = ["pyo3/extension-module"] -default = [] - [dependencies] fs = { path = "../fs" } hashing = { path = "../hashing" } diff --git a/src/rust/engine/engine_pyo3/src/externs/mod.rs b/src/rust/engine/engine_pyo3/src/externs/mod.rs deleted file mode 100644 index 770301dcaa7..00000000000 --- a/src/rust/engine/engine_pyo3/src/externs/mod.rs +++ /dev/null @@ -1,4 +0,0 @@ -// Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). -// Licensed under the Apache License, Version 2.0 (see LICENSE). - -mod interface; diff --git a/src/rust/engine/engine_pyo3/src/externs/interface/fs.rs b/src/rust/engine/engine_pyo3/src/fs.rs similarity index 99% rename from src/rust/engine/engine_pyo3/src/externs/interface/fs.rs rename to src/rust/engine/engine_pyo3/src/fs.rs index 15649db26ac..536d0d606ce 100644 --- a/src/rust/engine/engine_pyo3/src/externs/interface/fs.rs +++ b/src/rust/engine/engine_pyo3/src/fs.rs @@ -14,7 +14,7 @@ use fs::{GlobExpansionConjunction, PathGlobs, PreparedPathGlobs, StrictGlobMatch use hashing::{Digest, Fingerprint}; use store::Snapshot; -pub(crate) fn register(m: &PyModule) -> PyResult<()> { +pub fn register(m: &PyModule) -> PyResult<()> { m.add_function(wrap_pyfunction!(match_path_globs, m)?)?; m.add_function(wrap_pyfunction!(default_cache_path, m)?)?; m.add_class::()?; diff --git a/src/rust/engine/engine_pyo3/src/lib.rs b/src/rust/engine/engine_pyo3/src/lib.rs index 5d6c11b4051..550254c3755 100644 --- a/src/rust/engine/engine_pyo3/src/lib.rs +++ b/src/rust/engine/engine_pyo3/src/lib.rs @@ -32,4 +32,8 @@ #![allow(clippy::not_unsafe_ptr_arg_deref)] #![type_length_limit = "43757804"] -mod externs; +pub mod fs; +pub mod nailgun; +pub mod scheduler; +pub mod testutil; +pub mod workunits; diff --git a/src/rust/engine/engine_pyo3/src/externs/interface/nailgun.rs b/src/rust/engine/engine_pyo3/src/nailgun.rs similarity index 95% rename from src/rust/engine/engine_pyo3/src/externs/interface/nailgun.rs rename to src/rust/engine/engine_pyo3/src/nailgun.rs index fa1352024ef..642ac223385 100644 --- a/src/rust/engine/engine_pyo3/src/externs/interface/nailgun.rs +++ b/src/rust/engine/engine_pyo3/src/nailgun.rs @@ -1,14 +1,15 @@ // Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). // Licensed under the Apache License, Version 2.0 (see LICENSE). -use super::PyExecutor; use pyo3::create_exception; use pyo3::exceptions::{PyBrokenPipeError, PyException, PyKeyboardInterrupt}; use pyo3::prelude::*; use pyo3::types::PyDict; + +use crate::scheduler::PyExecutor; use task_executor::Executor; -pub(crate) fn register(py: Python, m: &PyModule) -> PyResult<()> { +pub fn register(py: Python, m: &PyModule) -> PyResult<()> { m.add( "PantsdConnectionException", py.get_type::(), diff --git a/src/rust/engine/engine_pyo3/src/externs/interface.rs b/src/rust/engine/engine_pyo3/src/scheduler.rs similarity index 63% rename from src/rust/engine/engine_pyo3/src/externs/interface.rs rename to src/rust/engine/engine_pyo3/src/scheduler.rs index 25db6ed6403..45669bf4aff 100644 --- a/src/rust/engine/engine_pyo3/src/externs/interface.rs +++ b/src/rust/engine/engine_pyo3/src/scheduler.rs @@ -4,26 +4,14 @@ use pyo3::exceptions::PyException; use pyo3::prelude::*; -mod fs; -mod nailgun; -mod testutil; -mod workunits; - -#[pymodule] -fn native_engine_pyo3(py: Python, m: &PyModule) -> PyResult<()> { - self::fs::register(m)?; - self::nailgun::register(py, m)?; - self::testutil::register(m)?; - self::workunits::register(m)?; - +pub fn register(m: &PyModule) -> PyResult<()> { m.add_class::()?; - Ok(()) } #[pyclass] #[derive(Debug, Clone)] -struct PyExecutor(task_executor::Executor); +pub struct PyExecutor(pub task_executor::Executor); #[pymethods] impl PyExecutor { diff --git a/src/rust/engine/engine_pyo3/src/externs/interface/testutil.rs b/src/rust/engine/engine_pyo3/src/testutil.rs similarity index 94% rename from src/rust/engine/engine_pyo3/src/externs/interface/testutil.rs rename to src/rust/engine/engine_pyo3/src/testutil.rs index 4d4b1ab10ed..14fc31ce11e 100644 --- a/src/rust/engine/engine_pyo3/src/externs/interface/testutil.rs +++ b/src/rust/engine/engine_pyo3/src/testutil.rs @@ -3,14 +3,15 @@ use std::sync::Arc; -use super::PyExecutor; use parking_lot::Mutex; use pyo3::exceptions::PyAssertionError; use pyo3::prelude::*; use pyo3::types::PyType; + +use crate::scheduler::PyExecutor; use testutil_mock::{StubCAS, StubCASBuilder}; -pub(crate) fn register(m: &PyModule) -> PyResult<()> { +pub fn register(m: &PyModule) -> PyResult<()> { m.add_class::()?; m.add_class::()?; Ok(()) diff --git a/src/rust/engine/engine_pyo3/src/externs/interface/workunits.rs b/src/rust/engine/engine_pyo3/src/workunits.rs similarity index 85% rename from src/rust/engine/engine_pyo3/src/externs/interface/workunits.rs rename to src/rust/engine/engine_pyo3/src/workunits.rs index 0ef97a3a703..535bb0ed354 100644 --- a/src/rust/engine/engine_pyo3/src/externs/interface/workunits.rs +++ b/src/rust/engine/engine_pyo3/src/workunits.rs @@ -2,9 +2,10 @@ // Licensed under the Apache License, Version 2.0 (see LICENSE). use pyo3::prelude::*; + use workunit_store::Metric; -pub(crate) fn register(m: &PyModule) -> PyResult<()> { +pub fn register(m: &PyModule) -> PyResult<()> { m.add_function(wrap_pyfunction!(all_counter_names, m)?)?; Ok(()) } diff --git a/src/rust/engine/engine_pyo3_extension/Cargo.toml b/src/rust/engine/engine_pyo3_extension/Cargo.toml new file mode 100644 index 00000000000..b7b266a2596 --- /dev/null +++ b/src/rust/engine/engine_pyo3_extension/Cargo.toml @@ -0,0 +1,22 @@ +[package] +version = "0.0.1" +edition = "2018" +name = "engine_pyo3_extension" +authors = [ "Pants Build " ] +publish = false + +[lib] +crate-type = ["cdylib"] + +[features] +# NB: To actually load this crate from python, the `extension-module` feature must be enabled. But +# unfortunately, enabling `extension-module` causes tests linked against `pyo3` to fail. We +# define a feature to enable that, but we do not enable it by default: someone building this module +# in order to extract `libengine_pyo3.so` should pass `cargo build .. --features=engine_pyo3/extension-module`. +# see https://github.com/PyO3/pyo3/issues/340 +extension-module = ["pyo3/extension-module"] +default = [] + +[dependencies] +engine_pyo3 = { "path" = "../engine_pyo3" } +pyo3 = "0.14.1" diff --git a/src/rust/engine/engine_pyo3_extension/build.rs b/src/rust/engine/engine_pyo3_extension/build.rs new file mode 100644 index 00000000000..f6d174b1240 --- /dev/null +++ b/src/rust/engine/engine_pyo3_extension/build.rs @@ -0,0 +1,34 @@ +// Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). +// Licensed under the Apache License, Version 2.0 (see LICENSE). + +#![deny(warnings)] +// Enable all clippy lints except for many of the pedantic ones. It's a shame this needs to be copied and pasted across crates, but there doesn't appear to be a way to include inner attributes from a common source. +#![deny( + clippy::all, + clippy::default_trait_access, + clippy::expl_impl_clone_on_copy, + clippy::if_not_else, + clippy::needless_continue, + clippy::unseparated_literal_suffix, + // TODO: Falsely triggers for async/await: + // see https://github.com/rust-lang/rust-clippy/issues/5360 + // clippy::used_underscore_binding +)] +// It is often more clear to show that nothing is being moved. +#![allow(clippy::match_ref_pats)] +// Subjective style. +#![allow( + clippy::len_without_is_empty, + clippy::redundant_field_names, + clippy::too_many_arguments +)] +// Default isn't as big a deal as people seem to think it is. +#![allow(clippy::new_without_default, clippy::new_ret_no_self)] +// Arc can be more clear than needing to grok Orderings: +#![allow(clippy::mutex_atomic)] + +fn main() { + // NB: The native extension only works with the Python interpreter version it was built with + // (e.g. Python 3.7 vs 3.8). + println!("cargo:rerun-if-env-changed=PY"); +} diff --git a/src/rust/engine/engine_pyo3_extension/src/lib.rs b/src/rust/engine/engine_pyo3_extension/src/lib.rs new file mode 100644 index 00000000000..e9f6903aa85 --- /dev/null +++ b/src/rust/engine/engine_pyo3_extension/src/lib.rs @@ -0,0 +1,40 @@ +// Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). +// Licensed under the Apache License, Version 2.0 (see LICENSE). + +#![deny(warnings)] +// Enable all clippy lints except for many of the pedantic ones. It's a shame this needs to be copied and pasted across crates, but there doesn't appear to be a way to include inner attributes from a common source. +#![deny( + clippy::all, + clippy::default_trait_access, + clippy::expl_impl_clone_on_copy, + clippy::if_not_else, + clippy::needless_continue, + clippy::unseparated_literal_suffix, + // TODO: Falsely triggers for async/await: + // see https://github.com/rust-lang/rust-clippy/issues/5360 + // clippy::used_underscore_binding +)] +// It is often more clear to show that nothing is being moved. +#![allow(clippy::match_ref_pats)] +// Subjective style. +#![allow( + clippy::len_without_is_empty, + clippy::redundant_field_names, + clippy::too_many_arguments +)] +// Default isn't as big a deal as people seem to think it is. +#![allow(clippy::new_without_default, clippy::new_ret_no_self)] +// Arc can be more clear than needing to grok Orderings: +#![allow(clippy::mutex_atomic)] + +use pyo3::prelude::*; + +#[pymodule] +fn native_engine_pyo3(py: Python, m: &PyModule) -> PyResult<()> { + engine_pyo3::fs::register(m)?; + engine_pyo3::nailgun::register(py, m)?; + engine_pyo3::scheduler::register(m)?; + engine_pyo3::testutil::register(m)?; + engine_pyo3::workunits::register(m)?; + Ok(()) +} diff --git a/src/rust/engine/src/externs/mod.rs b/src/rust/engine/src/externs/mod.rs index afc5af4238c..1e4360c3de1 100644 --- a/src/rust/engine/src/externs/mod.rs +++ b/src/rust/engine/src/externs/mod.rs @@ -422,11 +422,9 @@ pub enum GeneratorResponse { GetMulti(Vec), } -pub(crate) fn from_rust_cpython_to_pyo3(value: &PyObject) -> &pyo3::PyAny { - let pyo3_val = pyo3::Python::with_gil(|py| unsafe { - pyo3::PyAny::from_owned_ptr(py, value.as_ptr() as *mut pyo3::ffi::PyObject) - }); - // Forget the first instance, since the second instance has taken ownership. - std::mem::forget(value); - pyo3_val +pub(crate) fn from_rust_cpython_to_pyo3<'p>( + value: &PyObject, + py: pyo3::Python<'p>, +) -> &'p pyo3::PyAny { + unsafe { pyo3::PyAny::from_owned_ptr(py, value.as_ptr() as *mut pyo3::ffi::PyObject) } }