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

Use a new prost ServiceGenerator finalizer to fix duplicate client and server definitions #147

Closed
wants to merge 9 commits into from

Conversation

parasyte
Copy link

Note: This PR is in draft mode until the dependent PR in prost is accepted and released: https://github.com/danburkert/prost/pull/241

Motivation

Fixes #38

This change allows tonic-build to properly group all services from multiple .proto files into a single package namespace.

Solution

Defer code generation to the end of all package processing.

Jay Oster added 2 commits November 21, 2019 14:56
- Use a new prost ServiceGenerator finalizer to fix duplicate client and server definitions
@parasyte
Copy link
Author

Ah jeez, failures in cargo-deny and tests that I didn't touch at all... Maybe these will get sorted in master in the meantime and this PR will be fixed by merge or rebase! 😉

@parasyte
Copy link
Author

I'm not happy with the new requirement that user code has to include all packages explicitly. Go doesn't have this problem because it will happily pull all packages into scope from a single import directive.

I don't see an obvious way to auto-include packages within the generated source. The conflict is that imports in protobuf are "file level" and includes in tonic-build are "package level".

@parasyte
Copy link
Author

I see, this is actually a bug! The client and server modules are being output to the wrong package. I will be addressing this tomorrow.

@alce
Copy link
Collaborator

alce commented Nov 22, 2019

@parasyte the cargo-deny check is failing in part because of a duplicated version of crossbeam-queue, which is addressed here #145 and in part because you are referencing a different version of prost-build.

We'll need #145 to be merged first, or at least the commits that fix the crossbeam duplication and then fix the prost-build issue here

@parasyte
Copy link
Author

Ok, we're good here. All tests are passing, just waiting for prost-build 0.5.1 and @alce's crossbeam-queue patch for the cargo deny checks to pass.

@jen20
Copy link
Contributor

jen20 commented Nov 25, 2019

@parasyte I'll look at resolving the issue with cargo-deny shortly, it's affected quite a few of our open PRs right now.

@LucioFranco
Copy link
Member

This looks good! Once prost gets a publish out we can merge this! Thanks!

@parasyte
Copy link
Author

That sounds great. I'll keep the PR as a draft until I can update Cargo.toml with whatever prost version gets released.

@parasyte
Copy link
Author

Looks like #173 fixes this by namespacing the client/server modules separately.

@LucioFranco
Copy link
Member

@parasyte yup, thank you for this contribution sorry we couldn't get it in, but also looks like prost is a bit slow on upgrading anyways. I am gonna close this as it seems its been solved for you!

@parasyte
Copy link
Author

I haven't actually used the new code yet, but I am interested in trying it.

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.

Client/Server /can be/ duplicated into their include files
4 participants