Skip to content

Commit

Permalink
feat(cli): Add --target-dir option (#7350)
Browse files Browse the repository at this point in the history
  • Loading branch information
aakoshh authored Feb 12, 2025
1 parent 31becc6 commit 1b6ba5d
Show file tree
Hide file tree
Showing 7 changed files with 52 additions and 17 deletions.
3 changes: 3 additions & 0 deletions cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
"envrc",
"EXPONENTIATE",
"Flamegraph",
"flamegraphs",
"flate",
"fmtstr",
"foldl",
Expand Down Expand Up @@ -120,6 +121,7 @@
"impls",
"indexmap",
"injective",
"interners",
"Inlines",
"instrumenter",
"interner",
Expand Down Expand Up @@ -227,6 +229,7 @@
"tempdir",
"tempfile",
"termcolor",
"termion",
"thiserror",
"tslog",
"turbofish",
Expand Down
1 change: 1 addition & 0 deletions tooling/lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ pub(crate) fn resolve_workspace_for_source_path(file_path: &Path) -> Result<Work
members: vec![assumed_package],
selected_package_index: Some(0),
is_assumed: true,
target_dir: None,
};
Ok(workspace)
}
Expand Down
2 changes: 1 addition & 1 deletion tooling/lsp/src/requests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,7 @@ pub(crate) fn find_all_references_in_workspace(
));
}

// The LSP client usually removes duplicate loctions, but we do it here just in case they don't
// The LSP client usually removes duplicate locations, but we do it here just in case they don't
locations.sort_by_key(|location| {
(
location.uri.to_string(),
Expand Down
4 changes: 3 additions & 1 deletion tooling/nargo/src/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ use crate::{
#[derive(Clone)]
pub struct Workspace {
pub root_dir: PathBuf,
/// Optional target directory override.
pub target_dir: Option<PathBuf>,
pub members: Vec<Package>,
// If `Some()`, the `selected_package_index` is used to select the only `Package` when iterating a Workspace
pub selected_package_index: Option<usize>,
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion tooling/nargo_cli/benches/criterion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,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] {
Expand Down
52 changes: 39 additions & 13 deletions tooling/nargo_cli/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,12 @@ 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, value_parser = parse_path)]
target_dir: Option<PathBuf>,
}

/// Options for commands that work on either workspace or package scope.
Expand Down Expand Up @@ -130,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),
Expand Down Expand Up @@ -194,14 +191,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 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, 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 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,
Expand Down Expand Up @@ -249,18 +249,44 @@ fn lock_workspace(workspace: &Workspace, exclusive: bool) -> Result<Vec<impl Dro
Ok(locks)
}

/// Parses a path and turns it into an absolute one by joining to the current directory.
fn parse_path(path: &str) -> Result<PathBuf, String> {
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 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"));
assert!(err
.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());
}
}
5 changes: 4 additions & 1 deletion tooling/nargo_toml/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,7 @@ fn toml_to_workspace(
selected_package_index: Some(0),
members: vec![member],
is_assumed: false,
target_dir: None,
},
}
}
Expand Down Expand Up @@ -448,6 +449,7 @@ fn toml_to_workspace(
members,
selected_package_index,
is_assumed: false,
target_dir: None,
}
}
};
Expand Down Expand Up @@ -514,14 +516,15 @@ 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<String>,
) -> Result<Workspace, ManifestError> {
let nargo_toml = read_toml(toml_path)?;
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)?;
}
Expand Down

0 comments on commit 1b6ba5d

Please sign in to comment.