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

rename the build.rs such that it's not run by default #1277

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zancas
Copy link
Contributor

@zancas zancas commented Mar 15, 2024

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.

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.42%. Comparing base (1775f65) to head (3c5aa56).
Report is 16 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@str4d str4d left a 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 in PATH.

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.

Copy link
Contributor

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.

@daira
Copy link
Contributor

daira commented Jun 12, 2024

I just don't think we should make this change. The build.rs is doing precisely what a build.rs should do: the source is the .proto files, not the generated .rs files. It doesn't cause any problem if you don't have protoc on the path. It's common to take this approach in build systems that rely on tools that might not be installed.

I think we should close this PR.

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.

3 participants