Source code generation policy

[Mirko-von-Leipzig]

We generate code in a few build.rs across our various repos. Our current policy has been to generate this code in-tree, allowing us to audit this as part of code reviews in PRs.

General guidelines

This policy does go against the common guideline that code generation should be done out-of-tree and never checked into source control. This is also the policy that the Rust reference suggests:

Build scripts may save any output files or intermediate artifacts in the directory specified in the OUT_DIR environment variable. Scripts should not modify any files outside of that directory.

Why revisit this

Our policy has bitten us recently. Additional code generation was added to build.rs, and the generated files were added to the rerun-if-changed pragma (which is perfectly valid). However this caused the build script to always trigger, causing a full recompile each time. This was patched in #991, but I have to admit I still don't fully understand why it was happening. Probably it didn't like a trigger of a generated file; though that feels like it should be allowed to prevent accidental mutation post-generation.

Pros/Cons of current policy

Positives:

• audit of generated code
• generated code is (more) static between computers ("works on my machine syndrome")

Negatives:

• requires additional CI checks to ensure checked-in code matches generated code
• does not follow guidelines, can lead to silent issues like the above and doubts
• PRs are noisier e.g. have to review proto files and generated proto code (not really, but its there); sort of the corollary of our pro :)

[Mirko-von-Leipzig]

Personally I prefer the out-of-tree approach. But I've been significantly jaded by CMake's handling of such things; where it regularly runs afoul of similar issues with rebuilding and miscompiling.

If we are concerned with specific generated code audits e.g. rust files that we manually write our in the build script, then we could experiment auditing this by moving the format string into its own separate file. This file can then be include! by the build script's write!(format!(...)) invocation. The benefit would be we could inspect the "file" in its non-string form, where the only missing parts would be the placeholder {} parts. This should still give syntax highlighting if we exclude the outer quotes.

^^ haven't actually tried this; but I don't see why it wouldn't simply work :)

[bobbinth]

I somewhat prefer the in-tree approach because of the reasons you've listed in "positives" - i.e., the code that gets checked into the repo is all you need to build the project. But I also understand the downsides - so, this is not a super strong preference.

I wonder if we could simplify the rule to be something like: running cargo build and similar command should not execute the build scripts at all (e.g,. by gating them with env variables) - they get executed only via the make commands only.

This way, if I want to re-build some parts of the source code, I can always run make build but cargo build always works with "static" code. This may address the second point from your "negatives" list - though, the other two remain as is.

[Mirko-von-Leipzig]

I'd like to revive this discussion, cc @TomasArrachea.

In particular because of a PR reworking our node proto files 0xMiden/miden-node#723, and because its again caused some major CI regressions fixed in #1230.

I've also personally run into the issue that changing proto files does not cause rebuilds because one has to override it.

[PhilippGackstatter]

I often run into full rebuilds of miden-base that I don't understand why they're happening. This could have also been due to passing RUSTFLAGS="-C debug-assertions" to some commands and not to others which was removed in #1230. I suspect that BUILD_GENERATED_FILES_IN_SRC=1 is also responsible for some of these though. So I'd say +1 for trying the out-of-tree approach.

[bobbinth]

One thing to consider is that sometimes we need to do things like this, and while doing them out-of-tree is also possible, having the generated code committed, makes it a bit easier.

[Mirko-von-Leipzig]

tbh I think that's even more reason to keep it out-of-tree. The rust pipeline is not built for this:

/// Defines whether the build script should generate files in `/src`.
/// The docs.rs build pipeline has a read-only filesystem, so we have to avoid writing to `src`,
/// otherwise the docs will fail to build there. Note that writing to `OUT_DIR` is fine.
const CODEGEN: bool = option_env!("CODEGEN").is_some();

rust-lang/cargo#5073

https://doc.rust-lang.org/cargo/reference/build-script-examples.html#code-generation

[PhilippGackstatter]

One other downside of generated files in src is that such files shouldn't really matter when merging or rebasing, but since they're committed to src, they do cause conflicts during rebasing. The kernel_v0.rs file is a good example of this.