Split external-proto into two features

- `generate-proto`: when enabled, proto.rs is generated at build time. When it is disabled, it is presumed that proto.rs has already been generated - `protobuf-src`: when enabled, protoc is built from source using a build-dependency on protobuf-src. When it is disabled, `PROTOC` must be set by the builder in the environment to a protobuf compiler Signed-off-by: Tom Jakubowski <[email protected]>
finos · Oct 18, 2024 · f9ca424 · f9ca424
1 parent f8a5798
commit f9ca424
Show file tree

Hide file tree

Showing 7 changed files with 35 additions and 13 deletions.
diff --git a/rust/generate-metadata/Cargo.toml b/rust/generate-metadata/Cargo.toml
@@ -30,8 +30,8 @@ path = "../perspective-server"
 features = ["external-cpp", "disable-cpp"]
 
 [dependencies.perspective-client]
+features = ["generate-proto", "omit_metadata", "protobuf-src"]
 path = "../perspective-client"
-features = ["external-proto", "omit_metadata"]
 
 [dependencies.perspective-js]
 path = "../perspective-js"

diff --git a/rust/perspective-client/Cargo.toml b/rust/perspective-client/Cargo.toml
@@ -38,7 +38,10 @@ default = []
 
 # Should the project build the `proto.rs` via protoc from source or assume it
 # already exists?
-external-proto = ["protobuf-src"]
+generate-proto = []
+# Should the project build protobuf from source? If not, building this crate
+# requires PROTOC be set in the environment
+protobuf-src = ["dep:protobuf-src"]
 
 # When generating metadata, we can't rely on its existence - so enable this
 # to skip metadata generation. This currently only affects docs.

diff --git a/rust/perspective-client/build.rs b/rust/perspective-client/build.rs
@@ -24,7 +24,7 @@ fn prost_build() -> Result<()> {
     // This source file is included at `publish` time, but not `sbuild` time
     // because it is initially generated from the `perspective.proto` definition
     // in the C++ source.
-    if std::env::var("CARGO_FEATURE_EXTERNAL_PROTO").is_ok() {
+    if std::env::var("CARGO_FEATURE_GENERATE_PROTO").is_ok() {
         println!("cargo:warning=MESSAGE Building in development mode");
         let root_dir_env = std::env::var("PSP_ROOT_DIR").expect("Must set PSP_ROOT_DIR");
         let root_dir = Path::new(root_dir_env.as_str());
@@ -36,8 +36,19 @@ fn prost_build() -> Result<()> {
 
         println!("cargo:rerun-if-changed={}", proto_file.to_str().unwrap());
 
-        #[cfg(feature = "external-proto")]
+        // prost_build reads PROTOC from the environment.  When the `protobuf-src`
+        // feature is enabled, the build script sets PROTOC to the one built by
+        // that crate. When protobuf-src is disabled, builders must set PROTOC
+        // in the environment to a protocol buffer compiler.
+        #[cfg(feature = "protobuf-src")]
         std::env::set_var("PROTOC", protobuf_src::protoc());
+        #[cfg(not(feature = "protobuf-src"))]
+        if std::env::var("PROTOC").is_err() {
+            panic!(
+                "generate-proto is enabled and protobuf-src is disabled.  PROTOC must be set in \
+                 the environment to the path of a protocol buffer compiler"
+            )
+        }
 
         prost_build::Config::new()
             // .bytes(["ViewToArrowResp.arrow", "from_arrow"])

diff --git a/rust/perspective-js/Cargo.toml b/rust/perspective-js/Cargo.toml
@@ -34,7 +34,10 @@ path = "src/rust/lib.rs"
 export-init = []
 metadata = []
 default = []
-external-cpp = ["perspective-client/external-proto"]
+external-cpp = [
+    "perspective-client/generate-proto",
+    "perspective-client/protobuf-src",
+]
 
 [build-dependencies]
 serde_json = { version = "1.0.107", features = ["raw_value"] }

diff --git a/rust/perspective-python/Cargo.toml b/rust/perspective-python/Cargo.toml
@@ -37,11 +37,10 @@ rustdoc-args = ["--html-in-header", "docs/index.html"]
 
 [features]
 default = []
-external-cpp = [
-    "perspective-client/external-proto",
-    "perspective-server/external-cpp",
-]
 abi3 = ["pyo3/abi3-py39"]
+external-cpp = ["perspective-server/external-cpp"]
+generate-proto = ["perspective-client/generate-proto"]
+protobuf-src = ["perspective-client/protobuf-src"]
 
 [lib]
 crate-type = ["cdylib"]

diff --git a/rust/perspective-python/build.mjs b/rust/perspective-python/build.mjs
@@ -89,7 +89,7 @@ if (build_wheel) {
     }
 
     if (process.env.CONDA_BUILD === "1") {
-        console.log("Building with Conda flags for maturin");
+        console.log("Building with Conda flags and features");
         if (process.env.PYTHON) {
             console.log(`interpreter: ${process.env.PYTHON}`);
             flags += ` --interpreter=${process.env.PYTHON}`;
@@ -98,9 +98,14 @@ if (build_wheel) {
                 "Expected PYTHON to be set in CONDA_BUILD environment, but it isn't.  maturin will likely detect the wrong Python."
             );
         }
+        // we need to generate proto.rs using conda's protoc, which is set in
+        // the environment.  we use the unstable "versioned" python abi
+        features.push(["generate-proto"]);
     } else {
-        console.log("Building with regular flags for maturin");
-        features.push("abi3");
+        // standard for in-repo builds.  a different set will be standard in the sdist
+        const standard_features = ["abi3", "generate-proto", "protobuf-src"];
+        console.log("Building with standard flags and features");
+        features.push(...standard_features);
     }
 
     cmd.sh(`maturin build ${flags} --features=${features.join(",")} ${target}`);

diff --git a/rust/perspective/Cargo.toml b/rust/perspective/Cargo.toml
@@ -32,7 +32,8 @@ default = ["axum-ws"]
 axum-ws = ["tokio", "axum", "futures"]
 external-cpp = [
     "perspective-server/external-cpp",
-    "perspective-client/external-proto",
+    "perspective-client/generate-proto",
+    "perspective-client/protobuf-src",
 ]
 
 [dependencies]