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.

Sign up for GitHub

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

Jump to bottom

chore: update tonic to 0.10 #78

Closed
wants to merge 7 commits into from
Closed

chore: update tonic to 0.10 #78

wants to merge 7 commits into from

Conversation

fenollp
Copy link

@fenollp fenollp commented Oct 20, 2023

Hello!
I see you're using some fashion of dependencies vetting and this PR brings in deps diffs.
How can I help with the review process? I understand this isn't something that can be done in your stead.

@SuperFluffy SuperFluffy mentioned this pull request Nov 15, 2023
Merged
SuperFluffy added a commit to astriaorg/astria that referenced this pull request Nov 20, 2023
@SuperFluffy
build(proto): replace buf-generate by tonic_build (#581)
2b75024 
## 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
@neoeinstein
Copy link
Owner

This has been delayed in coming, but I'm doing a broad swing through my projects these past few days. I will indeed bring this in. Sorry for the delay in the meantime.

@fenollp
Copy link
Author

fenollp commented Feb 8, 2024

Hullo @neoeinstein ! Need some help?

@neoeinstein
Copy link
Owner

I think we're nearly there. The sticking point during my testing has been figuring out whether there were code generation changes that result in an incompatibility in versions, and so what the semantic version increment should be. It now appears that pbjson code generation relied on a trait that isn't implemented (or is implemented differently) in newer versions of prost. This means I'll likely need to release the version updates as a major version bump, something that should be completed and released shortly (and then I can quickly take requests to incorporate new builder functionality from these newer versions)

@neoeinstein
Copy link
Owner

Now released as protoc-gen-tonic 0.4.0.

@fenollp fenollp deleted the tonic-0.10 branch February 15, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fenollp @neoeinstein