tonic-health: check in generated code instead of requiring tonic-build

[sd2k]

Feature Request

As the author of a library/SDK that provides convenience wrappers over gRPC definitions, it'd be great if tonic-health didn't require protoc to compile, by checking in the generated code rather than generating it at build time. This would also benefit users of tonic who decide to check in their generated code but still want to use tonic-health.

Crates

• tonic-health

Motivation

There are a couple of use cases here:

1. I maintain the Grafana Plugin SDK which provides, amongst other things, idiomatic Rust wrappers around some gRPC services that must be implemented by Grafana plugins. With tonic 0.8.0 and prost 0.11.0 I've decided to check in the generated proto/gRPC code, to avoid Grafana plugin authors having to install protoc just to use the library (with the aim of completely abstracting away the protobuf/gRPC internals). That all works fine except for the tonic-health dependency which still requires protoc at build time.
2. Similarly, other authors of gRPC services may want to commit their generated code rather than add a dependency on protoc at build time. This precludes them using tonic-health, for now at least.

Proposal

Remove the tonic-build build dependency and just check in the generated code. Specifically:

1. Make the following changes to build.rs and lib.rs

   diff --git a/tonic-health/build.rs b/tonic-health/build.rs
   index 5b04330..e7d5b81 100644
   --- a/tonic-health/build.rs
   +++ b/tonic-health/build.rs
   @@ -1,11 +1,19 @@
   -use std::env;
   +// This build file was used to generate the code as a one-off,
   +// and is now disabled using `build = false` in `Cargo.toml`.
   +// This simplifies the build process for this crate by not requiring
   +// users to have protoc available.
   +//
   +// To regenerate those modules, set `build = true` in Cargo.toml,
   +// recompile this crate, and commit the updated generated code.
   +
    use std::path::PathBuf;
   
    fn main() -> Result<(), Box<dyn std::error::Error>> {
        let grpc_health_v1_descriptor_set_path: PathBuf =
   -        PathBuf::from(env::var("OUT_DIR").unwrap()).join("grpc_health_v1.bin");
   +        PathBuf::from("src/generated").join("grpc_health_v1.bin");
        tonic_build::configure()
            .file_descriptor_set_path(grpc_health_v1_descriptor_set_path)
   +        .out_dir("src/generated")
            .build_server(true)
            .build_client(true)
            .compile(&["proto/health.proto"], &["proto/"])?;
   diff --git a/tonic-health/src/lib.rs b/tonic-health/src/lib.rs
   index 59aab16..f29dc34 100644
   --- a/tonic-health/src/lib.rs
   +++ b/tonic-health/src/lib.rs
   @@ -23,14 +23,14 @@
   
    use std::fmt::{Display, Formatter};
   
   -/// Generated protobuf types from the `grpc.healthy.v1` package.
   +/// Generated protobuf types from the `grpc.health.v1` package.
    pub mod proto {
        #![allow(unreachable_pub)]
        #![allow(missing_docs)]
   -    tonic::include_proto!("grpc.health.v1");
   +    include!("generated/grpc.health.v1.rs");
   
        pub const GRPC_HEALTH_V1_FILE_DESCRIPTOR_SET: &[u8] =
   -        tonic::include_file_descriptor_set!("grpc_health_v1");
   +        include_bytes!("generated/grpc_health_v1.bin");
    }
   
    pub mod server;
   

2. Compile the crate to run the build script

3. Check in the src/generated directory

4. Set build = false in Cargo.toml

5. Remove the build dependency on tonic-build (or comment it out, so it's easy to re-generate if using new and improved versions of tonic-build)

Alternatives

As an alternative I could probably just copy the tonic-health proto definitions and code into my library, but that feels wrong 😄

Problems

The transport features

One problem I can see with this is that the transport feature of tonic-health is currently (supposedly) passed down to tonic-build, and that's not easily handled in an automatic way. Two points to make here:

1. This isn't actually done correctly right now because transport is a default feature of tonic-build and tonic-health doesn't set default_features = false in the build dependency 🙄
2. This could be fixed as a one-off by manually adding a #[cfg(feature = "transport")] to the generated code:

   diff --git a/tonic-health/src/generated/grpc.health.v1.rs b/tonic-health/src/generated/grpc.health.v1.rs
   index 634201e..30751a0 100644
   --- a/tonic-health/src/generated/grpc.health.v1.rs
   +++ b/tonic-health/src/generated/grpc.health.v1.rs
   @@ -43,6 +43,7 @@ pub mod health_client {
        pub struct HealthClient<T> {
            inner: tonic::client::Grpc<T>,
        }
   +    #[cfg(feature = "transport")]
        impl HealthClient<tonic::transport::Channel> {
            /// Attempt to create a new client by connecting to a given endpoint.
            pub async fn connect<D>(dst: D) -> Result<Self, tonic::transport::Error>

but it would need doing manually every time the generated code was updated, which isn't ideal 🤔

Regenerating code

I guess the biggest advantage of using tonic-build is that it makes use of the latest features of that library; having the generated code committed would mean regenerating and cutting a new release of tonic-health to take advantage of those improvements.

[LucioFranco]

Hey! I think checking in the generated code for both health and reflection would be great. What you suggested sounds great. For the transport feature I would actually not include it since its pretty easy to just use new and the transport feature generated code is just for convenience on the client side and I think most people use this code for the server side. The one thing we need to figure out is how to test this in CI. We already do this for prost-types in the bootstrap.rs test. We might want to consider providing this sort of util for others that want to do the same.

If you're also interested we could use both in prost and tonic guidance on when to commit vs use protoc. This would be very valuable docs to have.

[nashley]

In case it's helpful, here's roughly the steps I took to validate the generated source via github actions:

Cargo.toml:

[features]
default   = []

# generate gRPC code from `.proto`(s)
gen-proto = ["dep:tonic-build"]

buid.rs:

fn main() -> Result<(), Box<dyn std::error::Error>> {
	#[cfg(feature = "gen-proto")]
	tonic_build::configure()
		.out_dir("src/generated/")
		.compile(&["grpc_spec.proto"], &["proto/"])?;

	Ok(())
}

Usage in src/main.rs or wherever:

pub mod grpc_spec {
	include!("generated/grpc_spec.rs");
}

.github/workflows/pr.yml:

jobs:
  gen_proto:
    runs-on: ubuntu-latest
    steps:
      - name: Ensure generated code matches
        run: >
          echo 'If this fails, run `cargo build --features gen-proto` and commit the changes';
          git diff --exit-code --name-only -- src/generated/

[LucioFranco]

@nashley we could add this stuff to the readme if you're interested.

[sd2k]

Great, thanks for the replies! I'll take a stab at this soon, haven't had much free time since creating the issue.