-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tonic-health: check in generated code instead of requiring tonic-build #1052
Comments
Hey! I think checking in the generated code for both health and reflection would be great. What you suggested sounds great. For the If you're also interested we could use both in |
In case it's helpful, here's roughly the steps I took to validate the generated source via github actions:
[features]
default = []
# generate gRPC code from `.proto`(s)
gen-proto = ["dep:tonic-build"]
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 pub mod grpc_spec {
include!("generated/grpc_spec.rs");
}
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/ |
@nashley we could add this stuff to the readme if you're interested. |
Great, thanks for the replies! I'll take a stab at this soon, haven't had much free time since creating the issue. |
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 oftonic
who decide to check in their generated code but still want to usetonic-health
.Crates
tonic-health
Motivation
There are a couple of use cases here:
protoc
just to use the library (with the aim of completely abstracting away the protobuf/gRPC internals). That all works fine except for thetonic-health
dependency which still requires protoc at build time.protoc
at build time. This precludes them usingtonic-health
, for now at least.Proposal
Remove the
tonic-build
build dependency and just check in the generated code. Specifically:Make the following changes to
build.rs
andlib.rs
Compile the crate to run the build script
Check in the
src/generated
directorySet
build = false
in Cargo.tomlRemove the build dependency on
tonic-build
(or comment it out, so it's easy to re-generate if using new and improved versions oftonic-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
featuresOne problem I can see with this is that the
transport
feature oftonic-health
is currently (supposedly) passed down totonic-build
, and that's not easily handled in an automatic way. Two points to make here:transport
is a default feature oftonic-build
andtonic-health
doesn't setdefault_features = false
in the build dependency 🙄#[cfg(feature = "transport")]
to the generated code: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 oftonic-health
to take advantage of those improvements.The text was updated successfully, but these errors were encountered: