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

Add file_descriptor option to prost-build #311

Closed
wants to merge 2 commits into from

Conversation

jen20
Copy link
Contributor

@jen20 jen20 commented May 4, 2020

This pull request adds support for including an encoded version of the appropriate FileDescriptorProto in each generated Rust module in the form of a &'static [u8] named FILE_DESCRIPTOR. This is disabled by default, and is enabled by setting file_descriptor(true) in a Config builder.

The rationale for this pull request is to support the gRPC Server Reflection Protocol, which operates in terms of FileDescriptorProto, in Tonic.

@jen20 jen20 force-pushed the jen20/file-descriptor-proto branch from d55187a to c27da8b Compare May 4, 2020 01:19
prost-build/src/lib.rs Outdated Show resolved Hide resolved
jen20 added 2 commits May 4, 2020 15:10
This commit adds support for including an encoded version of the
appropriate `FileDescriptorProto` in each generated Rust module in the
form of a `&'static [u8]` named `FILE_DESCRIPTOR`. This is disabled by
default, and is enabled by setting `file_descriptor(true)` in a
`Config` builder.
This should fix CI errors reported upstream.
@jen20 jen20 force-pushed the jen20/file-descriptor-proto branch from c27da8b to 3fe3e40 Compare May 4, 2020 20:12
@danburkert
Copy link
Collaborator

A few thoughts in no particular order:

The correspondence between .proto files and generated Rust modules is n:1, not 1:1. As such, I think if you were to test this with two .proto files in the same package, it would fail to compile due to duplicate FILE_DESCRIPTOR symbols.

I think this would be more elegantly expressed with include_bytes!() as opposed to inlining the descriptor proto into the generated rust source.

Looking at the provided link on gRPC server reflection, the corresponding C++ code uses DescriptorDatabase. If you look at how the Descriptor machinery works for e.g. C++ generated code, it involves a lot of static init ctor magic. I'm not too keen on adding that kind of thing to prost, but it's probably an inevitability if prost is going to get good generic/reflective capabilities. I'd strongly strongly prefer to use something not homebrew for this, such as https://github.com/dtolnay/inventory.

@danburkert
Copy link
Collaborator

Perhaps a better approach would be to include the FileDescriptorSet instead of the FileDescriptor. I'm not sure if there's an obvious place to 'mount' that, though. Perhaps prost could just serialize it to a file in a well-known-location, and tonic could include_bytes! it?

@jen20
Copy link
Contributor Author

jen20 commented May 11, 2020

Ah, I didn't quite appreciate that there was not a 1-1 correspondence between generated Rust modules and protocol buffers files. My original approach was to write out the FileDescriptorSet, but I modified that to take a similar approach to the Go version before opening this.

I think in the short term it's fine to just write out the FileDescriptorSet to the output directory and have Tonic and whichever other consumers find that useful use include_bytes!. I'll close this pull request and open a new one to implement that approach instead. Getting general purpose reflection capabilities in prost itself probably warrants more thought, and I don't think this short-term tactical approach for getting reflection in Tonic necessarily designs anything out.

@jen20 jen20 closed this May 11, 2020
@jen20 jen20 deleted the jen20/file-descriptor-proto branch May 11, 2020 16:03
@danburkert
Copy link
Collaborator

That sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants