grpc: crate maintenance#2654
Conversation
| // If a prebuilt binary directory is specified but was empty, save the newly compiled | ||
| // binaries there so they can be cached and reused. |
There was a problem hiding this comment.
The cargo docs discourage writing artifacts outside of OUT_DIR. I suspect this may also lead to race conditions if there are multiple versions of the crate being built concurrently.
Is this being done only to speed up CI? I feel it's better to manually build the binaries using CMake once in CI instead of having a flag in the production build script to support this. Please let me know if you think otherwise.
Also, why does the regular Rust build cache not work in CI? Is it something that can be addressed by spending more time on the configuration, or is it a limitation in Swatinem/rust-cache@v2?
There was a problem hiding this comment.
There were many issues with the Swatinem/rust-cache@v2 crate. First, I had to give it a shared cache location, and I had to set cache-workspace-crates. I ran into more problems, and ultimately I couldn't get it working, and also the cache files were very large. I think maybe we should revert back to the manual caching that it was doing before, and just change the way the compilation worked to use cargo instead of cmake from the commandline?
There was a problem hiding this comment.
So it's easy enough to build this crate, then find the binary locations from the OUT_DIR (there's functions in lib.rs to tell us that). So we can find them and copy them out. But I think we have to be able to read them from the cache location and copy them back from there as part of build.rs -- if we're doing it outside of the build process, I don't know how we can determine where we should put them, and compiling the crate to call the functions it has would require building them. So I guess we can't completely go back to the way it was before. We could move the copying-out into it, though, though I suspect it's fine.
Also maybe we should publish our binary and download it / protoc instead of building by default?
There was a problem hiding this comment.
Also maybe we should publish our binary and download it / protoc instead of building by default?
In the present state, only the first build takes a while to compile protobuf from source, subsequent builds are using cargo's cache. Maybe the first build times are not a huge problem? If it is, we could add such an option in the future.
There was a problem hiding this comment.
It's not the time alone; it's also the C++/cmake dependencies. But regarding the time: because Rust puts the target directory local -- unlike Go -- if you're using grpc in 5 different projects (or if you use worktrees like I do, and don't manually configure things like I do so your target directory is shared), then you will have to rebuild it in every directory.
f53df38 to
9613c62
Compare
40b30f9 to
a356b0a
Compare
|
PR is updated. Here is the current state:
LMK what you think. |
arjan-bal
left a comment
There was a problem hiding this comment.
Mostly looks good, some minor comments.
| fn find_in_path(binary_name: &str) -> Option<PathBuf> { | ||
| if let Some(paths) = std::env::var_os("PATH") { | ||
| for path in std::env::split_paths(&paths) { | ||
| let candidate = path.join(binary_name); | ||
| if candidate.is_file() { | ||
| return Some(candidate); | ||
| } | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
This may fail when the fix exists but is not executable or when running on windows which has a PATHEXT environment variable and may check the cwd. Gemini mentioned that the which crate is the idiomatic way of finding binary paths. We could use it to simplify our code.
Instead of checking, we could also call the protoc or protoc-gen-rust-grpc binaries with the --version flag to check if the binaries are available.
There was a problem hiding this comment.
Changed to use which and to execute the commands.
| let compiled_protoc = PathBuf::from(protoc_gen_rust_grpc::protoc()); | ||
| let compiled_plugin = PathBuf::from(protoc_gen_rust_grpc::protoc_gen_rust_grpc()); | ||
| if compiled_protoc.exists() && compiled_plugin.exists() { | ||
| // The files may not exist if a build setting instructed |
There was a problem hiding this comment.
Is this sentence incomplete?
There was a problem hiding this comment.
Oops I forgot to finish my thought. Finished.
| are compiled, and the functions `protoc` and `protoc_gen_rust_grpc` can be used | ||
| to find their locations. | ||
|
|
||
| ## Building binaries manually |
There was a problem hiding this comment.
Just a thought that came up while reviewing the code: We could create a Rust binary crate that statically links to the C++ code (compiled via the cmake crate). The Rust binary would act as a thin wrapper, using FFI to forward command-line arguments to the C++ logic. This way, users can seamlessly build and install the protoc plugin using cargo install rather than invoking cmake manually.
There was a problem hiding this comment.
Oh I like that idea! Except that we need two binaries for now. I talked to Em and he's open to the idea of publishing something similar for protoc itself.
There was a problem hiding this comment.
(Sorry, to be clear; he was open to publishing something that can build protoc so we don't have to have something that builds both protoc and our plugin. I haven't talked to him about the FFI idea yet.)
| break | ||
| mkdir -p protoc-cache | ||
| # Find where cmake built the binaries. | ||
| find target/ -name "protoc" -o -name "protoc.exe" -o -name "protoc-gen-rust-grpc" -o -name "protoc-gen-rust-grpc.exe" | while read -r file; do |
There was a problem hiding this comment.
Since we're building protoc from source, we shouldn't need taiki-e/install-action@protoc, right?
There was a problem hiding this comment.
Definitely not while building the plugin. I removed it from there. But the other ones might, because AIUI tonic requires protoc in your path. But maybe our CI doesn't except for the codegen job? I'll play around with this some more.
There was a problem hiding this comment.
Aha! It's needed for the other jobs because the install rule includes standard proto include files -- not just the protoc binary.
|
|
||
| [build-dependencies] | ||
| tonic-prost-build = { path = "../tonic-prost-build" } | ||
| grpc-protobuf-build = { path = "../grpc-protobuf-build", default-features = false } |
There was a problem hiding this comment.
Since the feature flags for the gRPC examples are enabled by default and the grpc-protobuf-build feature is disabled, running the Tonic examples now requires protoc and protoc-gen-rust-grpc to be installed and in the user's PATH. For instance, to run the Tonic routeguide server, I had to execute:
cargo run --bin routeguide-server --no-default-features --features=routeguideIs this UX acceptable for Tonic users? If we want to improve this, we have a couple of options:
- Use
required-features: Disable the gRPC example features by default and addrequired-features` to each gRPC example binary. This ensures Cargo prints a clear error message instructing users to enable the exact features required. - Use a separate crate: Move the gRPC examples into a dedicated crate that has the necessary gRPC features enabled by default.
There was a problem hiding this comment.
Thanks for pointing this out. I'd actually like even the grpc examples to be runnable without having proto in your path. So what I've done is added an env var (GRPC_RUST_REGENERATE_PROTO) to build.rs to cause it to regenerate the protobuf output. And if it's not set, then nothing is regenerated and nothing fails if the binaries aren't there.
| // If a prebuilt binary directory is specified but was empty, save the newly compiled | ||
| // binaries there so they can be cached and reused. |
There was a problem hiding this comment.
Also maybe we should publish our binary and download it / protoc instead of building by default?
In the present state, only the first build takes a while to compile protobuf from source, subsequent builds are using cargo's cache. Maybe the first build times are not a huge problem? If it is, we could add such an option in the future.
dfawley
left a comment
There was a problem hiding this comment.
Thanks for the review!
| let compiled_protoc = PathBuf::from(protoc_gen_rust_grpc::protoc()); | ||
| let compiled_plugin = PathBuf::from(protoc_gen_rust_grpc::protoc_gen_rust_grpc()); | ||
| if compiled_protoc.exists() && compiled_plugin.exists() { | ||
| // The files may not exist if a build setting instructed |
There was a problem hiding this comment.
Oops I forgot to finish my thought. Finished.
|
|
||
| [build-dependencies] | ||
| tonic-prost-build = { path = "../tonic-prost-build" } | ||
| grpc-protobuf-build = { path = "../grpc-protobuf-build", default-features = false } |
There was a problem hiding this comment.
Thanks for pointing this out. I'd actually like even the grpc examples to be runnable without having proto in your path. So what I've done is added an env var (GRPC_RUST_REGENERATE_PROTO) to build.rs to cause it to regenerate the protobuf output. And if it's not set, then nothing is regenerated and nothing fails if the binaries aren't there.
| fn find_in_path(binary_name: &str) -> Option<PathBuf> { | ||
| if let Some(paths) = std::env::var_os("PATH") { | ||
| for path in std::env::split_paths(&paths) { | ||
| let candidate = path.join(binary_name); | ||
| if candidate.is_file() { | ||
| return Some(candidate); | ||
| } | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
Changed to use which and to execute the commands.
| are compiled, and the functions `protoc` and `protoc_gen_rust_grpc` can be used | ||
| to find their locations. | ||
|
|
||
| ## Building binaries manually |
There was a problem hiding this comment.
Oh I like that idea! Except that we need two binaries for now. I talked to Em and he's open to the idea of publishing something similar for protoc itself.
| break | ||
| mkdir -p protoc-cache | ||
| # Find where cmake built the binaries. | ||
| find target/ -name "protoc" -o -name "protoc.exe" -o -name "protoc-gen-rust-grpc" -o -name "protoc-gen-rust-grpc.exe" | while read -r file; do |
There was a problem hiding this comment.
Definitely not while building the plugin. I removed it from there. But the other ones might, because AIUI tonic requires protoc in your path. But maybe our CI doesn't except for the codegen job? I'll play around with this some more.
LucioFranco
left a comment
There was a problem hiding this comment.
Overall LGTM have a few changes I think we should get in first
| println!("cargo:rerun-if-env-changed=GRPC_RUST_REGENERATE_PROTO"); | ||
| let grpc_helloworld = env::var_os("CARGO_FEATURE_GRPC_HELLOWORLD").is_some(); | ||
| let grpc_routeguide = env::var_os("CARGO_FEATURE_GRPC_ROUTEGUIDE").is_some(); | ||
|
|
||
| if (grpc_helloworld || grpc_routeguide) && env::var_os("GRPC_RUST_REGENERATE_PROTO").is_some() { | ||
| let manifest_dir = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap()); | ||
|
|
||
| grpc_protobuf_build::CodeGen::new() | ||
| .output_dir(manifest_dir.join("src/grpc-helloworld/generated")) | ||
| .input("helloworld.proto") | ||
| .include(manifest_dir.join("proto/helloworld")) | ||
| .client_only() | ||
| .compile() | ||
| .unwrap(); | ||
|
|
||
| grpc_protobuf_build::CodeGen::new() | ||
| .output_dir(manifest_dir.join("src/grpc-routeguide/generated")) | ||
| .input("route_guide.proto") | ||
| .include(manifest_dir.join("proto/routeguide")) | ||
| .client_only() | ||
| .compile() | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
I think we should get some of these env var settings in a readme for the examples folder?
There was a problem hiding this comment.
The CARGO_FEATURE env vars are cargo magic. I added something for the GRPC_RUST_REGENERATE_PROTO.
| #[cfg(feature = "build-plugin")] | ||
| { | ||
| let compiled_protoc = PathBuf::from(protoc_gen_rust_grpc::protoc()); | ||
| let compiled_plugin = PathBuf::from(protoc_gen_rust_grpc::protoc_gen_rust_grpc()); | ||
| if compiled_protoc.exists() && compiled_plugin.exists() { | ||
| // The files may not exist if a build setting instructed protoc-gen-rust-grpc to | ||
| // skip the C++ build (DOCS_RS / our CI setting). | ||
| return Ok((compiled_protoc, compiled_plugin)); | ||
| } | ||
| } |
There was a problem hiding this comment.
we need to add a NO_VENDOR style env var (prob something like GRPC_RUST_NO_BUILD_PLUGIN) that even if build-plugin is enabled you can still disable it since feature flags are additive across the whole workspace if another crate enables it it may get enabled in your code. Can see the no vendor version here for openssl
There was a problem hiding this comment.
There's already GRPC_RUST_SKIP_CPP_BUILD in protoc-gen-rust-grpc -- should we just give that a better name and document it?
Otherwise, because the cmake happens in build.rs, there is nothing we can do in this crate to stop it from happening.
| * | ||
| */ | ||
|
|
||
| //! Protobuf integration for the [`grpc` crate](grpc). |
There was a problem hiding this comment.
| //! Protobuf integration for the [`grpc` crate](grpc). | |
| //! Protobuf integration for the [`grpc`](grpc) crate. |
|
|
||
|
|
||
| (cd interop && cargo build --bins) | ||
| # Skip building in github actions as the workflow already builds it (to |
There was a problem hiding this comment.
is this still needed? says to make sure cache works?
protoc-gen-rust-grpcis now a real Rust crate that is able to buildprotoc(for now) and our plugin binary.grpc-protobuf-buildusesprotoc-gen-rust-grpcto produce the binaries, instead of requiring the user to put them in their path.cargo buildforinteropinside the CI's environment instead of in therunenvironment which uses MSYS and for some reason doesn't like the way we're doing the caching. (Maybe this is a good enough reason to go back to how the cache was working before?)grpc/README.md, and add more info toCargo.tomls, removepublish=falseto allow them to be published, etc.