`zcash_client_backend`: Avoid writing to `src` dir in `build.rs`

nejucomo commented 3 months ago

Background / Motivation

While attempting to create a nix flake for zingolib, I hit a blocker due to the zcash_client_backend build script writing into src.

The nix build system aims for deterministic builds, and one mechanism it uses by default is ensuring all package sources are read-only during the build process. For this specific case a flake.nix-specific work-around would be to copy the source to a temporary read/write directory. However, this feels like a work-around for what I assume to be a work-around in zcash_client_backend, so this ticket is about addressing the upstream non-nix-specific behavior.

(Also: this is the first time I've seen the crane build library for nix fail on a cargo crate for this reason, so I think it is unusual/disrecommended practice in rust land.)

Current Status

I started looking into why zcash_client_backend does this, and see the comments such as this in build.rs that explain the rationale as allowing revision-controlled edits of the generated source. I glanced at the blame for one of the generated-and-also-manually-edited files and I can't yet tell which changes are the manual edits without more digging.

Are manual edits still required?

My guess is that they were necessary because protoc failed to include some rust attributes or some other tweak. Hopefully, it can now include all necessary changes in the build.rs inputs or .proto files?

I am not sure I can answer this without feedback from the maintainers, perhaps @str4d or @nuttycom. In fact, I currently don't understand why the manual edits aren't overwritten by the protoc output when I build.

Next Steps

If manual edits are no longer necessary, the fix should be dirt simple: remove the fs::copy lines in build.rs.

If not, the next step I would take is determine what customizations to protoc output are needed, see if newer versions support that, and/or investigate protoc alternatives like https://github.com/stepancheg/rust-protobuf/ (which also incidentally simplifies system prerequisites by avoiding "then install protoc in the README.md). ;-)

nejucomo commented 3 months ago

I went ahead with the nix-specific workaround downstream: https://github.com/zingolabs/zingolib/pull/1214#issuecomment-2168156897

In any case, I still think it's worth completing this ticket as a tech-debt improvement, although it does not seem urgent from my perspective.

str4d commented 3 months ago

I started looking into why zcash_client_backend does this, and see the comments such as this in build.rs that explain the rationale as allowing revision-controlled edits of the generated source.

That is not at all what this says. The quoted comment is "Copy the generated types into the source tree so changes can be committed"; the changes here are made by the generator (either due to changes to the source .proto files or to the generator itself), not manual edits.

I glanced at the blame for one of the generated-and-also-manually-edited files and I can't yet tell which changes are the manual edits without more digging.

This file is not generated. zcash_client_backend/proto/*.proto are the Protobuf files, from which the Rust code is generated. More specifically:

compact_formats.proto and proposal.proto are canonically defined in this repository.
service.proto is canonically defined in https://github.com/zcash/lightwalletd and duplicated here.

The core question here is "why is the build.rs writing into zcash_client_backend/src?" and the answer is "usability".

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, and we didn't check in the generated source code. We then switched to prost; its codegen isn't implemented in pure Rust and instead calls out to the protoc binary, and we did not want downstream crate users to be required to have protoc installed.

So we compromised by checking in the generated files and adding build.rs machinery to always keep things in-sync. The generated files need to live in zcash_client_backend/src because when a user doesn't have protoc installed, the files need to exist in the published crate (so we can't use the standard trick for generated files of having a placeholder in src/ that includes the contents of the generated source in target/, as the latter will not exist).

To be clear, the build script does nothing unless:

the protobuf files are present (which they are not in published crate versions, but I presume are in your nix flake).
protoc is available in PATH.

Thus you can ensure that no writes happen in zcash_client_backend/src by either deleting the files in zcash_client_backend/proto, or ensuring that protoc is not in the PATH.

nejucomo commented 3 months ago

Ah, I see. I misunderstood "…so changes can be committed" as implying human-made changes.

So the work-around of not having protoc on the path is sort of feasible: zingolib needs protoc to build. It is easy to distinguish building transitive dependencies outside of the cargo workspace vs the cargo workspace itself when building the nix package or running automated tests.

There is a wrinkle (for this downstream nix use case) is that nix develop is designed to set up a shell with "all prerequisites correctly configured" to build a given project. In this case, protoc is absent as the workaround described above, so then a third customization reinstalls that so that cargo build succeeds in that nix develop shell.

I'm not aware of any other use case where the source tree is read-only, so it seems like this is a lower priority vs enabling other cargo environments to build without protoc.

zcash / librustzcash