Use $PWD and --manifest-path to select a default package in workspaces (#25)

* Use `$PWD` and `--manifest-path` to select a default package in workspaces

This restores original, prevalent `--manifest-path` usage to select a
package containing the APK crate in a `[workspace]`, instead of using
`-p` for that.

* utils: Support path canonicalization for empty strings

When calling `.parent()` on a filename the result is `""`, which should
be treated as PWD (`"."`) but `dunce::canonicalize()` ends up in a "No
such file or directory".

* Canonicalize `--manifest-path` so that a workspace is always found

`.ancestors()` only calls `.parent()` on a `Path` which walks up until
the string empty, but this isn't the root of the filesystem if the path
wasn't absolute.

* Use workspace members to select package by `--manifest-path` or `$PWD`

Instead of relying on non-workspace logic.

* Pre-parse all workspace manifests to check for errors

* Pre-parse all member manifests for `ByName` selection too

* Reject subdirectory manifests that are not members of the workspace

The previous implementation had one fatal flaw: having a non-workspace
`Cargo.toml` in a subdirectory, with a `[workspace]` defined some
directories higher is invalid when that `[workspace]` doesn't include
the given `Cargo.toml` subdirectory/package.  `cargo` rejects this with
a `current package believes it's in a workspace when it's not`, and we
should do the same instead of having an `unwrap_or_else` that falls back
to using the workspace root `Cargo.toml` as a package (especially if it
might not contain a `[package]` at all).

This would for example fail when running `x new template` in the repo
directory, `cd`'ing into it and invoking `x build`.  Instead of
complaining about `template/Cargo.toml` not being part of the workspace,
it detects the workspace and falls back to building the root package
because the `template` directory appears to just be a subdirectory of
the root without defining a subpackage along it.  Since our root doesn't
define a `[package]`, the supposed-to-be-infallible `unwrap()` on
`manifest.package` below fails.

Author: MarijnS95

src/error.rs

@@ -11,10 +11,15 @@ pub enum Error {
     ManifestNotFound,
     RustcNotFound,
     ManifestPathNotFound,
-    MultiplePackagesNotSupported,
     GlobPatternError(&'static str),
     Glob(GlobError),
     UnexpectedWorkspace(PathBuf),
+    NoPackageInManifest(PathBuf),
+    PackageNotFound(PathBuf, String),
+    ManifestNotInWorkspace {
+        manifest: PathBuf,
+        workspace_manifest: PathBuf,
+    },
     Io(PathBuf, IoError),
     Toml(PathBuf, TomlError),
 }
@@ -25,16 +30,48 @@ 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::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 => {
-                "Multiple packages are not yet supported (ie. in a `[workspace]`). Use `-p` to select a specific package."
-            }
             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::UnexpectedWorkspace(path) => {
+                return write!(f, "Did not expect a `[workspace]` at `{}`", path.display())
+            }
+            Self::NoPackageInManifest(manifest) => {
+                return write!(
+                    f,
+                    "Failed to parse manifest at `{}`: virtual manifests must be configured with `[workspace]`",
+                    manifest.display()
+                )
+            }
+            Self::PackageNotFound(workspace, name) => {
+                return write!(
+                    f,
+                    "package `{}` not found in workspace `{}`",
+                    name,
+                    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),
         })

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,10 +18,75 @@ 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<HashMap<PathBuf, (PathBuf, Manifest)>> {
+        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)]
+#[serde(rename_all = "kebab-case")]
 pub struct Workspace {
+    #[serde(default)]
+    pub default_members: Vec<String>,
+    #[serde(default)]
     pub members: Vec<String>,
 }
 

src/subcommand.rs

@@ -49,23 +49,26 @@ impl Subcommand {
             .transpose()?;
 
         let search_path = manifest_path.map_or_else(
-            || std::env::current_dir().unwrap(),
-            |manifest_path| manifest_path.parent().unwrap().to_owned(),
-        );
+            || std::env::current_dir().map_err(|e| Error::Io(PathBuf::new(), e)),
+            |manifest_path| utils::canonicalize(manifest_path.parent().unwrap()),
+        )?;
 
-        // 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.
+        // 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
-            utils::find_package_manifest_in_workspace(workspace_manifest_path, workspace, package)?
-        } else {
-            // Otherwise scan up the directories
-            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

src/utils.rs

@@ -19,103 +19,98 @@ pub fn list_rust_files(dir: &Path) -> Result<Vec<String>> {
     Ok(files)
 }
 
-fn match_package_name(manifest: &Manifest, name: Option<&str>) -> bool {
-    if let Some(p) = &manifest.package {
-        if let Some(name) = name {
-            if name == p.name {
-                return true;
-            }
-        } else {
-            return true;
-        }
+pub fn canonicalize(mut path: &Path) -> Result<PathBuf> {
+    if path == Path::new("") {
+        path = Path::new(".");
     }
-
-    false
+    dunce::canonicalize(path).map_err(|e| Error::Io(path.to_owned(), e))
 }
 
 /// 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,
-    name: Option<&str>,
+    (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)?;
-
-    // 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)?;
+    let potential_manifest_dir = potential_manifest_path.parent().unwrap();
+    let workspace_manifest_dir = workspace_manifest_path.parent().unwrap();
 
-    // Check if the workspace manifest also contains a [package]
-    if match_package_name(workspace_manifest, Some(name)) {
-        return Ok((
-            workspace_manifest_path.to_owned(),
-            workspace_manifest.clone(),
-        ));
+    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(),
+        });
     }
 
-    // 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)?;
+    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 {
+                    return Ok((
+                        workspace_manifest_path.to_owned(),
+                        workspace_manifest.clone(),
+                    ));
+                }
+            }
 
-            // Workspace members cannot themselves be/contain a new workspace
-            if manifest.workspace.is_some() {
-                return Err(Error::UnexpectedWorkspace(manifest_path));
+            // Check all member packages inside the workspace
+            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));
+                }
             }
 
-            if match_package_name(&manifest, Some(name)) {
-                return Ok((manifest_path, manifest));
+            Err(Error::PackageNotFound(
+                workspace_manifest_path.clone(),
+                name.to_owned(),
+            ))
+        }
+        // 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))
         }
     }
-
-    Err(Error::ManifestNotFound)
 }
 
 /// Recursively walk up the directories until finding a `Cargo.toml`
-///
-/// When a workspace has been detected, [`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)> {
-    let path = dunce::canonicalize(path).map_err(|e| Error::Io(path.to_owned(), e))?;
-    for manifest_path in path
+pub fn find_manifest(path: &Path) -> Result<(PathBuf, Manifest)> {
+    let path = canonicalize(path)?;
+    let manifest_path = path
         .ancestors()
         .map(|dir| dir.join("Cargo.toml"))
-        .filter(|dir| dir.exists())
-    {
-        let manifest = Manifest::parse_from_toml(&manifest_path)?;
+        .find(|manifest| manifest.exists())
+        .ok_or(Error::ManifestNotFound)?;
 
-        // This function shouldn't be called when a workspace exists.
-        if manifest.workspace.is_some() {
-            return Err(Error::UnexpectedWorkspace(manifest_path));
-        }
+    let manifest = Manifest::parse_from_toml(&manifest_path)?;
 
-        if match_package_name(&manifest, name) {
-            return Ok((manifest_path, manifest));
-        }
-    }
-    Err(Error::ManifestNotFound)
+    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<Option<(PathBuf, Manifest)>> {
+    let potential_root = canonicalize(potential_root)?;
     for manifest_path in potential_root
         .ancestors()
         .map(|dir| dir.join("Cargo.toml"))
-        .filter(|dir| dir.exists())
+        .filter(|manifest| manifest.exists())
     {
         let manifest = Manifest::parse_from_toml(&manifest_path)?;
         if manifest.workspace.is_some() {