From 25591760618d7e36a93cc3637c78272d00d70aee Mon Sep 17 00:00:00 2001 From: Adam Cozzette Date: Fri, 13 Dec 2024 11:22:58 -0800 Subject: [PATCH] Rust: update protobuf_codegen crate to rely on separately installed protoc I also added a check to ensure that `protoc --version` matches the protobuf runtime version. The minitable generator plugin version should also match, but unfortunately we can't easily check this today because it does not provide a way to check its version. PiperOrigin-RevId: 705944904 --- rust/BUILD | 15 --- rust/release_crates/BUILD | 2 + rust/release_crates/cargo_test.sh | 6 +- rust/release_crates/protobuf_codegen/BUILD | 1 - .../protobuf_codegen/Cargo-template.toml | 5 +- .../protobuf_codegen/src/lib.rs | 127 +++++++++--------- upb_generator/minitable/BUILD | 2 +- 7 files changed, 79 insertions(+), 79 deletions(-) diff --git a/rust/BUILD b/rust/BUILD index 9ec6bc197f..99c1c95b5d 100644 --- a/rust/BUILD +++ b/rust/BUILD @@ -291,18 +291,3 @@ pkg_filegroup( prefix = "libupb", visibility = ["//rust/release_crates:__subpackages__"], ) - -# Bundle only the linux-x86_64 protoc for testing. -pkg_cross_compiled_binaries( - name = "vendored_protocs_test", - cpus = [ - "linux-x86_64", - ], - prefix = "bin", - tags = ["manual"], - targets = [ - "//upb_generator/minitable:protoc-gen-upb_minitable", - "//:protoc", - ], - visibility = ["//rust/release_crates:__subpackages__"], -) diff --git a/rust/release_crates/BUILD b/rust/release_crates/BUILD index 960eedf33f..ae90441d9c 100644 --- a/rust/release_crates/BUILD +++ b/rust/release_crates/BUILD @@ -8,6 +8,8 @@ sh_binary( "//rust/release_crates/protobuf:protobuf_crate", "//rust/release_crates/protobuf_codegen:protobuf_codegen_crate", "//rust/release_crates/protobuf_example:protobuf_example_crate", + "//src/google/protobuf/compiler:protoc", + "//upb_generator/minitable:protoc-gen-upb_minitable", ], tags = ["manual"], deps = ["@bazel_tools//tools/bash/runfiles"], diff --git a/rust/release_crates/cargo_test.sh b/rust/release_crates/cargo_test.sh index 598fe01990..54062a9a6d 100755 --- a/rust/release_crates/cargo_test.sh +++ b/rust/release_crates/cargo_test.sh @@ -56,7 +56,11 @@ mkdir $EXAMPLE_ROOT EXAMPLE_TAR=$(rlocation com_google_protobuf/rust/release_crates/protobuf_example/protobuf_example_crate.tar) echo "Expanding protobuf_example crate tar" -tar -xvf $EXAMPLE_TAR -C $EXAMPLE_ROOT +tar -xvf $EXAMPLE_TAR -C $EXAMPLE_ROOT + +# Put the Bazel-built protoc and plugin at the beginning of $PATH +PATH=$(dirname $(rlocation com_google_protobuf/protoc)):$PATH +PATH=$(dirname $(rlocation com_google_protobuf/upb_generator/minitable/protoc-gen-upb_minitable)):$PATH cd $CRATE_ROOT CARGO_HOME=$CARGO_HOME cargo test diff --git a/rust/release_crates/protobuf_codegen/BUILD b/rust/release_crates/protobuf_codegen/BUILD index e2ddf85dee..2c9f18bce8 100644 --- a/rust/release_crates/protobuf_codegen/BUILD +++ b/rust/release_crates/protobuf_codegen/BUILD @@ -7,7 +7,6 @@ pkg_tar( srcs = [ ":protobuf_codegen_files", "//:LICENSE", - "//rust:vendored_protocs_test", ], tags = ["manual"], visibility = ["//rust:__subpackages__"], diff --git a/rust/release_crates/protobuf_codegen/Cargo-template.toml b/rust/release_crates/protobuf_codegen/Cargo-template.toml index ef7275fa03..f9702c040e 100644 --- a/rust/release_crates/protobuf_codegen/Cargo-template.toml +++ b/rust/release_crates/protobuf_codegen/Cargo-template.toml @@ -9,4 +9,7 @@ rust-version = "1.74" [dependencies] -cc = "1.1.6" \ No newline at end of file +cc = "1.1.6" + +[dev-dependencies] +googletest = "0.12.0" diff --git a/rust/release_crates/protobuf_codegen/src/lib.rs b/rust/release_crates/protobuf_codegen/src/lib.rs index 68f394f07a..5a2d07828d 100644 --- a/rust/release_crates/protobuf_codegen/src/lib.rs +++ b/rust/release_crates/protobuf_codegen/src/lib.rs @@ -4,20 +4,49 @@ use std::path::{Path, PathBuf}; pub struct CodeGen { inputs: Vec, output_dir: PathBuf, - protoc_path: Option, - protoc_gen_upb_minitable_path: Option, includes: Vec, } const VERSION: &str = env!("CARGO_PKG_VERSION"); +// Given the output of "protoc --version", returns a shortened version string +// suitable for comparing against the protobuf crate version. +// +// The output of protoc --version looks something like "libprotoc XX.Y", +// optionally followed by "-dev" or "-rcN". We want to strip the "-dev" suffix +// if present and return something like "30.0" or "30.0-rc1". +fn protoc_version(protoc_output: &str) -> String { + let mut s = protoc_output.strip_prefix("libprotoc ").unwrap().to_string(); + let first_dash = s.find("-dev"); + if let Some(i) = first_dash { + s.truncate(i); + } + s +} + +// Given a crate version string, returns just the part of it suitable for +// comparing against the protoc version. The crate version is of the form +// "X.Y.Z" with an optional suffix starting with a dash. We want to drop the +// major version ("X.") and only keep the suffix if it starts with "-rc". +fn expected_protoc_version(cargo_version: &str) -> String { + let mut s = cargo_version.to_string(); + let is_release_candidate = s.find("-rc") != None; + if !is_release_candidate { + if let Some(i) = s.find('-') { + s.truncate(i); + } + } + let mut v: Vec<&str> = s.split('.').collect(); + assert_eq!(v.len(), 3); + v.remove(0); + v.join(".") +} + impl CodeGen { pub fn new() -> Self { Self { inputs: Vec::new(), output_dir: PathBuf::from(std::env::var("OUT_DIR").unwrap()).join("protobuf_generated"), - protoc_path: None, - protoc_gen_upb_minitable_path: None, includes: Vec::new(), } } @@ -37,20 +66,6 @@ impl CodeGen { self } - pub fn protoc_path(&mut self, protoc_path: impl AsRef) -> &mut Self { - self.protoc_path = Some(protoc_path.as_ref().to_owned()); - self - } - - pub fn protoc_gen_upb_minitable_path( - &mut self, - protoc_gen_upb_minitable_path: impl AsRef, - ) -> &mut Self { - self.protoc_gen_upb_minitable_path = - Some(protoc_gen_upb_minitable_path.as_ref().to_owned()); - self - } - pub fn include(&mut self, include: impl AsRef) -> &mut Self { self.includes.push(include.as_ref().to_owned()); self @@ -92,12 +107,22 @@ impl CodeGen { ); } - let protoc_path = if let Some(path) = &self.protoc_path { - path.clone() - } else { - protoc_path().expect("To be a supported platform") - }; - let mut cmd = std::process::Command::new(protoc_path); + let mut version_cmd = std::process::Command::new("protoc"); + let output = version_cmd + .arg("--version") + .output() + .map_err(|e| format!("failed to run protoc --version: {}", e))?; + + let protoc_version = protoc_version(&String::from_utf8(output.stdout).unwrap()); + let expected_protoc_version = expected_protoc_version(VERSION); + if protoc_version != expected_protoc_version { + panic!( + "Expected protoc version {} but found {}", + expected_protoc_version, protoc_version + ); + } + + let mut cmd = std::process::Command::new("protoc"); for input in &self.inputs { cmd.arg(input); } @@ -105,12 +130,6 @@ impl CodeGen { // Attempt to make the directory if it doesn't exist let _ = std::fs::create_dir(&self.output_dir); } - let protoc_gen_upb_minitable_path = if let Some(path) = &self.protoc_gen_upb_minitable_path - { - path.clone() - } else { - protoc_gen_upb_minitable_path().expect("To be a supported platform") - }; for include in &self.includes { println!("cargo:rerun-if-changed={}", include.display()); @@ -118,10 +137,6 @@ impl CodeGen { cmd.arg(format!("--rust_out={}", self.output_dir.display())) .arg("--rust_opt=experimental-codegen=enabled,kernel=upb") - .arg(format!( - "--plugin=protoc-gen-upb_minitable={}", - protoc_gen_upb_minitable_path.display() - )) .arg(format!("--upb_minitable_out={}", self.output_dir.display())); for include in &self.includes { cmd.arg(format!("--proto_path={}", include.display())); @@ -162,32 +177,24 @@ impl CodeGen { } } -fn get_path_for_arch() -> Option { - let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); - path.push("bin"); - match (std::env::consts::OS, std::env::consts::ARCH) { - ("macos", "x86_64") => path.push("osx-x86_64"), - ("macos", "aarch64") => path.push("osx-aarch_64"), - ("linux", "aarch64") => path.push("linux-aarch_64"), - ("linux", "powerpc64") => path.push("linux-ppcle_64"), - ("linux", "s390x") => path.push("linux-s390_64"), - ("linux", "x86") => path.push("linux-x86_32"), - ("linux", "x86_64") => path.push("linux-x86_64"), - ("windows", "x86") => path.push("win32"), - ("windows", "x86_64") => path.push("win64"), - _ => return None, - }; - Some(path) -} +#[cfg(test)] +mod tests { + use super::*; + use googletest::prelude::*; -pub fn protoc_path() -> Option { - let mut path = get_path_for_arch()?; - path.push("protoc"); - Some(path) -} + #[googletest::test] + fn test_protoc_version() { + assert_eq!(protoc_version("libprotoc 30.0"), "30.0"); + assert_eq!(protoc_version("libprotoc 30.0-dev"), "30.0"); + assert_eq!(protoc_version("libprotoc 30.0-rc1"), "30.0-rc1"); + } -pub fn protoc_gen_upb_minitable_path() -> Option { - let mut path = get_path_for_arch()?; - path.push("protoc-gen-upb_minitable"); - Some(path) + #[googletest::test] + fn test_expected_protoc_version() { + assert_eq!(expected_protoc_version("4.30.0"), "30.0"); + assert_eq!(expected_protoc_version("4.30.0-alpha"), "30.0"); + assert_eq!(expected_protoc_version("4.30.0-beta"), "30.0"); + assert_eq!(expected_protoc_version("4.30.0-pre"), "30.0"); + assert_eq!(expected_protoc_version("4.30.0-rc1"), "30.0-rc1"); + } } diff --git a/upb_generator/minitable/BUILD b/upb_generator/minitable/BUILD index a5eadfed93..83e75b0d7d 100644 --- a/upb_generator/minitable/BUILD +++ b/upb_generator/minitable/BUILD @@ -93,7 +93,7 @@ bootstrap_cc_binary( visibility = [ "//editions/codegen_tests:__pkg__", "//net/proto2/contrib/protoc_explorer:__pkg__", - "//rust:__pkg__", + "//rust:__subpackages__", "//third_party/prototiller/transformer:__pkg__", ], )