diff --git a/crates/nargo/src/cli/compile_cmd.rs b/crates/nargo/src/cli/compile_cmd.rs index 0511ed6a456..b239319d44c 100644 --- a/crates/nargo/src/cli/compile_cmd.rs +++ b/crates/nargo/src/cli/compile_cmd.rs @@ -7,6 +7,7 @@ use std::path::Path; use clap::Args; +use crate::resolver::DependencyResolutionError; use crate::{constants::TARGET_DIR, errors::CliError, resolver::Resolver}; use super::fs::program::save_program_to_file; @@ -66,7 +67,7 @@ pub(crate) fn run(args: CompileCommand, config: NargoConfig) -> Result<(), CliEr } } -fn setup_driver(program_dir: &Path) -> Result { +fn setup_driver(program_dir: &Path) -> Result { let backend = crate::backends::ConcreteBackend; Resolver::resolve_root_manifest(program_dir, backend.np_language()) } diff --git a/crates/nargo/src/errors.rs b/crates/nargo/src/errors.rs index 3e53b2df809..1fd86818852 100644 --- a/crates/nargo/src/errors.rs +++ b/crates/nargo/src/errors.rs @@ -4,6 +4,8 @@ use noirc_abi::errors::{AbiError, InputParserError}; use std::path::PathBuf; use thiserror::Error; +use crate::resolver::DependencyResolutionError; + #[derive(Debug, Error)] pub(crate) enum CliError { #[error("{0}")] @@ -23,6 +25,9 @@ pub(crate) enum CliError { #[error("Failed to verify proof {}", .0.display())] InvalidProof(PathBuf), + #[error(transparent)] + ResolutionError(#[from] DependencyResolutionError), + /// Error while compiling Noir into ACIR. #[error("Failed to compile circuit")] CompilationError, diff --git a/crates/nargo/src/lib.rs b/crates/nargo/src/lib.rs index e9e519d3335..7c5a988e458 100644 --- a/crates/nargo/src/lib.rs +++ b/crates/nargo/src/lib.rs @@ -2,8 +2,9 @@ #![warn(unused_crate_dependencies, unused_extern_crates)] #![warn(unreachable_pub)] -// Necessary as we use `color_eyre` in `main.rs`. -use color_eyre as _; +//! Nargo is the package manager for Noir +//! This name was used because it sounds like `cargo` and +//! Noir Package Manager abbreviated is npm, which is already taken. use noirc_frontend::graph::CrateType; use std::{ @@ -11,15 +12,6 @@ use std::{ path::{Path, PathBuf}, }; -use crate::errors::CliError; -// Nargo is the package manager for Noir -// This name was used because it sounds like `cargo` and -// Noir Package Manager abbreviated is npm, which is already taken. - -fn nargo_crates() -> PathBuf { - dirs::home_dir().unwrap().join("nargo") -} - mod backends; pub mod cli; mod constants; @@ -28,45 +20,36 @@ mod git; mod manifest; mod resolver; +use manifest::InvalidPackageError; + +fn nargo_crates() -> PathBuf { + dirs::home_dir().unwrap().join("nargo") +} + /// Searches for the Nargo.toml file /// /// XXX: In the end, this should find the root of the project and check /// for the Nargo.toml file there /// However, it should only do this after checking the current path /// This allows the use of workspace settings in the future. -fn find_package_manifest(current_path: &Path) -> Result { - match find_file(current_path, "Nargo", "toml") { - Some(p) => Ok(p), - None => Err(CliError::Generic(format!( - "cannot find a Nargo.toml in {}", - current_path.display() - ))), - } +fn find_package_manifest(current_path: &Path) -> Result { + find_file(current_path, "Nargo", "toml") + .ok_or_else(|| InvalidPackageError::MissingManifestFile(current_path.to_path_buf())) } -fn lib_or_bin(current_path: &Path) -> Result<(PathBuf, CrateType), CliError> { +fn lib_or_bin(current_path: &Path) -> Result<(PathBuf, CrateType), InvalidPackageError> { // A library has a lib.nr and a binary has a main.nr // You cannot have both. - let src_path = match find_dir(current_path, "src") { - Some(path) => path, - None => { - return Err(CliError::Generic(format!( - "cannot find src file in path {}", - current_path.display() - ))) - } - }; + let src_path = find_dir(current_path, "src") + .ok_or_else(|| InvalidPackageError::NoSourceDir(current_path.to_path_buf()))?; + let lib_nr_path = find_file(&src_path, "lib", "nr"); let bin_nr_path = find_file(&src_path, "main", "nr"); match (lib_nr_path, bin_nr_path) { - (Some(_), Some(_)) => Err(CliError::Generic( - "package cannot contain both a `lib.nr` and a `main.nr`".to_owned(), - )), + (Some(_), Some(_)) => Err(InvalidPackageError::ContainsMultipleCrates), (None, Some(path)) => Ok((path, CrateType::Binary)), (Some(path), None) => Ok((path, CrateType::Library)), - (None, None) => Err(CliError::Generic( - "package must contain either a `lib.nr`(Library) or a `main.nr`(Binary).".to_owned(), - )), + (None, None) => Err(InvalidPackageError::ContainsZeroCrates), } } diff --git a/crates/nargo/src/manifest.rs b/crates/nargo/src/manifest.rs index 11476442eff..990b6aefb72 100644 --- a/crates/nargo/src/manifest.rs +++ b/crates/nargo/src/manifest.rs @@ -1,8 +1,31 @@ use serde::Deserialize; use std::collections::BTreeMap; -use std::path::Path; +use std::path::{Path, PathBuf}; +use thiserror::Error; -use crate::errors::CliError; +/// Errors covering situations where a package is either missing or malformed. +#[derive(Debug, Error)] +pub(crate) enum InvalidPackageError { + /// Package doesn't have a manifest file + #[error("cannot find a Nargo.toml in {}", .0.display())] + MissingManifestFile(PathBuf), + + /// Package manifest is unreadable. + #[error("Nargo.toml is badly formed, could not parse.\n\n {0}")] + MalformedManifestFile(toml::de::Error), + + /// Package does not contain Noir source files. + #[error("cannot find src directory in path {}", .0.display())] + NoSourceDir(PathBuf), + + /// Package has neither of `main.nr` and `lib.nr`. + #[error("package must contain either a `lib.nr`(Library) or a `main.nr`(Binary).")] + ContainsZeroCrates, + + /// Package has both a `main.nr` (for binaries) and `lib.nr` (for libraries) + #[error("package cannot contain both a `lib.nr` and a `main.nr`")] + ContainsMultipleCrates, +} #[derive(Debug, Deserialize, Clone)] pub(crate) struct PackageManifest { @@ -53,32 +76,18 @@ pub(crate) enum Dependency { /// Parses a Nargo.toml file from it's path /// The path to the toml file must be present. /// Calling this function without this guarantee is an ICE. -pub(crate) fn parse>(path_to_toml: P) -> Result { +pub(crate) fn parse>( + path_to_toml: P, +) -> Result { let toml_as_string = std::fs::read_to_string(&path_to_toml).expect("ice: path given for toml file is invalid"); - match parse_toml_str(&toml_as_string) { - Ok(cfg) => Ok(cfg), - Err(msg) => { - let path = path_to_toml.as_ref(); - Err(CliError::Generic(format!("{}\n Location: {}", msg, path.display()))) - } - } + parse_toml_str(&toml_as_string) } -fn parse_toml_str(toml_as_string: &str) -> Result { - match toml::from_str::(toml_as_string) { - Ok(cfg) => Ok(cfg), - Err(err) => { - let mut message = "input.toml file is badly formed, could not parse\n\n".to_owned(); - // XXX: This error is not always that helpful, but it gives the line number - // and the entry, in those cases - // which is useful for telling the user where the error occurred - // XXX: maybe there is a way to extract ErrorInner from toml - message.push_str(&err.to_string()); - Err(message) - } - } +fn parse_toml_str(toml_as_string: &str) -> Result { + toml::from_str::(toml_as_string) + .map_err(InvalidPackageError::MalformedManifestFile) } #[test] diff --git a/crates/nargo/src/resolver.rs b/crates/nargo/src/resolver.rs index 1302f724835..c7349de86ce 100644 --- a/crates/nargo/src/resolver.rs +++ b/crates/nargo/src/resolver.rs @@ -6,10 +6,12 @@ use std::{ use acvm::Language; use noirc_driver::Driver; use noirc_frontend::graph::{CrateId, CrateName, CrateType}; +use thiserror::Error; use crate::{ - errors::CliError, + git::clone_git_repo, manifest::{Dependency, PackageManifest}, + InvalidPackageError, }; /// Creates a unique folder name for a GitHub repo @@ -21,6 +23,27 @@ pub(crate) fn resolve_folder_name(base: &url::Url, tag: &str) -> String { folder_name } +/// Errors covering situations where a crate's dependency tree cannot be resolved. +#[derive(Debug, Error)] +pub(crate) enum DependencyResolutionError { + /// Encountered error while downloading git repository. + #[error("{0}")] + GitError(String), + + /// Attempted to depend on a binary crate. + #[error("dependency {dep_pkg_name} is a binary package and so it cannot be depended upon.")] + BinaryDependency { dep_pkg_name: String }, + + /// Attempted to depend on remote crate which has a local dependency. + /// We have no guarantees that this local dependency will be available so must error. + #[error("remote(git) dependency has a local dependency.\ndependency located at {}", dependency_path.display())] + RemoteDepWithLocalDep { dependency_path: PathBuf }, + + /// Dependency is not a valid crate + #[error(transparent)] + MalformedDependency(#[from] InvalidPackageError), +} + #[derive(Debug, Clone)] struct CachedDep { entry_path: PathBuf, @@ -52,7 +75,7 @@ impl<'a> Resolver<'a> { pub(crate) fn resolve_root_manifest( dir_path: &std::path::Path, np_language: Language, - ) -> Result { + ) -> Result { let mut driver = Driver::new(&np_language); let (entry_path, crate_type) = super::lib_or_bin(dir_path)?; @@ -78,7 +101,7 @@ impl<'a> Resolver<'a> { &mut self, parent_crate: CrateId, manifest: PackageManifest, - ) -> Result<(), CliError> { + ) -> Result<(), DependencyResolutionError> { // First download and add these top level dependencies crates to the Driver for (dep_pkg_name, pkg_src) in manifest.dependencies.iter() { let (dir_path, dep_meta) = Resolver::cache_dep(pkg_src)?; @@ -86,9 +109,9 @@ impl<'a> Resolver<'a> { let (entry_path, crate_type) = (&dep_meta.entry_path, &dep_meta.crate_type); if crate_type == &CrateType::Binary { - return Err(CliError::Generic(format!( - "{dep_pkg_name} is a binary package and so it cannot be depended upon. src : {pkg_src:?}" - ))); + return Err(DependencyResolutionError::BinaryDependency { + dep_pkg_name: dep_pkg_name.to_string(), + }); } let crate_id = self.driver.create_non_local_crate(entry_path, *crate_type); @@ -98,12 +121,11 @@ impl<'a> Resolver<'a> { } // Resolve all transitive dependencies - for (dir_path, (crate_id, dep_meta)) in self.cached_packages.iter() { + for (dependency_path, (crate_id, dep_meta)) in self.cached_packages.iter() { if dep_meta.remote && manifest.has_local_path() { - return Err(CliError::Generic(format!( - "remote(git) dependency depends on a local path. \ndependency located at {}", - dir_path.display() - ))); + return Err(DependencyResolutionError::RemoteDepWithLocalDep { + dependency_path: dependency_path.to_path_buf(), + }); } let mut new_res = Resolver::with_driver(self.driver); new_res.resolve_manifest(*crate_id, dep_meta.manifest.clone())?; @@ -117,8 +139,11 @@ impl<'a> Resolver<'a> { /// /// If it's a local path, the same applies, however it will not /// be downloaded - fn cache_dep(dep: &Dependency) -> Result<(PathBuf, CachedDep), CliError> { - fn retrieve_meta(dir_path: &Path, remote: bool) -> Result { + fn cache_dep(dep: &Dependency) -> Result<(PathBuf, CachedDep), DependencyResolutionError> { + fn retrieve_meta( + dir_path: &Path, + remote: bool, + ) -> Result { let (entry_path, crate_type) = super::lib_or_bin(dir_path)?; let manifest_path = super::find_package_manifest(dir_path)?; let manifest = super::manifest::parse(manifest_path)?; @@ -127,7 +152,8 @@ impl<'a> Resolver<'a> { match dep { Dependency::Github { git, tag } => { - let dir_path = Resolver::resolve_git_dep(git, tag)?; + let dir_path = + clone_git_repo(git, tag).map_err(DependencyResolutionError::GitError)?; let meta = retrieve_meta(&dir_path, true)?; Ok((dir_path, meta)) } @@ -138,13 +164,6 @@ impl<'a> Resolver<'a> { } } } - - fn resolve_git_dep(url: &str, tag: &str) -> Result { - match super::git::clone_git_repo(url, tag) { - Ok(path) => Ok(path), - Err(msg) => Err(CliError::Generic(msg)), - } - } } // This needs to be public to support the tests in `cli/mod.rs`.