Skip to content

Commit

Permalink
feat: lock on Nargo.toml on several nargo commands (#6941)
Browse files Browse the repository at this point in the history
  • Loading branch information
asterite authored Jan 6, 2025
1 parent da94c2b commit 54d81ca
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 38 deletions.
22 changes: 11 additions & 11 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,6 @@ noir_lsp = { path = "tooling/lsp" }
noir_debugger = { path = "tooling/debugger" }
noirc_abi = { path = "tooling/noirc_abi" }
noirc_artifacts = { path = "tooling/noirc_artifacts" }
bb_abstraction_leaks = { path = "tooling/bb_abstraction_leaks" }
acvm_cli = { path = "tooling/acvm_cli" }

# Arkworks
ark-bn254 = { version = "^0.5.0", default-features = false, features = [
Expand Down
4 changes: 2 additions & 2 deletions tooling/nargo_cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ tracing.workspace = true
tracing-subscriber.workspace = true
tracing-appender = "0.2.3"
clap_complete = "4.5.36"
fs2 = "0.4.3"

[target.'cfg(not(unix))'.dependencies]
tokio-util = { version = "0.7.8", features = ["compat"] }
Expand All @@ -86,7 +87,6 @@ dirs.workspace = true
assert_cmd = "2.0.8"
assert_fs = "1.0.10"
predicates = "2.1.5"
file-lock = "2.1.11"
fm.workspace = true
criterion.workspace = true
pprof.workspace = true
Expand All @@ -102,7 +102,7 @@ light-poseidon = "0.2.0"
ark-bn254-v04 = { package = "ark-bn254", version = "^0.4.0", default-features = false, features = [
"curve",
] }
ark-ff-v04 = { package = "ark-ff", version = "^0.4.0", default-features = false }
ark-ff-v04 = { package = "ark-ff", version = "^0.4.0", default-features = false }

[[bench]]
name = "criterion"
Expand Down
23 changes: 0 additions & 23 deletions tooling/nargo_cli/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,33 +177,13 @@ fn generate_test_cases(
}
let test_cases = test_cases.join("\n");

// We need to isolate test cases in the same group, otherwise they overwrite each other's artifacts.
// On CI we use `cargo nextest`, which runs tests in different processes; for this we use a file lock.
// Locally we might be using `cargo test`, which run tests in the same process; in this case the file lock
// wouldn't work, becuase the process itself has the lock, and it looks like it can have N instances without
// any problems; for this reason we also use a `Mutex`.
let mutex_name = format! {"TEST_MUTEX_{}", test_name.to_uppercase()};
write!(
test_file,
r#"
lazy_static::lazy_static! {{
/// Prevent concurrent tests in the matrix from overwriting the compilation artifacts in {test_dir}
static ref {mutex_name}: std::sync::Mutex<()> = std::sync::Mutex::new(());
}}
{test_cases}
fn test_{test_name}(force_brillig: ForceBrillig, inliner_aggressiveness: Inliner) {{
let test_program_dir = PathBuf::from("{test_dir}");
// Ignore poisoning errors if some of the matrix cases failed.
let mutex_guard = {mutex_name}.lock().unwrap_or_else(|e| e.into_inner());
let file_guard = file_lock::FileLock::lock(
test_program_dir.join("Nargo.toml"),
true,
file_lock::FileOptions::new().read(true).write(true).append(true)
).expect("failed to lock Nargo.toml");
let mut nargo = Command::cargo_bin("nargo").unwrap();
nargo.arg("--program-dir").arg(test_program_dir);
nargo.arg("{test_command}").arg("--force");
Expand All @@ -221,9 +201,6 @@ fn test_{test_name}(force_brillig: ForceBrillig, inliner_aggressiveness: Inliner
}}
{test_content}
drop(file_guard);
drop(mutex_guard);
}}
"#
)
Expand Down
48 changes: 48 additions & 0 deletions tooling/nargo_cli/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ enum NargoCommand {
#[cfg(not(feature = "codegen-docs"))]
#[tracing::instrument(level = "trace")]
pub(crate) fn start_cli() -> eyre::Result<()> {
use std::fs::File;

use fs2::FileExt as _;
use nargo_toml::get_package_manifest;

let NargoCli { command, mut config } = NargoCli::parse();

// If the provided `program_dir` is relative, make it absolute by joining it to the current directory.
Expand All @@ -130,6 +135,28 @@ pub(crate) fn start_cli() -> eyre::Result<()> {
config.program_dir = program_dir;
}

let lock_file = match needs_lock(&command) {
Some(exclusive) => {
let toml_path = get_package_manifest(&config.program_dir)?;
let file = File::open(toml_path).expect("Expected Nargo.toml to exist");
if exclusive {
if file.try_lock_exclusive().is_err() {
eprintln!("Waiting for lock on Nargo.toml...");
}

file.lock_exclusive().expect("Failed to lock Nargo.toml");
} else {
if file.try_lock_shared().is_err() {
eprintln!("Waiting for lock on Nargo.toml...");
}

file.lock_shared().expect("Failed to lock Nargo.toml");
}
Some(file)
}
None => None,
};

match command {
NargoCommand::New(args) => new_cmd::run(args, config),
NargoCommand::Init(args) => init_cmd::run(args, config),
Expand All @@ -146,6 +173,10 @@ pub(crate) fn start_cli() -> eyre::Result<()> {
NargoCommand::GenerateCompletionScript(args) => generate_completion_script_cmd::run(args),
}?;

if let Some(lock_file) = lock_file {
lock_file.unlock().expect("Failed to unlock Nargo.toml");
}

Ok(())
}

Expand Down Expand Up @@ -193,6 +224,23 @@ fn command_dir(cmd: &NargoCommand, program_dir: &Path) -> Result<Option<PathBuf>
Ok(Some(nargo_toml::find_root(program_dir, workspace)?))
}

fn needs_lock(cmd: &NargoCommand) -> Option<bool> {
match cmd {
NargoCommand::Check(..)
| NargoCommand::Compile(..)
| NargoCommand::Execute(..)
| NargoCommand::Export(..)
| NargoCommand::Info(..) => Some(true),
NargoCommand::Debug(..) | NargoCommand::Test(..) => Some(false),
NargoCommand::Fmt(..)
| NargoCommand::New(..)
| NargoCommand::Init(..)
| NargoCommand::Lsp(..)
| NargoCommand::Dap(..)
| NargoCommand::GenerateCompletionScript(..) => None,
}
}

#[cfg(test)]
mod tests {
use clap::Parser;
Expand Down

0 comments on commit 54d81ca

Please sign in to comment.