-
Notifications
You must be signed in to change notification settings - Fork 255
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
rename the build.rs such that it's not run by default #1277
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1277 +/- ##
=======================================
Coverage 63.42% 63.42%
=======================================
Files 121 121
Lines 13705 13705
=======================================
Hits 8693 8693
Misses 5012 5012 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not currently convinced that this change has better trade-offs overall than the existing approach. The outer build script might get run in all builds, but it does nothing unless:
- the protobuf files are present (which they are not in published crate versions).
protoc
is available inPATH
.
We could perhaps exclude the build script from published crate versions in addition to excluding the *.proto
files; I don't recall if I tried that when I switched from protobuf
to prost
in #691.
For historical context, we previously always generated code from *.proto
files on-demand using protobuf-codegen-pure
, so we didn't need to deal with keeping the *.proto
files and generated code in sync. prost
codegen isn't implemented in pure Rust and instead calls out to the protoc
binary, so we compromised by checking in the generated files and adding machinery to always keep things in-sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This rename silently breaks CI: the protobuf consistency check will now pass even if the
*.proto
files and generated files are out-of-sync. - This rename silently breaks the workflows of developers maintaining the protobuf files, which combined with the CI break could have led to
*.proto
changes not correctly being reflected in the generated files. - There is now no way to trigger the generator without renaming the build script back to
build.rs
. The rename will inevitably get accidentally checked in along with a future protobuf update, undoing the change.
I just don't think we should make this change. The I think we should close this PR. |
This script is solely for the purpose of producing Rust source code from protobuf spec files, to support lightwalletd gRPC services.
It should only be run when a developer explicitly intends to change [add/remove/modify] those bindings.
It's unlikely this will be the case for the majority of builds, and probably shouldn't be done implicitly.