diff --git a/src/error.rs b/src/error.rs index b19f731..809e1b4 100644 --- a/src/error.rs +++ b/src/error.rs @@ -16,6 +16,10 @@ pub enum Error { UnexpectedWorkspace(PathBuf), NoPackageInManifest(PathBuf), PackageNotFound(PathBuf, String), + ManifestNotInWorkspace { + manifest: PathBuf, + workspace_manifest: PathBuf, + }, Io(PathBuf, IoError), Toml(PathBuf, TomlError), } @@ -52,6 +56,22 @@ impl Display for Error { workspace.display() ) } + Self::ManifestNotInWorkspace { + manifest, + workspace_manifest, + } => { + return write!(f, "current package believes it's in a workspace when it's not: +current: {} +workspace: {workspace_manifest_path} + +this may be fixable by adding `{package_subpath}` to the `workspace.members` array of the manifest located at: {workspace_manifest_path} +Alternatively, to keep it out of the workspace, add an empty `[workspace]` table to the package's manifest.", + // TODO: Parse workspace.exclude and add back "add the package to the `workspace.exclude` array, or" + manifest.display(), + package_subpath = manifest.parent().unwrap().strip_prefix(workspace_manifest.parent().unwrap()).unwrap().display(), + workspace_manifest_path = workspace_manifest.display(), + ) + }, Self::Io(path, error) => return write!(f, "{}: {}", path.display(), error), Self::Toml(file, error) => return write!(f, "{}: {}", file.display(), error), }) diff --git a/src/manifest.rs b/src/manifest.rs index 6552a6b..f25aec7 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -1,6 +1,11 @@ -use crate::error::{Error, Result}; use serde::Deserialize; -use std::path::Path; +use std::{ + collections::HashMap, + path::{Path, PathBuf}, +}; + +use crate::error::{Error, Result}; +use crate::utils; #[derive(Clone, Debug, Deserialize)] pub struct Manifest { @@ -13,6 +18,67 @@ impl Manifest { let contents = std::fs::read_to_string(path).map_err(|e| Error::Io(path.to_owned(), e))?; toml::from_str(&contents).map_err(|e| Error::Toml(path.to_owned(), e)) } + + /// Returns a mapping from manifest directory to manifest path and loaded manifest + pub fn members(&self, workspace_root: &Path) -> Result> { + let workspace = self + .workspace + .as_ref() + .ok_or(Error::ManifestNotAWorkspace)?; + let workspace_root = utils::canonicalize(workspace_root)?; + + // Check all member packages inside the workspace + let mut all_members = HashMap::new(); + + for member in &workspace.members { + for manifest_dir in glob::glob(workspace_root.join(member).to_str().unwrap())? { + let manifest_dir = manifest_dir?; + let manifest_path = manifest_dir.join("Cargo.toml"); + let manifest = Manifest::parse_from_toml(&manifest_path)?; + + // Workspace members cannot themselves be/contain a new workspace + if manifest.workspace.is_some() { + return Err(Error::UnexpectedWorkspace(manifest_path)); + } + + // And because they cannot contain a [workspace], they may not be a virtual manifest + // and must hence contain [package] + if manifest.package.is_none() { + return Err(Error::NoPackageInManifest(manifest_path)); + } + + all_members.insert(manifest_dir, (manifest_path, manifest)); + } + } + + Ok(all_members) + } + + /// Returns `self` if it contains `[package]` but not `[workspace]`, (i.e. it cannot be + /// a workspace nor a virtual manifest), and describes a package named `name` if not [`None`]. + pub fn map_nonvirtual_package( + self, + manifest_path: PathBuf, + name: Option<&str>, + ) -> Result<(PathBuf, Self)> { + if self.workspace.is_some() { + return Err(Error::UnexpectedWorkspace(manifest_path)); + } + + if let Some(package) = &self.package { + if let Some(name) = name { + if package.name == name { + Ok((manifest_path, self)) + } else { + Err(Error::PackageNotFound(manifest_path, name.into())) + } + } else { + Ok((manifest_path, self)) + } + } else { + Err(Error::NoPackageInManifest(manifest_path)) + } + } } #[derive(Clone, Debug, Deserialize)] diff --git a/src/subcommand.rs b/src/subcommand.rs index c07411c..3d9638d 100644 --- a/src/subcommand.rs +++ b/src/subcommand.rs @@ -53,21 +53,22 @@ impl Subcommand { |manifest_path| utils::canonicalize(manifest_path.parent().unwrap()), )?; - // Scan the given and all parent directories for a Cargo.toml containing a workspace + // Scan up the directories based on --manifest-path and the working directory to find a Cargo.toml + let potential_manifest = utils::find_manifest(&search_path)?; + // Perform the same scan, but for a Cargo.toml containing [workspace] let workspace_manifest = utils::find_workspace(&search_path)?; - let (manifest_path, manifest) = if let Some((workspace_manifest_path, workspace)) = - &workspace_manifest - { - // If a workspace was found, find packages relative to it - let selector = match package { - Some(name) => utils::PackageSelector::ByName(name), - None => utils::PackageSelector::ByPath(&search_path), - }; - utils::find_package_manifest_in_workspace(workspace_manifest_path, workspace, selector)? - } else { - // Otherwise scan up the directories based on --manifest-path and the working directory. - utils::find_package_manifest(&search_path, package)? + let (manifest_path, manifest) = { + if let Some(workspace_manifest) = &workspace_manifest { + utils::find_package_manifest_in_workspace( + workspace_manifest, + potential_manifest, + package, + )? + } else { + let (manifest_path, manifest) = potential_manifest; + manifest.map_nonvirtual_package(manifest_path, package)? + } }; // The manifest is known to contain a package at this point diff --git a/src/utils.rs b/src/utils.rs index db479b5..adc5a18 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,7 +1,6 @@ use crate::config::Config; use crate::error::{Error, Result}; use crate::manifest::Manifest; -use std::collections::HashMap; use std::ffi::OsStr; use std::path::{Path, PathBuf}; @@ -27,50 +26,35 @@ pub fn canonicalize(mut path: &Path) -> Result { dunce::canonicalize(path).map_err(|e| Error::Io(path.to_owned(), e)) } -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub enum PackageSelector<'a> { - ByName(&'a str), - ByPath(&'a Path), -} - /// Tries to find a package by the given `name` in the [workspace root] or member -/// of the given [workspace] [`Manifest`]. -/// -/// When a workspace is not detected, call [`find_package_manifest()`] instead. +/// of the given [workspace] [`Manifest`], and possibly falls back to a potential +/// manifest based on the working directory or `--manifest-path` as found by +/// [`find_manifest()`] and passed as argument to `potential_manifest`. /// /// [workspace root]: https://doc.rust-lang.org/cargo/reference/workspaces.html#root-package /// [workspace]: https://doc.rust-lang.org/cargo/reference/workspaces.html#workspaces pub fn find_package_manifest_in_workspace( - workspace_manifest_path: &Path, - workspace_manifest: &Manifest, - selector: PackageSelector<'_>, + (workspace_manifest_path, workspace_manifest): &(PathBuf, Manifest), + (potential_manifest_path, potential_manifest): (PathBuf, Manifest), + package_name: Option<&str>, ) -> Result<(PathBuf, Manifest)> { - let workspace = workspace_manifest - .workspace - .as_ref() - .ok_or(Error::ManifestNotAWorkspace)?; - let workspace_root = canonicalize(workspace_manifest_path.parent().unwrap())?; + let potential_manifest_dir = potential_manifest_path.parent().unwrap(); + let workspace_manifest_dir = workspace_manifest_path.parent().unwrap(); - // Check all member packages inside the workspace - let mut all_members = HashMap::new(); - - for member in &workspace.members { - for manifest_dir in glob::glob(workspace_root.join(member).to_str().unwrap())? { - let manifest_dir = manifest_dir?; - let manifest_path = manifest_dir.join("Cargo.toml"); - let manifest = Manifest::parse_from_toml(&manifest_path)?; - - // Workspace members cannot themselves be/contain a new workspace - if manifest.workspace.is_some() { - return Err(Error::UnexpectedWorkspace(manifest_path)); - } - - all_members.insert(manifest_dir, (manifest_path, manifest)); - } + let workspace_members = workspace_manifest.members(workspace_manifest_dir)?; + // Make sure the found workspace includes the manifest "specified" by the user via --manifest-path or $PWD + if workspace_manifest_path != &potential_manifest_path + && !workspace_members.contains_key(potential_manifest_dir) + { + return Err(Error::ManifestNotInWorkspace { + manifest: potential_manifest_path, + workspace_manifest: workspace_manifest_path.clone(), + }); } - match selector { - PackageSelector::ByName(name) => { + match package_name { + // Any package in the workspace can be used if `-p` is used + Some(name) => { // Check if the workspace manifest also contains a [package] if let Some(package) = &workspace_manifest.package { if package.name == name { @@ -82,44 +66,31 @@ pub fn find_package_manifest_in_workspace( } // Check all member packages inside the workspace - for (_manifest_dir, (manifest_path, manifest)) in all_members { - if let Some(package) = &manifest.package { - if package.name == name { - return Ok((manifest_path, manifest)); - } - } else { - return Err(Error::NoPackageInManifest(manifest_path)); + for (_manifest_dir, (manifest_path, manifest)) in workspace_members { + // .members() already checked for it having a package + let package = manifest.package.as_ref().unwrap(); + if package.name == name { + return Ok((manifest_path, manifest)); } } Err(Error::PackageNotFound( - workspace_manifest_path.into(), - name.into(), + workspace_manifest_path.clone(), + name.to_owned(), )) } - PackageSelector::ByPath(path) => { - let path = canonicalize(path)?; - - // Find the closest member based on the given path - Ok(path - .ancestors() - // Move manifest out of the HashMap - .find_map(|dir| all_members.remove(dir)) - .unwrap_or_else(|| { - ( - workspace_manifest_path.to_owned(), - workspace_manifest.clone(), - ) - })) + // Otherwise use the manifest we just found, as long as it contains `[package]` + None => { + if potential_manifest.package.is_none() { + return Err(Error::NoPackageInManifest(potential_manifest_path)); + } + Ok((potential_manifest_path, potential_manifest)) } } } /// Recursively walk up the directories until finding a `Cargo.toml` -/// -/// When a workspace has been detected, use [`find_package_manifest_in_workspace()`] to find packages -/// instead (that are members of the given workspace). -pub fn find_package_manifest(path: &Path, name: Option<&str>) -> Result<(PathBuf, Manifest)> { +pub fn find_manifest(path: &Path) -> Result<(PathBuf, Manifest)> { let path = canonicalize(path)?; let manifest_path = path .ancestors() @@ -129,28 +100,13 @@ pub fn find_package_manifest(path: &Path, name: Option<&str>) -> Result<(PathBuf let manifest = Manifest::parse_from_toml(&manifest_path)?; - // This function shouldn't be called when a workspace exists. - if manifest.workspace.is_some() { - return Err(Error::UnexpectedWorkspace(manifest_path)); - } - - if let Some(package) = &manifest.package { - if let Some(name) = name { - if package.name == name { - Ok((manifest_path, manifest)) - } else { - Err(Error::PackageNotFound(manifest_path, name.into())) - } - } else { - Ok((manifest_path, manifest)) - } - } else { - Err(Error::NoPackageInManifest(manifest_path)) - } + Ok((manifest_path, manifest)) } -/// Find the first `Cargo.toml` that contains a `[workspace]` +/// Recursively walk up the directories until finding a `Cargo.toml` +/// that contains a `[workspace]` pub fn find_workspace(potential_root: &Path) -> Result> { + let potential_root = canonicalize(potential_root)?; for manifest_path in potential_root .ancestors() .map(|dir| dir.join("Cargo.toml"))