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 include_file_descriptor_set to prost-build #326

Closed
wants to merge 1 commit into from

Conversation

jen20
Copy link
Contributor

@jen20 jen20 commented May 11, 2020

This pull request adds support for writing an encoded version of the appropriate FileDescriptorSet to a file named file_descriptor_set.bin in the output directory. This can be used with the include_bytes macro by consuming applications or libraries.

The rationale for this pull request is to support the gRPC Server Reflection Protocol, which operates in terms of FileDescriptorProto, in Tonic. FileDescriptorProto instances are contained inside a
FileDescriptorSet.

This supersedes #311 following discussion on that issue.

@jen20 jen20 force-pushed the jen20/file-descriptor-set branch from aecd357 to ed927ba Compare June 19, 2020 04:45
@jen20
Copy link
Contributor Author

jen20 commented Jun 19, 2020

I've force pushed here to rebase this on top of the current master branch.

@jen20
Copy link
Contributor Author

jen20 commented Jun 19, 2020

The new CI failures seem unrelated to this change, however.

@jen20
Copy link
Contributor Author

jen20 commented Jul 7, 2020

It seems like my change in 24d7877 would fix the build errors which presumably will hit any PR to this repo right now, but GHA is still using the version of the workflow from the main repo rather than the fork.

@danburkert danburkert mentioned this pull request Jul 7, 2020
@danburkert danburkert force-pushed the jen20/file-descriptor-set branch 2 times, most recently from 86a1761 to c4aeb27 Compare July 18, 2020 21:55
@cpcloud
Copy link

cpcloud commented Aug 17, 2020

It looks like this is passing. Is this PR ready to be merged? It's on the must-have list for the next release, which I know folks are keen to start using (including myself).

@cab
Copy link
Contributor

cab commented Sep 30, 2020

Thanks for the work here, this has been very useful as a branch -- is there anything I can do to help get this merged/released?

wchargin added a commit to wchargin/prost that referenced this pull request Nov 7, 2020
Builds are failing on unrelated pull requests (e.g., tokio-rs#326) because the
`curl` crate has pushed a new version that depends on Rust 1.40, while
our CI tests against 1.39. We could probably bump to require 1.40 as
well, especially since we want to use the same `#[non_exhaustive]`
attribute, per tokio-rs#43. But a minimal change to unblock development and keep
the current minimum stable Rust version is to just pin `curl < 0.4.34`.

Test Plan:
Running `cargo +1.39.0 test --workspace --all-targets` passes on my
Linux machine.

wchargin-branch: deps-curl-pre-0.4.34
wchargin-source: c2910530bd49190605176c9f69f3aff1863443c2
@wchargin
Copy link
Contributor

wchargin commented Nov 7, 2020

@jen20: I’ve merged #382 to fix the build error. That PR passes by
itself on CI, and yours passes locally for me (on both 1.39.0 and
stable) when I rebase it on top of #382. Perhaps it’s worth merging
master again to verify that this fixes the CI issues?

(Edited now that #382 is merged.)

danburkert pushed a commit that referenced this pull request Nov 8, 2020
Builds are failing on unrelated pull requests (e.g., #326) because the
`curl` crate has pushed a new version that depends on Rust 1.40, while
our CI tests against 1.39. We could probably bump to require 1.40 as
well, especially since we want to use the same `#[non_exhaustive]`
attribute, per #43. But a minimal change to unblock development and keep
the current minimum stable Rust version is to just pin `curl < 0.4.34`.

Test Plan:
Running `cargo +1.39.0 test --workspace --all-targets` passes on my
Linux machine.

wchargin-branch: deps-curl-pre-0.4.34
wchargin-source: c2910530bd49190605176c9f69f3aff1863443c2
@jen20 jen20 force-pushed the jen20/file-descriptor-set branch from 9e06601 to 286c0cc Compare November 28, 2020 17:20
@jen20
Copy link
Contributor Author

jen20 commented Nov 28, 2020

I've rebased this on top of the latest master as suggested. Unfortunately the build seems to have hit the GitHub Deprecation of set-path and friends while installing Ninja, and I don't have time to look at that yet. The good news is, the build and all the tests pass locally, so this should just be a case of updating the CI system.

Update: actually the work to fix this seems minimal: #397 should do so, after which this can again be rebased.

@jen20 jen20 force-pushed the jen20/file-descriptor-set branch from 286c0cc to bd8da93 Compare November 29, 2020 16:33
@jen20
Copy link
Contributor Author

jen20 commented Nov 29, 2020

This is rebased on top of #397, so the build should now succeed.

@jen20 jen20 force-pushed the jen20/file-descriptor-set branch from bd8da93 to b36dfcd Compare December 7, 2020 02:26
@jen20
Copy link
Contributor Author

jen20 commented Dec 7, 2020

Rebased again on top of 278e8ab.

This commit adds support for writing an encoded version of the
appropriate FileDescriptorSet to a file named `file_descriptor_set.bin`
in the output directory. This can be used with the `include_bytes` macro
by consuming applications or libraries.

The rationale for this pull request is to support the gRPC Server
Reflection Protocol, which operates in terms of FileDescriptorProto, in
Tonic. FileDescriptorProto instances are contained inside a
FileDescriptorSet.
@danburkert
Copy link
Collaborator

@jen20 I've merged a slightly modified version of this as part of #409. Thanks for your patience, and doubly thanks for the tonic reflection work.

@danburkert danburkert closed this Dec 27, 2020
@jen20 jen20 deleted the jen20/file-descriptor-set branch December 28, 2020 03:02
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.

5 participants