Ignore empty codegen modules in outputs #726
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
|
@trevoranderson thank you for doing this! Would it be possible to add a test of the original behavior that was fixed with this so we can make sure we don't regress in the future? |
…x and seeing it fail, by adding the non-working reverted fix and seeing it still fail, and finally by adding the fix back in and seeing it pass
|
Made an attempt at that too. I used an proto adapted from the hyperium issue report which reproduces the issue without my fix and works with the fix applied. |
LucioFranco
approved these changes
Oct 17, 2022
| @@ -156,6 +150,13 @@ use crate::ident::to_snake; | |||
| use crate::message_graph::MessageGraph; | |||
| use crate::path::PathMap; | |||
|
|
|||
| mod ast; | |||
…s_and_include_file
Closed
tottoto
added a commit
to tottoto/tonic
that referenced
this pull request
Jan 17, 2023
This file was generated by the previous version of prost-build. This behaviour was fixed in tokio-rs/prost#726.
tottoto
added a commit
to tottoto/tonic
that referenced
this pull request
Jan 17, 2023
This file was generated by the previous version of prost-build. This behaviour was fixed in tokio-rs/prost#726.
LucioFranco
pushed a commit
to hyperium/tonic
that referenced
this pull request
Feb 4, 2023
This file was generated by the previous version of prost-build. This behaviour was fixed in tokio-rs/prost#726.
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.
References:
Original report: #228
My report: #724
Partial fix in: #605
Issue with fix: hyperium/tonic#964
Reverted in: #639
After digging in, the issue with the first fix is that while it prevented the file from being generated, it did not prevent the file from being referenced in the "include" file because both locations iterate the module hash. My proposed solution is to instead avoid adding the empty module to the list at all. Then the file does not get output and the include file does not have an entry that would otherwise be referencing an empty file.
Also did a small refactor pass on the surrounding loop to destructure the tuple instead of using tuple.0 syntax wherever it is used.
Tested by reproducing the original issue using
tonic-buildwith the original behavior working then reintroduced the bug, then fixed it with my changes. Manually verified output files and included them in a new rust project to make sure they worked and verified that the extra file is not present in file system or the include file. Also ran cargo test on prost