Linearize workspace detection (#23)

When `cargo` scans for a workspace it simply walks up the directories
until finding the first `Cargo.toml` with a `[workspace]` declaration.
All crates are detected relative to this, and no special skipping
behaviour (walking further up the directories) occurs when a package was
not found (was previously happening in `find_package()+member()`).

Simplify the original code somewhat by making this feat more obvious,
rejecting more invalid configurations.

Author: MarijnS95

src/error.rs

@@ -7,12 +7,14 @@ use toml::de::Error as TomlError;
 #[derive(Debug)]
 pub enum Error {
     InvalidArgs,
+    ManifestNotAWorkspace,
     ManifestNotFound,
     RustcNotFound,
     ManifestPathNotFound,
     MultiplePackagesNotSupported,
     GlobPatternError(&'static str),
     Glob(GlobError),
+    UnexpectedWorkspace(PathBuf),
     Io(PathBuf, IoError),
     Toml(PathBuf, TomlError),
 }
@@ -23,6 +25,7 @@ impl Display for Error {
     fn fmt(&self, f: &mut Formatter) -> FmtResult {
         f.write_str(match self {
             Self::InvalidArgs => "Invalid args.",
+            Self::ManifestNotAWorkspace => "The provided Cargo.toml does not contain a `[workspace]`",
             Self::ManifestNotFound => "Didn't find Cargo.toml.",
             Self::ManifestPathNotFound => "The manifest-path must be a path to a Cargo.toml file",
             Self::MultiplePackagesNotSupported => {
@@ -31,6 +34,7 @@ impl Display for Error {
             Self::RustcNotFound => "Didn't find rustc.",
             Self::GlobPatternError(error) => error,
             Self::Glob(error) => return error.fmt(f),
+            Self::UnexpectedWorkspace(path) => return write!(f, "Did not expect a `[workspace]` at {}", path.display()),
             Self::Io(path, error) => return write!(f, "{}: {}", path.display(), error),
             Self::Toml(file, error) => return write!(f, "{}: {}", file.display(), error),
         })

src/subcommand.rs

@@ -25,6 +25,7 @@ impl Subcommand {
             args.package.len() < 2,
             "Multiple packages are not supported yet by `cargo-subcommand`"
         );
+        let package = args.package.get(0).map(|s| s.as_str());
         assert!(
             !args.workspace,
             "`--workspace` is not supported yet by `cargo-subcommand`"
@@ -46,15 +47,30 @@ impl Subcommand {
             })
             .transpose()?;
 
-        let (manifest_path, package) = utils::find_package(
-            &manifest_path.unwrap_or_else(|| std::env::current_dir().unwrap()),
-            args.package.get(0).map(|s| s.as_str()),
-        )?;
+        let search_path = manifest_path.map_or_else(
+            || std::env::current_dir().unwrap(),
+            |manifest_path| manifest_path.parent().unwrap().to_owned(),
+        );
+
+        // Scan the given and all parent directories for a Cargo.toml containing a workspace
+        // TODO: If set, validate that the found workspace is either the given `--manifest-path`,
+        // or contains the given `--manifest-path` as member.
+        let workspace_manifest = utils::find_workspace(&search_path)?;
+
+        let (manifest_path, package) =
+            if let Some((workspace_manifest_path, workspace)) = &workspace_manifest {
+                // If a workspace was found, find packages relative to it
+                utils::find_package_in_workspace(workspace_manifest_path, workspace, package)?
+            } else {
+                // Otherwise scan up the directories
+                utils::find_package(&search_path, package)?
+            };
+
         let root_dir = manifest_path.parent().unwrap();
 
         // TODO: Find, parse, and merge _all_ config files following the hierarchical structure:
         // https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure
-        let config = LocalizedConfig::find_cargo_config_for_workspace(&root_dir)?;
+        let config = LocalizedConfig::find_cargo_config_for_workspace(root_dir)?;
         if let Some(config) = &config {
             config.set_env_vars().unwrap();
         }
@@ -76,9 +92,10 @@ impl Subcommand {
             });
 
         let target_dir = target_dir.unwrap_or_else(|| {
-            utils::find_workspace(&manifest_path, &package)
-                .unwrap()
-                .unwrap_or_else(|| manifest_path.clone())
+            workspace_manifest
+                .as_ref()
+                .map(|(path, _)| path)
+                .unwrap_or_else(|| &manifest_path)
                 .parent()
                 .unwrap()
                 .join(utils::get_target_dir_name(config.as_deref()).unwrap())

src/utils.rs

@@ -19,22 +19,73 @@ pub fn list_rust_files(dir: &Path) -> Result<Vec<String>> {
     Ok(files)
 }
 
-fn member(manifest: &Path, members: &[String], package: &str) -> Result<Option<PathBuf>> {
-    let workspace_dir = manifest.parent().unwrap();
-    for member in members {
-        for manifest_dir in glob::glob(workspace_dir.join(member).to_str().unwrap())? {
+fn match_package_name(manifest: &Manifest, name: Option<&str>) -> Option<String> {
+    if let Some(p) = &manifest.package {
+        if let Some(name) = name {
+            if name == p.name {
+                return Some(p.name.clone());
+            }
+        } else {
+            return Some(p.name.clone());
+        }
+    }
+
+    None
+}
+
+/// 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()`] instead.
+///
+/// [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_in_workspace(
+    workspace_manifest_path: &Path,
+    workspace_manifest: &Manifest,
+    name: Option<&str>,
+) -> Result<(PathBuf, String)> {
+    let workspace = workspace_manifest
+        .workspace
+        .as_ref()
+        .ok_or(Error::ManifestNotAWorkspace)?;
+
+    // When building inside a workspace, require a package to be selected on the cmdline
+    // as selecting the right package is non-trivial and selecting multiple packages
+    // isn't currently supported yet.  See the selection mechanism:
+    // https://doc.rust-lang.org/cargo/reference/workspaces.html#package-selection
+    let name = name.ok_or(Error::MultiplePackagesNotSupported)?;
+
+    // Check if the workspace manifest also contains a [package]
+    if let Some(package) = match_package_name(workspace_manifest, Some(name)) {
+        return Ok((workspace_manifest_path.to_owned(), package));
+    }
+
+    // Check all member packages inside the workspace
+    let workspace_root = workspace_manifest_path.parent().unwrap();
+    for member in &workspace.members {
+        for manifest_dir in glob::glob(workspace_root.join(member).to_str().unwrap())? {
             let manifest_path = manifest_dir?.join("Cargo.toml");
             let manifest = Manifest::parse_from_toml(&manifest_path)?;
-            if let Some(p) = manifest.package.as_ref() {
-                if p.name == package {
-                    return Ok(Some(manifest_path));
-                }
+
+            // Workspace members cannot themselves be/contain a new workspace
+            if manifest.workspace.is_some() {
+                return Err(Error::UnexpectedWorkspace(manifest_path));
+            }
+
+            if let Some(package) = match_package_name(&manifest, Some(name)) {
+                return Ok((manifest_path, package));
             }
         }
     }
-    Ok(None)
+
+    Err(Error::ManifestNotFound)
 }
 
+/// Recursively walk up the directories until finding a `Cargo.toml`
+///
+/// When a workspace has been detected, [`find_package_in_workspace()`] to find packages
+/// instead (that are members of the given workspace).
 pub fn find_package(path: &Path, name: Option<&str>) -> Result<(PathBuf, String)> {
     let path = dunce::canonicalize(path).map_err(|e| Error::Io(path.to_owned(), e))?;
     for manifest_path in path
@@ -43,38 +94,29 @@ pub fn find_package(path: &Path, name: Option<&str>) -> Result<(PathBuf, String)
         .filter(|dir| dir.exists())
     {
         let manifest = Manifest::parse_from_toml(&manifest_path)?;
-        if let Some(p) = manifest.package.as_ref() {
-            if let Some(name) = name {
-                if name == p.name {
-                    return Ok((manifest_path, p.name.clone()));
-                }
-            } else {
-                return Ok((manifest_path, p.name.clone()));
-            }
+
+        // This function shouldn't be called when a workspace exists.
+        if manifest.workspace.is_some() {
+            return Err(Error::UnexpectedWorkspace(manifest_path));
         }
-        if let Some(w) = manifest.workspace.as_ref() {
-            // TODO: This should also work if name is None - then all packages should simply be returned
-            let name = name.ok_or(Error::MultiplePackagesNotSupported)?;
-            if let Some(manifest_path) = member(&manifest_path, &w.members, name)? {
-                return Ok((manifest_path, name.to_string()));
-            }
+
+        if let Some(package) = match_package_name(&manifest, name) {
+            return Ok((manifest_path, package));
         }
     }
     Err(Error::ManifestNotFound)
 }
 
-pub fn find_workspace(manifest: &Path, name: &str) -> Result<Option<PathBuf>> {
-    let dir = manifest.parent().unwrap();
-    for manifest_path in dir
+/// Find the first `Cargo.toml` that contains a `[workspace]`
+pub fn find_workspace(potential_root: &Path) -> Result<Option<(PathBuf, Manifest)>> {
+    for manifest_path in potential_root
         .ancestors()
         .map(|dir| dir.join("Cargo.toml"))
         .filter(|dir| dir.exists())
     {
         let manifest = Manifest::parse_from_toml(&manifest_path)?;
-        if let Some(w) = manifest.workspace.as_ref() {
-            if member(&manifest_path, &w.members, name)?.is_some() {
-                return Ok(Some(manifest_path));
-            }
+        if manifest.workspace.is_some() {
+            return Ok(Some((manifest_path, manifest)));
         }
     }
     Ok(None)