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.

README.md

@@ -19,9 +19,9 @@ imd:55abbd4b70af4353bdea2595bbddcac4a2b7891a      David’s iPhone      ios arm6
 Build or run on a device:
 ```sh
 x build --device adb:16ee50bc
-[1/3] Fetch precompiled artefacts
+[1/3] Fetch precompiled artifacts
 info: component 'rust-std' for target 'aarch64-linux-android' is up to date
-[1/3] Fetch precompiled artefacts [72ms]
+[1/3] Fetch precompiled artifacts [72ms]
 [2/3] Build rust
     Finished dev [unoptimized + debuginfo] target(s) in 0.11s
 [2/3] Build rust [143ms]

xbuild/src/cargo/manifest.rs

@@ -1,6 +1,11 @@
-use anyhow::Result;
+use anyhow::{Context, Result};
 use serde::Deserialize;
-use std::path::Path;
+use std::{
+    collections::HashMap,
+    path::{Path, PathBuf},
+};
+
+use super::utils;
 
 #[derive(Debug, Clone, Deserialize)]
 #[serde(untagged)]
@@ -20,6 +25,85 @@ impl Manifest {
         let contents = std::fs::read_to_string(path)?;
         Ok(toml::from_str(&contents)?)
     }
+
+    /// 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()
+            .context("The provided Cargo.toml does not contain a `[workspace]`")?;
+        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).with_context(|| {
+                    format!(
+                        "Failed to load manifest for workspace member `{}`",
+                        manifest_dir.display()
+                    )
+                })?;
+
+                // Workspace members cannot themselves be/contain a new workspace
+                anyhow::ensure!(
+                    manifest.workspace.is_none(),
+                    "Did not expect a `[workspace]` at `{}`",
+                    manifest_path.display(),
+                );
+
+                // And because they cannot contain a [workspace], they may not be a virtual manifest
+                // and must hence contain [package]
+                anyhow::ensure!(
+                    manifest.package.is_some(),
+                    "Failed to parse manifest at `{}`: virtual manifests must be configured with `[workspace]`",
+                    manifest_path.display(),
+                );
+
+                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)> {
+        anyhow::ensure!(
+            self.workspace.is_none(),
+            "Did not expect a `[workspace]` at `{}`",
+            manifest_path.display(),
+        );
+
+        if let Some(package) = &self.package {
+            if let Some(name) = name {
+                if package.name == name {
+                    Ok((manifest_path, self))
+                } else {
+                    Err(anyhow::anyhow!(
+                        "package `{}` not found in workspace `{}`",
+                        manifest_path.display(),
+                        name,
+                    ))
+                }
+            } else {
+                Ok((manifest_path, self))
+            }
+        } else {
+            Err(anyhow::anyhow!(
+                "Failed to parse manifest at `{}`: virtual manifests must be configured with `[workspace]`",
+                manifest_path.display(),
+            ))
+        }
+    }
 }
 
 #[derive(Clone, Debug, Deserialize)]

xbuild/src/cargo/mod.rs

@@ -49,21 +49,22 @@ impl Cargo {
             |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

xbuild/src/cargo/utils.rs

@@ -1,7 +1,6 @@
 use super::config::Config;
 use super::manifest::Manifest;
 use anyhow::{Context, Result};
-use std::collections::HashMap;
 use std::ffi::OsStr;
 use std::path::{Path, PathBuf};
 
@@ -28,12 +27,6 @@ pub fn canonicalize(mut path: &Path) -> Result<PathBuf> {
         .with_context(|| format!("Failed to canonicalize `{}`", path.display()))
 }
 
-#[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`].
 ///
@@ -42,44 +35,38 @@ pub enum PackageSelector<'a> {
 /// [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()
-        .context("The provided Cargo.toml does not contain a `[workspace]`")?;
-    let workspace_root = workspace_manifest_path.parent().unwrap();
-    let workspace_root = 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).with_context(|| {
-                format!(
-                    "Failed to load manifest for workspace member `{}`",
-                    manifest_dir.display()
-                )
-            })?;
-
-            // Workspace members cannot themselves be/contain a new workspace
-            anyhow::ensure!(
-                manifest.workspace.is_none(),
-                "Did not expect a `[workspace]` at `{}`",
-                manifest_path.display(),
-            );
-
-            all_members.insert(manifest_dir, (manifest_path, manifest));
-        }
+    let potential_manifest_dir = potential_manifest_path.parent().unwrap();
+    let workspace_manifest_dir = workspace_manifest_path.parent().unwrap();
+    let package_subpath = potential_manifest_dir
+        .strip_prefix(workspace_manifest_dir)
+        .unwrap();
+
+    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)
+    {
+        anyhow::bail!(
+                    "current package believes it's in a workspace when it's not:
+current:   {potential_manifest_path}
+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"
+                    potential_manifest_path = potential_manifest_path.display(),
+                    workspace_manifest_path = workspace_manifest_path.display(),
+                    package_subpath = package_subpath.display(),
+                );
     }
 
-    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 {
@@ -91,48 +78,34 @@ 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 {
-                    anyhow::bail!(
-                        "Failed to parse manifest at `{}`: virtual manifests must be configured with `[workspace]`",
-                        manifest_path.display(),
-                    );
+            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(anyhow::anyhow!(
+            anyhow::bail!(
                 "package `{}` not found in workspace `{}`",
-                workspace_manifest_path.display(),
                 name,
-            ))
+                workspace_manifest_path.display(),
+            )
         }
-        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 => {
+            anyhow::ensure!(
+                potential_manifest.package.is_some(),
+                "Failed to parse manifest at `{}`: virtual manifests must be configured with `[workspace]`",
+                potential_manifest_path.display(),
+            );
+            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()
@@ -142,37 +115,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.
-    anyhow::ensure!(
-        manifest.workspace.is_none(),
-        "Did not expect a `[workspace]` at `{}`",
-        manifest_path.display(),
-    );
-
-    if let Some(package) = &manifest.package {
-        if let Some(name) = name {
-            if package.name == name {
-                Ok((manifest_path, manifest))
-            } else {
-                Err(anyhow::anyhow!(
-                    "package `{}` not found in workspace `{}`",
-                    manifest_path.display(),
-                    name,
-                ))
-            }
-        } else {
-            Ok((manifest_path, manifest))
-        }
-    } else {
-        Err(anyhow::anyhow!(
-            "Failed to parse manifest at `{}`: virtual manifests must be configured with `[workspace]`",
-            manifest_path.display(),
-        ))
-    }
+    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"))

xbuild/src/command/build.rs

@@ -16,14 +16,14 @@ pub fn build(env: &BuildEnv) -> Result<()> {
 
     let mut runner = TaskRunner::new(3, env.verbose());
 
-    runner.start_task("Fetch precompiled artefacts");
+    runner.start_task("Fetch precompiled artifacts");
     let manager = DownloadManager::new(env)?;
     if !env.offline() {
         manager.prefetch()?;
         runner.end_verbose_task();
     }
 
-    runner.start_task("Build rust");
+    runner.start_task(format!("Build rust `{}`", env.name));
     let bin_target = env.target().platform() != Platform::Android;
     let has_lib = env.root_dir().join("src").join("lib.rs").exists();
     if bin_target || has_lib {