Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: lock on target directories not Nargo.toml #7538

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Feb 26, 2025

Description

Problem*

Resolves #7542

Summary*

This PR moves the locks which are currently applied to Nargo.toml files onto the target directory. This allows users to run commands in parallel more easily without any potential for clashing by just specifying a separate build directory.

As an example of this, I've moved the integration tests to use their own target directory per test so compiling for acir/brillig and with different inliner settings don't conflict with each other.

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@TomAFrench TomAFrench requested a review from a team February 26, 2025 20:11
@TomAFrench TomAFrench enabled auto-merge February 26, 2025 23:55
@aakoshh
Copy link
Contributor

aakoshh commented Feb 27, 2025

In #7345 I changed the locking so when nargo is invoked on the workspace, it should still lock the individual Nargo.toml of the packages, and thus the parallel nargo compile pattern should have not experienced any contention between packages, while still writing to the common target directory of the workspace itself. Within the target directory they just wrote JSON files, so there was no other output directory to lock.

I understand your change would allow concurrent nargo compile commands to run on the same package as long as the output directory is different, so you can improve the 6 integration tests that we run for each test directory. How does it handle the workspace scenario?

fs2::FileExt::lock_shared(&file)
.unwrap_or_else(|e| panic!("Failed to lock {path_display}: {e}"));
}
let lock_path = target_dir.join(".nargo-lock");
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest locking on a package specific lock file in the output directory, to allow the compilations in aztec-packages to go concurrently without all waiting on this single .nargo-lock to be free.

}
Ok(locks)

Ok(LockedFile(file))
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also suggest removing the lock files to not leave around hundreds of them, but maybe that's not important. Or maybe they can all be in a target/locks directory, so nobody is bothered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add file lock on target directory to prevent multiple commands using the same output dir concurrently.
2 participants