feat(grpc): Add `protobuf` codegen #2320

arjan-bal · 2025-07-02T18:22:41Z

This PR includes the following:

A tonic codec that uses protobuf-rust.
A protoc plugin that generates tonic compatible client code.
A grpc-build crate that helps generate code during cargo builds.
Interop tests that use the new client codgen running against a Go server. This is used as an integration test for the new codegen.

To keep the CI fast, the protoc plugin binary for each OS is cached. The plugin binaries are re-built only if the plugin's code or build files are updated.

Notes

The codegen is generating code for tonic. It will be changed when the gRPC API design is finalized and the channel implementation is complete.

LucioFranco · 2025-07-02T18:29:45Z

compiler/README.md

@@ -0,0 +1,29 @@
+## Usage example


Should we rename this containg folder? Compiler seems quite ambiguous?

Yes, it is. I was using the same name as Java and the core repo. I've changed it to the name of protoc plugin, protoc-gen-rust-grpc now.

interop/test.sh

arjan-bal · 2025-07-07T04:58:25Z

@dfawley please have a look

arjan-bal · 2025-07-07T10:38:16Z

grpc/src/codec/protobuf.rs

+        let serialized = item.serialize().map_err(from_decode_error)?;
+        buf.put_slice(&serialized.as_slice());


@LucioFranco I can think of the following ways to avoid a copy here:

Request the protobuf crate to expose an API that allows deserialization into a destination buffer.

Change the codec API to return a Vec<u8> instead of accepting the destination as a parameter. Tonic can then create an EncodedBuf from the returned Vec<u8>.

Make the constructor of EncodedBuf public. This would allow a codec to create its own EncodedBuf and std::mem::swap it with the one passed to the encode method, avoiding a copy.

I think option 3 is a reasonable workaround that doesn't require API changes. Wanted to get your thoughts on this.

protoc-gen-rust-grpc/src/grpc_rust_plugin.cc

acozzette · 2025-07-08T19:46:18Z

protoc-gen-rust-grpc/README.md

+PLUGIN_PATH="$(pwd)/bazel-bin/src/protoc-gen-rust-grpc"
+
+# Run protoc with the Rust and Rust gRPC plugins
+protoc \


I think we should probably advise users to use grpc_build::CodeGen and/or protobuf_codegen::CodeGen instead of directly invoking protoc. If you directly invoke protoc and check in the generated code, then it's at risk of becoming stale if/when you upgrade your protobuf dependency.

Added a note discouraging the direct use of the plugin. I've also added a README in the tonic_protobuf_build (formerly grpc_build) crate.

I'm not opposed to recommending it that way, but is this a real concern? I thought the protobuf library would support older generated code, but not vice-versa? I'm fairly sure that's true for Go at least.

We are going to make an exception for some libraries, but we do generally plan to require the generated code to match the runtime version. In most other languages we are more relaxed about this but we have found that it's more difficult to maintain protobuf when we have to accommodate stale gencode.

protoc-gen-rust-grpc/README.md

tonic-protobuf-build/src/lib.rs

dfawley

There's quite a lot here, but it mostly looks good to me, pending some comments that are mostly nits. I loosely skimmed the interop code since you said that was copied from the other tonic tests. And my C++ is very rusty so it might be good to get a review from someone on the gRPC-C++ team. Maybe @drfloob ?

interop/src/client_new_codegen.rs

dfawley · 2025-07-10T03:45:20Z

protoc-gen-rust-grpc/README.md

+PLUGIN_PATH="$(pwd)/bazel-bin/src/protoc-gen-rust-grpc"
+
+# Run protoc with the Rust and Rust gRPC plugins
+protoc \


I'm not opposed to recommending it that way, but is this a real concern? I thought the protobuf library would support older generated code, but not vice-versa? I'm fairly sure that's true for Go at least.

protoc-gen-rust-grpc/README.md

protoc-gen-rust-grpc/src/BUILD

protoc-gen-rust-grpc/src/grpc_rust_plugin.cc

protoc-gen-rust-grpc/src/grpc_rust_generator.h

tonic-protobuf-build/src/lib.rs

Co-authored-by: Doug Fawley <[email protected]>

drfloob

I didn't find any logic errors in the C++ code. There are quite a few Google C++ style guide issues, though I'm not sure they're enforced for this codebase. Many of them are good practices regardless, so I've left suggestions.

protoc-gen-rust-grpc/src/grpc_rust_generator.h

drfloob · 2025-07-11T18:12:40Z

protoc-gen-rust-grpc/src/grpc_rust_generator.h

+#ifndef NET_GRPC_COMPILER_RUST_GENERATOR_H_
+#define NET_GRPC_COMPILER_RUST_GENERATOR_H_
+
+#include "absl/log/absl_log.h"


The project could benefit from a C++ formatter & linter. System included should precede other libraries' headers. See https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

Acknowledged. I'll raise a follow-up PR with scripts to run clang-format and buildifier, both locally and on CI. I don't want to make this PR bigger.

protoc-gen-rust-grpc/src/grpc_rust_generator.h

protoc-gen-rust-grpc/src/grpc_rust_generator.cc

protoc-gen-rust-grpc/src/grpc_rust_plugin.cc

arjan-bal

@drfloob Thanks for the thorough review — I've addressed your comments.

interop/src/client_new_codegen.rs

protoc-gen-rust-grpc/README.md

protoc-gen-rust-grpc/src/BUILD

protoc-gen-rust-grpc/src/grpc_rust_generator.cc

protoc-gen-rust-grpc/src/grpc_rust_plugin.cc

arjan-bal · 2025-07-14T07:48:43Z

protoc-gen-rust-grpc/src/grpc_rust_generator.h

+#ifndef NET_GRPC_COMPILER_RUST_GENERATOR_H_
+#define NET_GRPC_COMPILER_RUST_GENERATOR_H_
+
+#include "absl/log/absl_log.h"


Acknowledged. I'll raise a follow-up PR with scripts to run clang-format and buildifier, both locally and on CI. I don't want to make this PR bigger.

LucioFranco · 2025-07-15T19:49:14Z

@arjan-bal we need to get CI passing before we can merge. I think we can just bump the msrv to 1.79 but lets do that in a separate PR.

As for the semver CI job I think we can just add a publish = false for now to that crate and it should resolve the issue.

LucioFranco

Overall LGTM, lets get these last few things addressed and CI passing then lets merge.

LucioFranco · 2025-07-16T12:03:58Z

.github/workflows/CI.yml

+          # Share repository cache between workflows.
+          repository-cache: true
+          module-root: ./protoc-gen-rust-grpc
+      - name: Build protoc plugin


Does this step take a long time? Generally, speaking I would rather that we build the plugin in each test job to ensure we are building the latest code and there is no caching issues. Could you explain the reasoning for breaking it up like this?

Added a comment in the yaml. Building the protoc plugin from scratch takes 6–14 minutes, depending on the OS. This delays the execution of workflows that use the plugin in build.rs files.

Example workflow execution: https://github.com/hyperium/tonic/actions/runs/16217230891/job/45789317103

Okay yeah if this works its fine for now, we probably want to improve this in the future..

grpc/Cargo.toml

grpc/src/macros.rs

interop/Cargo.toml

interop/src/bin/client_new_codegen.rs

LucioFranco · 2025-07-16T12:40:07Z

tonic-protobuf-build/src/lib.rs

+                .unwrap();
+        }
+        let crate_mapping_path = if self.generate_message_code {
+            self.output_dir.join("crate_mapping.txt")


is it possible to have multiple codegens override the same file? Do we need to add a warning incase say you do that by accident and its surprising?

is it possible to have multiple codegens override the same file?

Yes it's possible. A bigger problem when having multiple protoc invocations using the same output directory is the generated.rs file getting overwritten. The generated.rs file exports the generated message symbols for all the proto files that were part of the input protos list.

Do we need to add a warning incase say you do that by accident and its surprising?

@acozzette wanted to get your views on this. Have you considered this issue?

LucioFranco · 2025-07-16T12:40:33Z

tonic-protobuf-build/src/lib.rs

+        };
+
+        // Generate the service code.
+        let mut cmd = std::process::Command::new("protoc");


We probably want to allow users to set the path to protoc via an env var as well. We do this in the prost code so we should follow how it does that.

Here are the relevant prost docs: https://docs.rs/prost-build/latest/prost_build/index.html#sourcing-protoc

The message codegen also doesn't support setting the path to protoc using an env var. I think protobuf-codegen should support this before gRPC. Do you want me to file a feature request for protobuf?

Yeah, that would be good I think it shouldn't be too contentious.

Filed protocolbuffers/protobuf#22718

tonic-protobuf/Cargo.toml

LucioFranco · 2025-07-16T12:43:17Z

tonic-protobuf/src/lib.rs

+    type Error = Status;
+
+    fn encode(&mut self, item: Self::Item, buf: &mut EncodeBuf<'_>) -> Result<(), Self::Error> {
+        let serialized = item.serialize().map_err(from_decode_error)?;


We should add a todo here mentioning that we want to figure out how to remove this extra copy that happens.

Added a comment. Do you want me to file an issue for the protobuf-rust?

That might be good, we discussed this with @acozzette and co last week but probably good to track it somewhere as well. I would be okay if this was also just a tonic issue.

Opened a tonic issue with context about why protobuf doesn't provide the required API: #2345

LucioFranco · 2025-07-16T12:44:19Z

tonic-protobuf/src/lib.rs

+        let item = U::parse(slice).map_err(from_decode_error)?;
+        buf.advance(slice.len());


I think its possible that the incoming buf slice is larger than the actual message, we should see if we can pull the len from the decoded amount of bytes.

From my understanding, each field in an encoded proto message either has a length prefix or a fixed length. The parser should continue reading fields until it reaches the end of the buffer. Fields not present in the proto descriptor should be ignored to ensure forward compatibility—for example, when a new field is added to the proto, but the receiver is using an older version. A parsing error should be returned if the bytes cannot be parsed. Since there is no length prefix for the entire message, the parser must consume all the bytes it is given.

I think its possible that the incoming buf slice is larger than the actual message

Based on the above, I believe it may be incorrect to pass a slice to a parser but not consume the entire buffer. Can you clarify when this situation arises?

Hmm that is a good question, I will look into it, maybe we also want to actually provide some type safe wrapper that enforces this invariant. I wouldn't worry about this for now I think your assumption is probably right on what we actually do.

Co-authored-by: Lucio Franco <[email protected]>

arjan-bal added 2 commits July 2, 2025 20:57

protobuf codec, codegen, interop test

1f78401

CI fixes

6a379ea

arjan-bal changed the title ~~gRPC client codegen~~ [WIP] gRPC client codegen Jul 2, 2025

LucioFranco reviewed Jul 2, 2025

View reviewed changes

arjan-bal changed the title ~~[WIP] gRPC client codegen~~ gRPC client codegen Jul 2, 2025

arjan-bal marked this pull request as draft July 2, 2025 18:33

LucioFranco reviewed Jul 2, 2025

View reviewed changes

interop/test.sh Outdated Show resolved Hide resolved

LucioFranco changed the title ~~gRPC client codegen~~ feat(grpc): Add protobuf codegen Jul 2, 2025

arjan-bal force-pushed the grpc-codegen branch 4 times, most recently from d39e0f5 to de384b4 Compare July 4, 2025 08:54

Enforce c++17

f3f9446

arjan-bal force-pushed the grpc-codegen branch 17 times, most recently from 9e45ab4 to dfc5213 Compare July 4, 2025 22:43

arjan-bal commented Jul 7, 2025

View reviewed changes

LucioFranco mentioned this pull request Jul 7, 2025

Release tonic 0.14 (with prost 0.14) #2317

Open

acozzette reviewed Jul 7, 2025

View reviewed changes

protoc-gen-rust-grpc/src/grpc_rust_plugin.cc Outdated Show resolved Hide resolved

Avoid using protobuf rust's context

89d7331

arjan-bal requested a review from acozzette July 8, 2025 18:09

acozzette reviewed Jul 8, 2025

View reviewed changes

arjan-bal added 2 commits July 9, 2025 14:26

create a seperate module for the protobuf codec

9305da7

Add readme for codegen

ca8d638

arjan-bal requested a review from acozzette July 9, 2025 11:03

acozzette reviewed Jul 9, 2025

View reviewed changes

protoc-gen-rust-grpc/README.md Show resolved Hide resolved

tonic-protobuf-build/src/lib.rs Outdated Show resolved Hide resolved

dfawley approved these changes Jul 10, 2025

View reviewed changes

arjan-bal and others added 2 commits July 11, 2025 15:26

Apply suggestions from code review

08c6be1

Co-authored-by: Doug Fawley <[email protected]>

copyright in build files, clippy fixes and typos in README

4431e83

drfloob reviewed Jul 11, 2025

View reviewed changes

mostly C++ readability fixes

8a8e4e6

arjan-bal commented Jul 14, 2025

View reviewed changes

arjan-bal requested a review from drfloob July 14, 2025 07:52

Format generated code during build

cd77988

acozzette approved these changes Jul 14, 2025

View reviewed changes

LucioFranco requested changes Jul 16, 2025

View reviewed changes

Address review

9f06a59

arjan-bal force-pushed the grpc-codegen branch from 869d9ee to 9f06a59 Compare July 17, 2025 13:36

arjan-bal and others added 2 commits July 17, 2025 19:12

Apply suggestions from code review

5cd495c

Co-authored-by: Lucio Franco <[email protected]>

Combine client binaries, use flag for codec

c3e03f9

arjan-bal requested a review from LucioFranco July 17, 2025 18:00

arjan-bal added 2 commits July 21, 2025 15:11

Merge remote-tracking branch 'source/master' into grpc-codegen

e8aaa51

Fix cpp function name casing

726d227

arjan-bal mentioned this pull request Jul 22, 2025

Allow specifying protoc location using an env var for rust codegen protocolbuffers/protobuf#22718

Open

		let serialized = item.serialize().map_err(from_decode_error)?;
		buf.put_slice(&serialized.as_slice());

		let item = U::parse(slice).map_err(from_decode_error)?;
		buf.advance(slice.len());

feat(grpc): Add protobuf codegen #2320

Are you sure you want to change the base?

feat(grpc): Add protobuf codegen #2320

Conversation

arjan-bal commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arjan-bal commented Jul 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drfloob left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arjan-bal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucioFranco commented Jul 15, 2025

Uh oh!

LucioFranco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feat(grpc): Add `protobuf` codegen #2320

feat(grpc): Add `protobuf` codegen #2320

arjan-bal commented Jul 2, 2025 •

edited

Loading

LucioFranco Jul 21, 2025 •

edited

Loading