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?
| // TODO: Once the codegen is updated to allow the `OUT_DIR` env var to | ||
| // be unset, change this to use `.output_dir` instead. |
There was a problem hiding this comment.
Since the OUT_DIR override is still present, we should leave a comment explaining why it's required.
| prettyplease = "0.2.35" | ||
| protobuf-codegen = { version = "4.34.0-release" } | ||
| syn = "2.0.104" | ||
| protoc-gen-rust-grpc = { version = "0.9.0-alpha.1", path = "../protoc-gen-rust-grpc" } |
There was a problem hiding this comment.
Should the version of the protoc-gen-rust-grpc dependency also be 0.9.0-alpha.2?
| [dependencies] | ||
| bytes = "1.11.1" | ||
| grpc = { path = "../grpc" } | ||
| grpc = { version = "0.9.0-alpha.1", path = "../grpc" } |
There was a problem hiding this comment.
Should the version of the grpc dependency be 0.9.0-alpha.2? Or is it intentionally using the second latest version?
| [build-dependencies] | ||
| tonic-prost-build = {path = "../tonic-prost-build"} | ||
| grpc-protobuf-build = {path = "../grpc-protobuf-build"} | ||
| protoc-gen-rust-grpc = {path = "../protoc-gen-rust-grpc"} No newline at end of file |
There was a problem hiding this comment.
nit: No newline at end of file.
| @@ -0,0 +1,52 @@ | |||
| /* | |||
| * | |||
| * Copyright 2025 gRPC authors. | |||
There was a problem hiding this comment.
nit: The copyright year needs to be 2026.
|
|
||
| Note: as part of compiling, the source files for `protoc` are downloaded from | ||
| the [protobuf github repository](https://github.com/protocolbuffers/protobuf). | ||
| The archive's checksum is verified before compiling. |
There was a problem hiding this comment.
There is a fallback which skips hash verification for unknown versions, should we remove that to avoid supply chain attacks?
grpc-rust/protoc-gen-rust-grpc/cmake/FetchProtobuf.cmake
Lines 23 to 26 in af47335
| path: protoc-cache | ||
| key: protoc-binaries-${{ runner.os }}-${{ hashFiles('protoc-gen-rust-grpc/src/cpp_source/**') }} | ||
| - uses: Swatinem/rust-cache@v2 | ||
| - run: cargo build -p interop |
There was a problem hiding this comment.
Is this done to avoid building the binaries twice, once for TLS and once for non-TLS tests? Maybe we should add this to the comment in test.sh. It took me a couple of minutes to understand the use reason for caching the artifacts in CI.
There was a problem hiding this comment.
See PR description:
We now run cargo build for interop inside the CI's environment instead of in the run environment 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?)
I can try to expand on this in the comment in test.sh if we don't decide to change things further.
2 participants
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.