Skip to content

refactor: avoid unnecessary asm/ tree copy in standards build script#2538

Draft
mmagician wants to merge 2 commits intonextfrom
claude/review-pr-2452-qd5DI
Draft

refactor: avoid unnecessary asm/ tree copy in standards build script#2538
mmagician wants to merge 2 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
/// Recursively copies `src` into `dst`.
///
/// This function will overwrite the existing files if re-executed.
pub fn copy_directory<T: AsRef<Path>, R: AsRef<Path>>(
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter Mar 4, 2026

Choose a reason for hiding this comment

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

Can we do the same removal in the other build.rs scripts in protocol and agglayer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I think it can be applied similarly to the agglayer crate.

Re: protocol, we cannot use the same pattern. The reason is the copy_shared_modules function, which copies asm/shared_modules/ into both asm/kernels/transaction/lib/ and asm/protocol/ before compilation. This mutates the directory layout so the assembler can resolve imports like $kernel::account_id.

I'd put this PR on hold until we re-enable the agglayer crate though, because otherwise we can't really know if the build process works well or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Re: protocol, we cannot use the same pattern. The reason is the copy_shared_modules function, which copies asm/shared_modules/ into both asm/kernels/transaction/lib/ and asm/protocol/ before compilation. This mutates the directory layout so the assembler can resolve imports like $kernel::account_id.

Makes sense! Then we can remove it once we can set up a proper project for this shared library (0xMiden/miden-vm#2510).

@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?

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