From 013e7ce2eb883ccb573d7fed1c3d878da756aaa1 Mon Sep 17 00:00:00 2001 From: Michael-F-Bryan Date: Mon, 18 Jul 2022 23:59:31 +0800 Subject: [PATCH 1/3] Return a more specific error from Model::from_path() --- crates/fj-host/src/lib.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index 910dd342f..c6a5c62e4 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -50,13 +50,12 @@ impl Model { pub fn from_path( path: PathBuf, target_dir: Option, - ) -> io::Result { + ) -> Result { let crate_dir = path.canonicalize()?; let metadata = cargo_metadata::MetadataCommand::new() .current_dir(&crate_dir) - .exec() - .map_err(metadata_error_to_io)?; + .exec()?; let pkg = package_associated_with_directory(&metadata, &crate_dir)?; let src_path = crate_dir.join("src"); @@ -207,13 +206,6 @@ impl Model { } } -fn metadata_error_to_io(e: cargo_metadata::Error) -> std::io::Error { - match e { - cargo_metadata::Error::Io(io) => io, - _ => std::io::Error::new(io::ErrorKind::Other, e), - } -} - fn package_associated_with_directory<'m>( metadata: &'m cargo_metadata::Metadata, dir: &Path, @@ -334,6 +326,11 @@ pub enum Error { /// Error while watching the model code for changes #[error("Error watching model for changes")] Notify(#[from] notify::Error), + + /// An error occurred while trying to use evaluate + /// [`cargo_metadata::MetadataCommand`]. + #[error("Unable to determine the crate's metadata")] + CargoMetadata(#[from] cargo_metadata::Error), } type ModelFn = unsafe extern "C" fn(args: &Parameters) -> fj::Shape; From 82b5f6c8221ef670422fe6688317cb94876f68a9 Mon Sep 17 00:00:00 2001 From: Michael-F-Bryan Date: Tue, 19 Jul 2022 00:13:12 +0800 Subject: [PATCH 2/3] Provide a more useful specific error message when we can't determine a model's directory --- crates/fj-host/src/lib.rs | 46 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index c6a5c62e4..0d094ca5b 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -209,7 +209,7 @@ impl Model { fn package_associated_with_directory<'m>( metadata: &'m cargo_metadata::Metadata, dir: &Path, -) -> Result<&'m cargo_metadata::Package, io::Error> { +) -> Result<&'m cargo_metadata::Package, Error> { for pkg in metadata.workspace_packages() { let crate_dir = pkg .manifest_path @@ -221,9 +221,33 @@ fn package_associated_with_directory<'m>( } } - let msg = format!("\"{}\" doesn't point to a crate", dir.display()); + Err(ambiguous_path_error(metadata, dir)) +} - Err(io::Error::new(io::ErrorKind::Other, msg)) +fn ambiguous_path_error( + metadata: &cargo_metadata::Metadata, + dir: &Path, +) -> Error { + let possible_paths = metadata + .workspace_members + .iter() + .map(|id| PathBuf::from(&metadata[id].manifest_path)) + .map(|mut cargo_toml_path| { + let _ = cargo_toml_path.pop(); + cargo_toml_path + }) + .map(|crate_dir| { + crate_dir + .strip_prefix(&metadata.workspace_root) + .unwrap_or(&crate_dir) + .to_path_buf() + }) + .collect(); + + Error::AmbiguousPath { + dir: dir.to_path_buf(), + possible_paths, + } } /// Watches a model for changes, reloading it continually @@ -331,6 +355,22 @@ pub enum Error { /// [`cargo_metadata::MetadataCommand`]. #[error("Unable to determine the crate's metadata")] CargoMetadata(#[from] cargo_metadata::Error), + + /// The user pointed us to a directory, but it doesn't look like that was + /// a crate root (i.e. the folder containing `Cargo.toml`). + #[error( + "It doesn't look like \"{}\" is a crate directory. Did you mean one of {}?", + dir.display(), + possible_paths.iter().map(|p| p.display().to_string()) + .collect::>() + .join(", ") + )] + AmbiguousPath { + /// The model directory supplied by the user. + dir: PathBuf, + /// The directories for each crate in the workspace. + possible_paths: Vec, + }, } type ModelFn = unsafe extern "C" fn(args: &Parameters) -> fj::Shape; From 72a633a5e60a4e3570b67317eb64eaaf01787a3c Mon Sep 17 00:00:00 2001 From: Michael-F-Bryan Date: Tue, 19 Jul 2022 00:19:42 +0800 Subject: [PATCH 3/3] Rewrite the ambiguous path error function for readability --- crates/fj-host/src/lib.rs | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index 0d094ca5b..362d635f6 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -228,21 +228,21 @@ fn ambiguous_path_error( metadata: &cargo_metadata::Metadata, dir: &Path, ) -> Error { - let possible_paths = metadata - .workspace_members - .iter() - .map(|id| PathBuf::from(&metadata[id].manifest_path)) - .map(|mut cargo_toml_path| { - let _ = cargo_toml_path.pop(); - cargo_toml_path - }) - .map(|crate_dir| { - crate_dir - .strip_prefix(&metadata.workspace_root) - .unwrap_or(&crate_dir) - .to_path_buf() - }) - .collect(); + let mut possible_paths = Vec::new(); + + for id in &metadata.workspace_members { + let cargo_toml = &metadata[id].manifest_path; + let crate_dir = cargo_toml + .parent() + .expect("A Cargo.toml always has a parent"); + // Try to make the path relative to the workspace root so error messages + // aren't super long. + let simplified_path = crate_dir + .strip_prefix(&metadata.workspace_root) + .unwrap_or(crate_dir); + + possible_paths.push(simplified_path.into()); + } Error::AmbiguousPath { dir: dir.to_path_buf(), @@ -368,7 +368,8 @@ pub enum Error { AmbiguousPath { /// The model directory supplied by the user. dir: PathBuf, - /// The directories for each crate in the workspace. + /// The directories for each crate in the workspace, relative to the + /// workspace root. possible_paths: Vec, }, }