fix(build): Don't replace extern_paths #261
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tonic-build will replace all paths except for google well known types with one rooted at the super module. For paths that have already been replaced with a fully qualified path via the extern_path config option, (e.g. "::uuid::Uuid"), this results in an invalid path (e.g. "super::::uuid::Uuid"), and a build failure. These paths should also be excluded when prefixing relative modules with super.
LucioFranco
requested changes
Feb 8, 2020
extern_path expects fully qualified proto and rust paths
Thanks, I've added to the docstring for extern_path clarifying the need for fully qualified paths |
LucioFranco
requested changes
Feb 12, 2020
There was a problem hiding this comment.
I'd like to see one last thing, I am worried that rustfmt removes the leading :: so I'm curious if we could add a test https:/hyperium/tonic/tree/master/tests here to make sure this doesn't happen.
add a test that extern path does indeed result in service types using the specified type from an external crate. we test this by creating a service type that has a proto from a different crate, and asserting that it does indeed impl a trait from that crate
|
Sorry about the delay; been caught up in quite a bit recently. I adapted my repro crates into tests; i just assert that a type generated from a service uses an external crate. |
LucioFranco
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tonic-build will replace all paths except for google well known types
with one rooted at the super module. For paths that have already been
replaced with a fully qualified path via the extern_path config option,
(e.g. "::uuid::Uuid"), this results in an invalid path
(e.g. "super::::uuid::Uuid"), and a build failure. These paths should
also be excluded when prefixing relative modules with super.
Motivation
See commit message, and #260
When extern_path'd are used in service method definitions, prost-build panics.
See https:/cthulhua/tonic_extern_path/tree/master for a repro of the issue (it works when build against this branch).
Solution
Similar to how google well known types are handled, in the event an absolute path would be made relative, leave it absolute.