Skip to content

refactor: remove unnecessary asm/ tree copy from standards and agglayer build scripts#2538

Open
mmagician wants to merge 5 commits intonextfrom
claude/review-pr-2452-qd5DI
Open

refactor: remove unnecessary asm/ tree copy from standards and agglayer build scripts#2538
mmagician wants to merge 5 commits intonextfrom
claude/review-pr-2452-qd5DI

Conversation

@mmagician
Copy link
Collaborator

miden-standards: Remove the full asm/ directory copy to OUT_DIR entirely.
This crate never mutates its source tree, so the assembler and error
extractor can read directly from the crate's asm/ directory.

miden-protocol: Replace the bulk copy of the entire asm/ tree with
targeted staging of only the two directories that need modification
(kernels/transaction/ and protocol/). The assembler requires shared
modules to be physically present alongside these source files, but
shared_utils/ and shared_modules/ themselves don't need to be copied.
Event extraction now reads directly from the original source.

Also simplifies copy_dir_recursive (replacing the old copy_directory
with its awkward prefix-stripping API) and removes dead code.

https://claude.ai/code/session_01HDd5o3XxcgZiGrvBDFsUr1

refactor: scope change to standards build only, leave protocol as-is

The protocol crate needs source-level staging because the assembler's
`$kernel::` import resolution requires shared modules to be physically
co-located with kernel source. This cannot be avoided without assembler
changes, so revert the protocol build.rs to the base branch version.

https://claude.ai/code/session_01HDd5o3XxcgZiGrvBDFsUr1
@mmagician mmagician added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Mar 2, 2026
@mmagician mmagician marked this pull request as ready for review March 2, 2026 22:02
@mmagician mmagician marked this pull request as draft March 5, 2026 08:06
@PhilippGackstatter
Copy link
Contributor

Something else we can do after #2452 is removing BUILD_GENERATED_FILES_IN_SRC (not urgent). Do you want to include that in this PR or should we do it separately?

mmagician and others added 2 commits March 17, 2026 09:15
Read MASM sources directly from the crate's asm/ directory instead of
copying them to OUT_DIR first. The copy is unnecessary because agglayer
doesn't mutate the directory structure during compilation.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@mmagician mmagician changed the title refactor: avoid unnecessary asm/ tree copy in standards build script refactor: remove unnecessary asm/ tree copy from standards and agglayer build scripts Mar 17, 2026
@mmagician mmagician force-pushed the claude/review-pr-2452-qd5DI branch from 0becf7d to 302dcf0 Compare March 17, 2026 13:37
@mmagician mmagician marked this pull request as ready for review March 17, 2026 17:51
Comment on lines -47 to +46
generate_canonical_zeros(&src.join(ASM_AGGLAYER_BRIDGE_DIR))?;
let crate_path = Path::new(&crate_dir);
generate_canonical_zeros(&crate_path.join(ASM_DIR).join(ASM_AGGLAYER_BRIDGE_DIR))?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the only two things left writing into src are the canonical zeros and the error constants. Can we generate the error constants in agglayer the same way as in miden-standards, by writing to OUT_DIR?

Then we'd only have the canonical zeros left. These shouldn't really change, right? If so, we could consider generating them with a script once and have the build.rs only validate their correctness without writing into src. Then we would be fully compliant with the "don't write into src policy from rust/cargo" and could remove BUILD_GENERATED_FILES_IN_SRC.

Another strategy for removing the canonical zeros from src is to keep the copy_directory approach for agglayer MASM code. We copy the MASM source to OUT_DIR and generate the canonical zeros directly in the appropriate location within the MASM source code in OUT_DIR. Similar to how the miden-protocol build.rs script creates the kernel and protocol libraries with the shared_utils and account_id.masm. Wdyt?

PR otherwise lgtm 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog This PR does not require an entry in the `CHANGELOG.md` file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants