From 0864bf1441815ed6669e33c51e5d881d086a27ff Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 12 Feb 2025 10:56:15 +0000 Subject: [PATCH 1/5] Add Workspace::target_dir --- tooling/lsp/src/lib.rs | 2 ++ tooling/lsp/src/requests/mod.rs | 2 +- tooling/lsp/src/requests/test_run.rs | 1 + tooling/lsp/src/requests/tests.rs | 1 + tooling/nargo/src/workspace.rs | 4 +++- tooling/nargo_cli/benches/criterion.rs | 3 ++- tooling/nargo_cli/src/cli/compile_cmd.rs | 2 +- tooling/nargo_cli/src/cli/dap_cmd.rs | 1 + tooling/nargo_cli/src/cli/mod.rs | 12 +++++++++--- tooling/nargo_toml/src/lib.rs | 9 +++++++-- 10 files changed, 28 insertions(+), 9 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index f8a4bd6b74a..d1bf34861fb 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -274,6 +274,7 @@ pub(crate) fn resolve_workspace_for_source_path(file_path: &Path) -> Result return Ok(workspace), Err(error) => { @@ -316,6 +317,7 @@ pub(crate) fn resolve_workspace_for_source_path(file_path: &Path) -> Result, pub members: Vec, // If `Some()`, the `selected_package_index` is used to select the only `Package` when iterating a Workspace pub selected_package_index: Option, @@ -34,7 +36,7 @@ impl Workspace { } pub fn target_directory_path(&self) -> PathBuf { - self.root_dir.join(TARGET_DIR) + self.target_dir.as_ref().cloned().unwrap_or_else(|| self.root_dir.join(TARGET_DIR)) } pub fn export_directory_path(&self) -> PathBuf { diff --git a/tooling/nargo_cli/benches/criterion.rs b/tooling/nargo_cli/benches/criterion.rs index e43e498ea06..b8ff1844aae 100644 --- a/tooling/nargo_cli/benches/criterion.rs +++ b/tooling/nargo_cli/benches/criterion.rs @@ -42,6 +42,7 @@ fn read_compiled_programs_and_inputs( &toml_path, nargo_toml::PackageSelection::All, Some(noirc_driver::NOIR_ARTIFACT_VERSION_STRING.to_string()), + None, ) .expect("failed to resolve workspace"); @@ -156,7 +157,7 @@ fn criterion_test_execution(c: &mut Criterion, test_program_dir: &Path, force_br }); } -/// Go through all the selected tests and executem with and without Brillig. +/// Go through all the selected tests and execute them with and without Brillig. fn criterion_selected_tests_execution(c: &mut Criterion) { for test_program_dir in get_selected_tests() { for force_brillig in [false, true] { diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index bb08d2675cb..a6873ddb79a 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -388,7 +388,7 @@ mod tests { let verbose = matches!(sel, PackageSelection::Selected(_)); let test_workspaces = read_test_program_dirs(&test_programs_dir(), "execution_success") - .filter_map(|dir| read_workspace(&dir, sel.clone()).ok()) + .filter_map(|dir| read_workspace(&dir, None, sel.clone()).ok()) .collect::>(); assert!(!test_workspaces.is_empty(), "should find some test workspaces"); diff --git a/tooling/nargo_cli/src/cli/dap_cmd.rs b/tooling/nargo_cli/src/cli/dap_cmd.rs index 91db844ead3..b058a696214 100644 --- a/tooling/nargo_cli/src/cli/dap_cmd.rs +++ b/tooling/nargo_cli/src/cli/dap_cmd.rs @@ -80,6 +80,7 @@ fn find_workspace(project_folder: &str, package: Option<&str>) -> Option Some(workspace), Err(err) => { diff --git a/tooling/nargo_cli/src/cli/mod.rs b/tooling/nargo_cli/src/cli/mod.rs index efc6a351e33..67951908cb0 100644 --- a/tooling/nargo_cli/src/cli/mod.rs +++ b/tooling/nargo_cli/src/cli/mod.rs @@ -58,6 +58,10 @@ pub(crate) struct NargoConfig { // REMINDER: Also change this flag in the LSP test lens if renamed #[arg(long, hide = true, global = true, default_value = "./")] program_dir: PathBuf, + + /// Override the default target directory. + #[arg(long, hide = true, global = true)] + target_dir: Option, } /// Options for commands that work on either workspace or package scope. @@ -168,6 +172,7 @@ pub(crate) fn start_cli() -> eyre::Result<()> { /// Read a given program directory into a workspace. fn read_workspace( program_dir: &Path, + target_dir: Option<&Path>, selection: PackageSelection, ) -> Result { let toml_path = get_package_manifest(program_dir)?; @@ -176,6 +181,7 @@ fn read_workspace( &toml_path, selection, Some(NOIR_ARTIFACT_VERSION_STRING.to_owned()), + target_dir, )?; Ok(workspace) @@ -194,14 +200,14 @@ where // or a specific package; if that's the case then parse the package name to select it in the workspace. let selection = match cmd.package_selection() { PackageSelection::DefaultOrAll if workspace_dir != package_dir => { - let workspace = read_workspace(&package_dir, PackageSelection::DefaultOrAll)?; - let package = workspace.into_iter().next().expect("there should be exactly 1 package"); + let package = read_workspace(&package_dir, None, PackageSelection::DefaultOrAll)?; + let package = package.into_iter().next().expect("there should be exactly 1 package"); PackageSelection::Selected(package.name.clone()) } other => other, }; // Parse the top level workspace with the member selected. - let workspace = read_workspace(&workspace_dir, selection)?; + let workspace = read_workspace(&workspace_dir, config.target_dir.as_ref(), selection)?; // Lock manifests if the command needs it. let _locks = match cmd.lock_type() { LockType::None => None, diff --git a/tooling/nargo_toml/src/lib.rs b/tooling/nargo_toml/src/lib.rs index 59bee569430..2378ef0ee76 100644 --- a/tooling/nargo_toml/src/lib.rs +++ b/tooling/nargo_toml/src/lib.rs @@ -396,6 +396,7 @@ fn toml_to_workspace( selected_package_index: Some(0), members: vec![member], is_assumed: false, + target_dir: None, }, } } @@ -448,6 +449,7 @@ fn toml_to_workspace( members, selected_package_index, is_assumed: false, + target_dir: None, } } }; @@ -514,14 +516,17 @@ pub enum PackageSelection { } /// Resolves a Nargo.toml file into a `Workspace` struct as defined by our `nargo` core. +/// +/// As a side effect it downloads project dependencies as well. pub fn resolve_workspace_from_toml( toml_path: &Path, package_selection: PackageSelection, current_compiler_version: Option, + target_dir_path: Option<&Path>, ) -> Result { let nargo_toml = read_toml(toml_path)?; - let workspace = toml_to_workspace(nargo_toml, package_selection)?; - + let mut workspace = toml_to_workspace(nargo_toml, package_selection)?; + workspace.target_dir = target_dir_path.map(|p| p.to_path_buf()); if let Some(current_compiler_version) = current_compiler_version { semver::semver_check_workspace(&workspace, current_compiler_version)?; } From 66b5d78236dd4349a01914f515fee715d858e4b4 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 12 Feb 2025 11:01:18 +0000 Subject: [PATCH 2/5] Remove the mostly unused target_dir parameter from TOML resolvers --- tooling/lsp/src/lib.rs | 1 - tooling/lsp/src/requests/test_run.rs | 1 - tooling/lsp/src/requests/tests.rs | 1 - tooling/nargo_cli/benches/criterion.rs | 1 - tooling/nargo_cli/src/cli/compile_cmd.rs | 2 +- tooling/nargo_cli/src/cli/dap_cmd.rs | 1 - tooling/nargo_cli/src/cli/mod.rs | 9 +++++---- tooling/nargo_toml/src/lib.rs | 4 +--- 8 files changed, 7 insertions(+), 13 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index d1bf34861fb..c722bfdfd3e 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -274,7 +274,6 @@ pub(crate) fn resolve_workspace_for_source_path(file_path: &Path) -> Result return Ok(workspace), Err(error) => { diff --git a/tooling/lsp/src/requests/test_run.rs b/tooling/lsp/src/requests/test_run.rs index 6b7aad298a1..e2d8edd46c8 100644 --- a/tooling/lsp/src/requests/test_run.rs +++ b/tooling/lsp/src/requests/test_run.rs @@ -44,7 +44,6 @@ fn on_test_run_request_inner( &toml_path, PackageSelection::Selected(crate_name.clone()), Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), - None, // We're not reading or producing artifacts here. ) .map_err(|err| { // If we found a manifest, but the workspace is invalid, we raise an error about it diff --git a/tooling/lsp/src/requests/tests.rs b/tooling/lsp/src/requests/tests.rs index e474e20cde4..81910bebedb 100644 --- a/tooling/lsp/src/requests/tests.rs +++ b/tooling/lsp/src/requests/tests.rs @@ -44,7 +44,6 @@ fn on_tests_request_inner( &toml_path, PackageSelection::All, Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), - None, ) .map_err(|err| { // If we found a manifest, but the workspace is invalid, we raise an error about it diff --git a/tooling/nargo_cli/benches/criterion.rs b/tooling/nargo_cli/benches/criterion.rs index b8ff1844aae..0722e957833 100644 --- a/tooling/nargo_cli/benches/criterion.rs +++ b/tooling/nargo_cli/benches/criterion.rs @@ -42,7 +42,6 @@ fn read_compiled_programs_and_inputs( &toml_path, nargo_toml::PackageSelection::All, Some(noirc_driver::NOIR_ARTIFACT_VERSION_STRING.to_string()), - None, ) .expect("failed to resolve workspace"); diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index a6873ddb79a..bb08d2675cb 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -388,7 +388,7 @@ mod tests { let verbose = matches!(sel, PackageSelection::Selected(_)); let test_workspaces = read_test_program_dirs(&test_programs_dir(), "execution_success") - .filter_map(|dir| read_workspace(&dir, None, sel.clone()).ok()) + .filter_map(|dir| read_workspace(&dir, sel.clone()).ok()) .collect::>(); assert!(!test_workspaces.is_empty(), "should find some test workspaces"); diff --git a/tooling/nargo_cli/src/cli/dap_cmd.rs b/tooling/nargo_cli/src/cli/dap_cmd.rs index b058a696214..91db844ead3 100644 --- a/tooling/nargo_cli/src/cli/dap_cmd.rs +++ b/tooling/nargo_cli/src/cli/dap_cmd.rs @@ -80,7 +80,6 @@ fn find_workspace(project_folder: &str, package: Option<&str>) -> Option Some(workspace), Err(err) => { diff --git a/tooling/nargo_cli/src/cli/mod.rs b/tooling/nargo_cli/src/cli/mod.rs index 67951908cb0..0cf397b4638 100644 --- a/tooling/nargo_cli/src/cli/mod.rs +++ b/tooling/nargo_cli/src/cli/mod.rs @@ -172,7 +172,6 @@ pub(crate) fn start_cli() -> eyre::Result<()> { /// Read a given program directory into a workspace. fn read_workspace( program_dir: &Path, - target_dir: Option<&Path>, selection: PackageSelection, ) -> Result { let toml_path = get_package_manifest(program_dir)?; @@ -181,7 +180,6 @@ fn read_workspace( &toml_path, selection, Some(NOIR_ARTIFACT_VERSION_STRING.to_owned()), - target_dir, )?; Ok(workspace) @@ -200,14 +198,17 @@ where // or a specific package; if that's the case then parse the package name to select it in the workspace. let selection = match cmd.package_selection() { PackageSelection::DefaultOrAll if workspace_dir != package_dir => { - let package = read_workspace(&package_dir, None, PackageSelection::DefaultOrAll)?; + let package = read_workspace(&package_dir, PackageSelection::DefaultOrAll)?; let package = package.into_iter().next().expect("there should be exactly 1 package"); PackageSelection::Selected(package.name.clone()) } other => other, }; // Parse the top level workspace with the member selected. - let workspace = read_workspace(&workspace_dir, config.target_dir.as_ref(), selection)?; + let mut workspace = read_workspace(&workspace_dir, selection)?; + // Optionally override the target directory. It's only done here because most commands like the LSP and DAP + // don't read or write artifacts, so they don't use the target directory. + workspace.target_dir = config.target_dir.clone(); // Lock manifests if the command needs it. let _locks = match cmd.lock_type() { LockType::None => None, diff --git a/tooling/nargo_toml/src/lib.rs b/tooling/nargo_toml/src/lib.rs index 2378ef0ee76..edf26411cf5 100644 --- a/tooling/nargo_toml/src/lib.rs +++ b/tooling/nargo_toml/src/lib.rs @@ -522,11 +522,9 @@ pub fn resolve_workspace_from_toml( toml_path: &Path, package_selection: PackageSelection, current_compiler_version: Option, - target_dir_path: Option<&Path>, ) -> Result { let nargo_toml = read_toml(toml_path)?; - let mut workspace = toml_to_workspace(nargo_toml, package_selection)?; - workspace.target_dir = target_dir_path.map(|p| p.to_path_buf()); + let workspace = toml_to_workspace(nargo_toml, package_selection)?; if let Some(current_compiler_version) = current_compiler_version { semver::semver_check_workspace(&workspace, current_compiler_version)?; } From eca9cdc5eb2b1146910f8d0d97ba750d121d5e4a Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 12 Feb 2025 11:09:32 +0000 Subject: [PATCH 3/5] Normalize both root and target paths --- tooling/nargo_cli/src/cli/mod.rs | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/tooling/nargo_cli/src/cli/mod.rs b/tooling/nargo_cli/src/cli/mod.rs index 0cf397b4638..179ae7c530c 100644 --- a/tooling/nargo_cli/src/cli/mod.rs +++ b/tooling/nargo_cli/src/cli/mod.rs @@ -56,11 +56,11 @@ struct NargoCli { #[derive(Args, Clone, Debug)] pub(crate) struct NargoConfig { // REMINDER: Also change this flag in the LSP test lens if renamed - #[arg(long, hide = true, global = true, default_value = "./")] + #[arg(long, hide = true, global = true, default_value = "./", value_parser = parse_path)] program_dir: PathBuf, /// Override the default target directory. - #[arg(long, hide = true, global = true)] + #[arg(long, hide = true, global = true, value_parser = parse_path)] target_dir: Option, } @@ -134,14 +134,7 @@ enum LockType { #[cfg(not(feature = "codegen-docs"))] #[tracing::instrument(level = "trace")] pub(crate) fn start_cli() -> eyre::Result<()> { - use fm::NormalizePath; - - let NargoCli { command, mut config } = NargoCli::parse(); - - // If the provided `program_dir` is relative, make it absolute by joining it to the current directory. - if !config.program_dir.is_absolute() { - config.program_dir = std::env::current_dir().unwrap().join(config.program_dir).normalize(); - } + let NargoCli { command, config } = NargoCli::parse(); match command { NargoCommand::New(args) => new_cmd::run(args, config), @@ -256,6 +249,16 @@ fn lock_workspace(workspace: &Workspace, exclusive: bool) -> Result Result { + use fm::NormalizePath; + let mut path: PathBuf = path.parse().map_err(|e| format!("failed to parse path: {e}"))?; + if !path.is_absolute() { + path = std::env::current_dir().unwrap().join(path).normalize(); + } + Ok(path) +} + #[cfg(test)] mod tests { use clap::Parser; From 5ed34ac7cce74679834ca6e63aeffa4208c2e9b9 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 12 Feb 2025 11:16:59 +0000 Subject: [PATCH 4/5] Test that the target dir is set --- tooling/nargo_cli/src/cli/mod.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/tooling/nargo_cli/src/cli/mod.rs b/tooling/nargo_cli/src/cli/mod.rs index 179ae7c530c..1d59fdb806f 100644 --- a/tooling/nargo_cli/src/cli/mod.rs +++ b/tooling/nargo_cli/src/cli/mod.rs @@ -261,11 +261,13 @@ fn parse_path(path: &str) -> Result { #[cfg(test)] mod tests { + use super::NargoCli; use clap::Parser; + #[test] fn test_parse_invalid_expression_width() { let cmd = "nargo --program-dir . compile --expression-width 1"; - let res = super::NargoCli::try_parse_from(cmd.split_ascii_whitespace()); + let res = NargoCli::try_parse_from(cmd.split_ascii_whitespace()); let err = res.expect_err("should fail because of invalid width"); assert!(err.to_string().contains("expression-width")); @@ -273,4 +275,18 @@ mod tests { .to_string() .contains(acvm::compiler::MIN_EXPRESSION_WIDTH.to_string().as_str())); } + + #[test] + fn test_parse_target_dir() { + let cmd = "nargo --program-dir . --target-dir ../foo/bar execute"; + let cli = NargoCli::try_parse_from(cmd.split_ascii_whitespace()).expect("should parse"); + + let target_dir = cli.config.target_dir.expect("should parse target dir"); + assert!(target_dir.is_absolute(), "should be made absolute"); + assert!(target_dir.ends_with("foo/bar")); + + let cmd = "nargo --program-dir . execute"; + let cli = NargoCli::try_parse_from(cmd.split_ascii_whitespace()).expect("should parse"); + assert!(cli.config.target_dir.is_none()); + } } From 3dab839ca2340bf9f9697ff13bdc5f1ae2fc7f00 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 12 Feb 2025 11:18:58 +0000 Subject: [PATCH 5/5] Add some words to cspell --- cspell.json | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cspell.json b/cspell.json index de712fb0a04..c7ad22afa96 100644 --- a/cspell.json +++ b/cspell.json @@ -93,6 +93,7 @@ "envrc", "EXPONENTIATE", "Flamegraph", + "flamegraphs", "flate", "fmtstr", "foldl", @@ -120,6 +121,7 @@ "impls", "indexmap", "injective", + "interners", "Inlines", "instrumenter", "interner", @@ -227,6 +229,7 @@ "tempdir", "tempfile", "termcolor", + "termion", "thiserror", "tslog", "turbofish",