fix: only rebuild proto on changes #412

Mirko-von-Leipzig · 2024-07-17T11:08:47Z

The proto files are being "rebuilt" on every compile at the moment. This is due to the

    println!("cargo:rerun-if-changed=generated");

which is a directory that does not exist, causing it to trigger on every build.

There were several attempts at this; none were really good enough. My last attempt is changing very little but adds a few consistency improvements.

Notably, it adds a simple CI check to ensure changes the main proto files is propagated to all repo's which use them.

To aid this, I've made the build script outputs more repeatable by

deleting target files and folders
sorting outputs

Mirko-von-Leipzig · 2024-07-17T11:16:38Z

As a side note - it feels weird putting "fixes" under Enhancements in the changelog. Should there also be a Fixes?

bobbinth

Looks good! Thank you! I left one question inline.

I am however unsure why the generated directory is included at all -- maybe to rebuild it if it accidentally got deleted? Though that seems excessive as the dev can see this happened using git.

I believe the motivation for this is described in #9 (comment) - but I'm not sure if it is still valid.

As a side note - it feels weird putting "fixes" under Enhancements in the changelog. Should there also be a Fixes?

Yes, there should be Fixes section as well. Overall, I think we should have 3 categories (or maybe more) in the changelog: Features, Enhancements, and Fixes (we had 2 of these in the last release).

crates/proto/build.rs

bobbinth

Thank you! This is not a super-deep review, but I did leave a couple of small comments inline. Regarding the general approach, I have the following comments:

I would prefer to keep proto and rpc-proto crates separate. The main reason is that rpc-proto is meant to be the interface definition only for the RPC component of the node (it doesn't quite work like that right now, but that's a separate issue) and it is meant to be consumed by external users. While proto is mean to contain interfaces of all node components and to be used primarily internally.
I do like adding the make proto command to the Makefile and having this be the only way to re-build generated files and copy protobuf files into rpc-proto. Whether the implementation of this command relies on a separate binary or build scripts gated by an environment variable is not super important - but I do think that using just the build scripts may be cleaner here.
I think it would be great if we added a CI check to make sure that updates to .proto files are properly propagated (i.e., someone didn't forget to run the make proto command).

So, my recommendations for this PR would be:

Revert the merging of proto and rpc-proto crates.
Add proto command to the Makefile.
a. If possible, rely on the build scripts to recompile/copy the protobuf files. For example, maybe this command would just do BUILD_PROTO=1 cargo build and the build scripts would be fully conditional on BUILD_PROTO variable.
b. If the above doesn't work for some reason, create a separate binary to perform this tasks (similar to what is already in this PR).
Add a CI check to make sure make proto command does not cause a diff.

bin/compile-proto/src/main.rs

bobbinth · 2024-07-18T20:04:42Z

Cargo.toml

  "crates/rpc",
  "crates/store",
  "crates/utils",
  "crates/test-macro",
 ]
-default-members = ["bin/faucet", "bin/node", "crates/proto", "crates/rpc-proto"]
+default-members = ["bin/faucet", "bin/node", "crates/proto"]


I'm wondering: do we need to have default-members at all? If we don't have it, cargo build etc. commands will be applied to all crates, right? Not sure that's a bad thing.

Correct; its actually somewhat annoying to have as you always have to include the package details for non-default-members.

Mirko-von-Leipzig · 2024-07-19T10:43:53Z

I really wish github allowed replying to top level comments..

@bobbinth what about a more explicit approach of moving ownership of the proto definitions to the proto crate. We can then

Remove the env flag trigger from the proto/build.rs since the definitions are now part of the crate filesystem (I think that was the issue?).
Add the functionality required by rpc-proto to proto under some feature flag (sort of how my current impl merged the two). Really this would only require the actual file contents be made available under a nostd implementation.
Have rpc-proto use proto as a dependency with no need for build.rs

Benefits:

A single copy of the proto definitions.
A single "pure" build.rs with no bool flags, no copying around.
The current (temporary) dependency of rpc-proto is made explicit.
Non-breaking change for both crates.

Mirko-von-Leipzig · 2024-07-19T12:49:17Z

I've implemented my latest idea.

I'm waiting on CI to double-check that I don't need some nostd related feature-gating.

--edit-- ah right this CI doesn't have nostd checks. Hmm. I can compile it locally without the std feature enabled but I'm unsure if that's sufficient evidence.

I'm hoping to avoid adding std feature gates to the proto crate just to re-export proto::PROTO_FILES in rpc-proto which I believe should be fine.

Mirko-von-Leipzig · 2024-07-19T12:54:29Z

crates/rpc-proto/src/lib.rs

@@ -3,8 +3,7 @@
 #[cfg(feature = "std")]
 extern crate std;

-mod proto_files;
-pub use proto_files::PROTO_FILES;
+pub use miden_node_proto::PROTO_FILES;


Is exporting a nostd compliant const from an otherwise non-compliant dependency fine? I think it is -- haven't experimented much with nostd.

crates/proto/build.rs

Mirko-von-Leipzig · 2024-07-19T15:55:11Z

We can still add a CI check if we want; but should be difficult to get wrong with the build script

bobbinth · 2024-07-22T06:13:21Z

I think this approach works, but I'm not sure I like it better than what we had before (assuming we get rid of the un-needed rebuilds).

One reason is that I think there is benefit to keeping .proto files separate from the Rust code. These files describe the interfaces of the node components regardless of the language these components are implemented in. Granted, we don't have any plans right now for any non-Rust implementations, but if someone wanted to grab these files to build a Go implementation of the node, it would be a bit more natural to get them from the repo root rather than from one of the internal crates.

Another reason is that as I mentioned, ideally rpc-proto shouldn't pull in any of the files related to storage/block producer. We don't have this working yet because of the way we wrote the .proto files and refactoring them is a pretty low priority, but it feels a bit hacky to couple proto and rpc-proto crates.

So, unless there are some complications (i.e., conditional build scripts don't work for some reason), I'd still prefer to keep the .proto files in the repo root.

Mirko-von-Leipzig · 2024-07-22T11:00:56Z

I've reverted to the original format and placing. I've just added a CI check which rebuilds and ensures there is no diff.

bobbinth

All looks good! Thank you!

bobbinth · 2024-07-22T20:37:38Z

crates/proto/build.rs

+/// Generate `mod.rs` which includes all files in the folder as submodules.
+fn generate_mod_rs(directory: impl AsRef<Path>) -> std::io::Result<()> {


Nice catch!

Mirko-von-Leipzig force-pushed the proto-rebuild-on-change branch from 1a7b766 to 8398d00 Compare July 17, 2024 11:12

bobbinth approved these changes Jul 17, 2024

View reviewed changes

crates/proto/build.rs Outdated Show resolved Hide resolved

Mirko-von-Leipzig marked this pull request as draft July 18, 2024 08:18

Mirko-von-Leipzig marked this pull request as ready for review July 18, 2024 13:54

bobbinth requested changes Jul 18, 2024

View reviewed changes

Mirko-von-Leipzig force-pushed the proto-rebuild-on-change branch from e70b8e6 to 1d2e19c Compare July 19, 2024 12:46

Mirko-von-Leipzig commented Jul 19, 2024

View reviewed changes

Mirko-von-Leipzig requested a review from bobbinth July 19, 2024 12:55

igamigo reviewed Jul 19, 2024

View reviewed changes

crates/proto/build.rs Outdated Show resolved Hide resolved

crates/proto/build.rs Outdated Show resolved Hide resolved

Mirko-von-Leipzig force-pushed the proto-rebuild-on-change branch from 398d8c6 to 069dc2d Compare July 19, 2024 15:53

Mirko-von-Leipzig added 2 commits July 22, 2024 11:26

feat: improve proto build scripts

1769869

Add a lint for proto files

8a224de

Mirko-von-Leipzig force-pushed the proto-rebuild-on-change branch 2 times, most recently from fbd5d66 to 8a224de Compare July 22, 2024 10:07

Mirko-von-Leipzig added 6 commits July 22, 2024 12:09

Fix workflow

1da496d

Workflow again

8ab47df

fixup build files

677b591

fixup workflow

fe1d13c

full run

18514e9

changelog

2a90553

bobbinth and others added 2 commits July 22, 2024 13:20

Merge branch 'next' into proto-rebuild-on-change

728c61e

fix: typo

d18df7b

bobbinth approved these changes Jul 22, 2024

View reviewed changes

bobbinth merged commit bc5da6b into next Jul 22, 2024
9 checks passed

bobbinth deleted the proto-rebuild-on-change branch July 22, 2024 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: only rebuild proto on changes #412

fix: only rebuild proto on changes #412

Mirko-von-Leipzig commented Jul 17, 2024 •

edited

Loading

Mirko-von-Leipzig commented Jul 17, 2024

bobbinth left a comment

bobbinth left a comment

bobbinth Jul 18, 2024

Mirko-von-Leipzig Jul 19, 2024

Mirko-von-Leipzig commented Jul 19, 2024 •

edited

Loading

Mirko-von-Leipzig commented Jul 19, 2024 •

edited

Loading

Mirko-von-Leipzig Jul 19, 2024

Mirko-von-Leipzig commented Jul 19, 2024

bobbinth commented Jul 22, 2024

Mirko-von-Leipzig commented Jul 22, 2024

bobbinth left a comment

bobbinth Jul 22, 2024

		/// Generate `mod.rs` which includes all files in the folder as submodules.
		fn generate_mod_rs(directory: impl AsRef<Path>) -> std::io::Result<()> {

fix: only rebuild proto on changes #412

fix: only rebuild proto on changes #412

Conversation

Mirko-von-Leipzig commented Jul 17, 2024 • edited Loading

Mirko-von-Leipzig commented Jul 17, 2024

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth Jul 18, 2024

Choose a reason for hiding this comment

Mirko-von-Leipzig Jul 19, 2024

Choose a reason for hiding this comment

Mirko-von-Leipzig commented Jul 19, 2024 • edited Loading

Mirko-von-Leipzig commented Jul 19, 2024 • edited Loading

Mirko-von-Leipzig Jul 19, 2024

Choose a reason for hiding this comment

Mirko-von-Leipzig commented Jul 19, 2024

bobbinth commented Jul 22, 2024

Mirko-von-Leipzig commented Jul 22, 2024

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth Jul 22, 2024

Choose a reason for hiding this comment

Mirko-von-Leipzig commented Jul 17, 2024 •

edited

Loading

Mirko-von-Leipzig commented Jul 19, 2024 •

edited

Loading

Mirko-von-Leipzig commented Jul 19, 2024 •

edited

Loading