Commit generated protobuf code, only regenerate on-demand #42
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
MCOfficer
commented
Apr 4, 2024
oll3
requested changes
Apr 4, 2024
There was a problem hiding this comment.
Looks good! Added a single request/question.
Other than that I think the method you've selected seems like the best option given the alternatives you listed. I think it makes it simple enough to regenerate the file when wanted.
Should also remove protoc from the CI, but i want to hear your thoughts first.
The only reason I can think of to keep it would be to validate the checked-in file, but don't know what such a test would be good for really. Anyway, it can be restored if ever needed so let's remove it.
|
Thanks @MCOfficer ! ⭐ |
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.
closes #41
Since v0.11 (4fea65b),
prost-buildrequiresprotocto be installed while compiling. This means any downstream application / library of bitar also needs it to compile.This PR adds the generated code directly the repository, and only regenerates it on-demand.
Deciding when to regenerate the code is a bit of a pain - you can't use
cargo:rerun-if-changed, since that will run directly after a clean check-out, and thus fail withoutprotoc. The best compromise between elegance and practicality i found was to only regenerate when the generated file has been removed. I've added some comments to clarify this; since the file is rarely changed it shouldn't be too much of a hassle.Alternative solutions to consider:
rerun-if-changedand check ifprotocis in PATH. This "silent switch" could lead to build inconsistencies downstream, where the buildscript also runs..rsfile, compile the protobuf files to a.binusingprotocand commit that. The buildscript would work as before (sourcing from the.bininstead of.proto3), without needingprotoc. See for instance feat: vendor pbjson-types descriptor set (#62) influxdata/pbjson#79protocwith this crate, despite the security concerns raised in Remove build.rs tokio-rs/prost#657. Seeprotoc-prebuilt,protoc_bin_vendored,protoc_fetcher