Skip to content
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

build(proto): replace buf-generate by tonic_build #581

Merged
merged 6 commits into from
Nov 20, 2023

Conversation

SuperFluffy
Copy link
Member

@SuperFluffy SuperFluffy commented Nov 15, 2023

Summary

Replaces buf generate by tonic_build to generate rust code from protobuf.

Background

buf generate relies on the third party plugins protoc-gen-prost and protoc-gen-tonic, which are not yet updated to prost-build 0.12 and tonic-build 0.10 (and hence prost 0.12, tonic 0.10).

This patch moves code generation away from the buf generate executable to the Rust tonic-build crate, because we need to update prost/tonic and because these projects do not guarantee that code generated with older versions work with newer versions.

Changes

  • Replace invoking the buf generate executable by using tonic_build directly
  • Use buf build to create a file descriptor set that can be fed into tonic_build (the latter skips its own invocation of protoc)
  • Clean the generated code by removing all files that are not prefixed with astria. (tonic also creates client and server code for tendermint abci; I am not sure why, probably due to the descriptor set)

Testing

Not tested directly; this should not affect the actual code generated. Everything should still compile, all tests should pass.

Related Issues

PR requiring a newer version of penumbra and hence prost: #579

PR againast protoc-gen-prost: neoeinstein/protoc-gen-prost#78

@github-actions github-actions bot added the proto pertaining to the Astria Protobuf spec label Nov 15, 2023
@SuperFluffy SuperFluffy changed the title change buf to tonic build refactor: replace buf-build by tonic_build Nov 15, 2023
@SuperFluffy SuperFluffy changed the title refactor: replace buf-build by tonic_build refactor: replace buf-generate by tonic_build Nov 15, 2023
@SuperFluffy SuperFluffy changed the title refactor: replace buf-generate by tonic_build build(proto): replace buf-generate by tonic_build Nov 15, 2023
@SuperFluffy SuperFluffy requested a review from a team November 15, 2023 16:02
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The *.tonic.rs files have been absorbed as modules directly into their parent files (these were include!ed previously I believe)

Copy link
Collaborator

@noot noot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! so are we still using buf for handling dependencies, but tonic for the actual build?

@SuperFluffy
Copy link
Member Author

SuperFluffy commented Nov 15, 2023

looks good! so are we still using buf for handling dependencies, but tonic for the actual build?

Yeah, so buf fetches the the tendermint protos as defined in astria-proto/proto/buf.yaml. and then compiles our protos + the tendermint protos it just pulled, emitting a file descriptor set. We then feed this set into tonic-build.

My understanding is that tonic-build would usually invoke protoc, which would also result in that file descriptor set. But we .skip_protoc() because we want buf to deal with proto-dependecy resolution.

This way we avoid having to vendor tendermint.

@SuperFluffy SuperFluffy merged commit 2b75024 into main Nov 20, 2023
@SuperFluffy SuperFluffy deleted the superfluffy/buf-to-prost branch November 20, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proto pertaining to the Astria Protobuf spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants