xbuild/cargo: Linearize workspace detection

xbuild/src/cargo/config.rs

@@ -8,10 +8,22 @@ pub struct Config {
 }
 
 impl Config {
-    pub fn parse_from_toml(path: &Path) -> Result<Self> {
+    pub fn parse_from_toml(path: impl AsRef<Path>) -> Result<Self> {
         let contents = std::fs::read_to_string(path)?;
         Ok(toml::from_str(&contents)?)
     }
+
+    /// Search for and open `.cargo/config.toml` in any parent of the workspace root path.
+    pub fn find_cargo_config_for_workspace(workspace: impl AsRef<Path>) -> Result<Option<Self>> {
+        let workspace = workspace.as_ref();
+        let workspace = dunce::canonicalize(workspace)?;
+        workspace
+            .ancestors()
+            .map(|dir| dir.join(".cargo/config.toml"))
+            .find(|p| p.is_file())
+            .map(Config::parse_from_toml)
+            .transpose()
+    }
 }
 
 #[derive(Debug, Deserialize)]

xbuild/src/cargo/manifest.rs

@@ -2,7 +2,7 @@ use anyhow::Result;
 use serde::Deserialize;
 use std::path::Path;
 
-#[derive(Debug, Deserialize)]
+#[derive(Clone, Debug, Deserialize)]
 pub struct Manifest {
     pub workspace: Option<Workspace>,
     pub package: Option<Package>,
@@ -15,12 +15,16 @@ impl Manifest {
     }
 }
 
-#[derive(Debug, Deserialize)]
+#[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>,
 }
 
-#[derive(Debug, Deserialize)]
+#[derive(Clone, Debug, Deserialize)]
 pub struct Package {
     pub name: String,
     pub version: String,

xbuild/src/cargo/mod.rs

@@ -1,5 +1,5 @@
-use crate::{Arch, CompileTarget, Opt, Platform};
 use anyhow::Result;
+use std::ffi::OsStr;
 use std::path::{Path, PathBuf};
 use std::process::Command;
 
@@ -10,9 +10,15 @@ mod utils;
 
 pub use artifact::{Artifact, CrateType};
 
+use self::config::Config;
+use self::manifest::Manifest;
+use crate::{Arch, CompileTarget, Opt, Platform};
+
 pub struct Cargo {
     package: String,
-    manifest: PathBuf,
+    _workspace_manifest: Option<Manifest>,
+    manifest: Manifest,
+    package_root: PathBuf,
     target_dir: PathBuf,
     offline: bool,
 }
@@ -24,11 +30,60 @@ impl Cargo {
         target_dir: Option<PathBuf>,
         offline: bool,
     ) -> Result<Self> {
-        let (manifest, package) = utils::find_package(
-            &manifest_path.unwrap_or_else(|| std::env::current_dir().unwrap()),
-            package,
-        )?;
-        let root_dir = manifest.parent().unwrap();
+        let manifest_path = manifest_path
+            .map(|path| {
+                if path.file_name() != Some(OsStr::new("Cargo.toml")) || !path.is_file() {
+                    Err(anyhow::anyhow!(
+                        "The manifest-path must be a path to a Cargo.toml file"
+                    ))
+                } else {
+                    Ok(path)
+                }
+            })
+            .transpose()?;
+
+        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
+        let workspace_manifest = utils::find_workspace(&search_path)?;
+
+        let (manifest_path, manifest) =
+            if let (Some(package), Some((workspace_manifest_path, workspace))) =
+                (package, &workspace_manifest)
+            {
+                // If a workspace was found, and the user chose a package with `-p`, find packages relative to it
+                // TODO: What if we call `cargo apk run` in the workspace root, and detect a workspace? It should
+                // then use the `[package]` defined in the workspace (will be found below, though, but currently
+                // fails with UnexpectedWorkspace)
+                utils::find_package_manifest_in_workspace(
+                    workspace_manifest_path,
+                    workspace,
+                    package,
+                )?
+            } else {
+                // Otherwise scan up the directories based on --manifest-path and the working directory.
+                // TODO: When we're in a workspace but the user didn't select a package by name, this
+                // is the right logic to use as long as we _also_ validate that the Cargo.toml we found
+                // was a member of this workspace?
+                utils::find_package_manifest(&search_path, package)?
+            };
+
+        // The manifest is known to contain a package at this point
+        let package = &manifest.package.as_ref().unwrap().name;
+
+        let package_root = 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 = Config::find_cargo_config_for_workspace(package_root)?;
+        // TODO: Import LocalizedConfig code from cargo-subcommand and propagate `[env]`
+        // if let Some(config) = &config {
+        //     config.set_env_vars().unwrap();
+        // }
+
         let target_dir = target_dir
             .or_else(|| {
                 std::env::var_os("CARGO_BUILD_TARGET_DIR")
@@ -42,17 +97,22 @@ impl Cargo {
                     target_dir
                 }
             });
+
         let target_dir = target_dir.unwrap_or_else(|| {
-            utils::find_workspace(&manifest, &package)
-                .unwrap()
-                .unwrap_or_else(|| manifest.clone())
+            workspace_manifest
+                .as_ref()
+                .map(|(path, _)| path)
+                .unwrap_or_else(|| &manifest_path)
                 .parent()
                 .unwrap()
-                .join(utils::get_target_dir_name(root_dir).unwrap())
+                .join(utils::get_target_dir_name(config.as_ref()).unwrap())
         });
+
         Ok(Self {
-            package,
+            package: package.clone(),
+            _workspace_manifest: workspace_manifest.map(|(_path, manifest)| manifest),
             manifest,
+            package_root: package_root.to_owned(),
             target_dir,
             offline,
         })
@@ -66,32 +126,32 @@ impl Cargo {
         &self.package
     }
 
-    pub fn manifest(&self) -> &Path {
+    pub fn manifest(&self) -> &Manifest {
         &self.manifest
     }
 
-    pub fn root_dir(&self) -> &Path {
-        self.manifest.parent().unwrap()
+    pub fn package_root(&self) -> &Path {
+        &self.package_root
     }
 
     pub fn examples(&self) -> Result<Vec<Artifact>> {
         let mut artifacts = vec![];
-        for file in utils::list_rust_files(&self.root_dir().join("examples"))? {
+        for file in utils::list_rust_files(&self.package_root().join("examples"))? {
             artifacts.push(Artifact::Example(file));
         }
         Ok(artifacts)
     }
 
     pub fn bins(&self) -> Result<Vec<Artifact>> {
         let mut artifacts = vec![];
-        for file in utils::list_rust_files(&self.root_dir().join("src").join("bin"))? {
+        for file in utils::list_rust_files(&self.package_root().join("src").join("bin"))? {
             artifacts.push(Artifact::Root(file));
         }
         Ok(artifacts)
     }
 
     pub fn build(&self, target: CompileTarget, target_dir: &Path) -> Result<CargoBuild> {
-        CargoBuild::new(target, self.root_dir(), target_dir, self.offline)
+        CargoBuild::new(target, self.package_root(), target_dir, self.offline)
     }
 
     pub fn artifact(

xbuild/src/cargo/utils.rs

@@ -1,6 +1,6 @@
 use super::config::Config;
 use super::manifest::Manifest;
-use anyhow::Result;
+use anyhow::{Context, Result};
 use std::ffi::OsStr;
 use std::path::{Path, PathBuf};
 
@@ -19,77 +19,130 @@ 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())? {
+/// 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.
+///
+/// [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: &str,
+) -> Result<(PathBuf, Manifest)> {
+    let workspace = workspace_manifest
+        .workspace
+        .as_ref()
+        .context("The provided Cargo.toml does not contain a `[workspace]`")?;
+
+    // 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(),
+            ));
+        }
+    }
+
+    // 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
+            anyhow::ensure!(
+                manifest.workspace.is_none(),
+                "Did not expect a `[workspace]` at `{}`",
+                manifest_path.display(),
+            );
+
+            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(),
+                );
             }
         }
     }
-    Ok(None)
+
+    Err(anyhow::anyhow!(
+        "package `{}` not found in workspace `{}`",
+        workspace_manifest_path.display(),
+        name,
+    ))
 }
 
-pub fn find_package(path: &Path, name: Option<&str>) -> Result<(PathBuf, String)> {
+/// 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)?;
-    for manifest_path in path
+    let manifest_path = path
         .ancestors()
         .map(|dir| dir.join("Cargo.toml"))
-        .filter(|dir| dir.exists())
-    {
-        let manifest = Manifest::parse_from_toml(&manifest_path)?;
-        if let Some(p) = manifest.package.as_ref() {
-            if let (Some(n1), n2) = (name, &p.name) {
-                if n1 == n2 {
-                    return Ok((manifest_path, p.name.clone()));
-                }
+        .find(|dir| dir.exists())
+        .context("Didn't find Cargo.toml.")?;
+
+    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 {
-                return Ok((manifest_path, p.name.clone()));
-            }
-        }
-        if let (Some(w), Some(name)) = (manifest.workspace.as_ref(), name) {
-            if let Some(manifest_path) = member(&manifest_path, &w.members, name)? {
-                return Ok((manifest_path, name.to_string()));
+                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(),
+        ))
     }
-    anyhow::bail!("cargo manifest not found");
 }
 
-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())
+        .filter(|manifest| manifest.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)
 }
 
-/// Search for .cargo/config.toml file relative to the workspace root path.
-pub fn find_cargo_config(path: &Path) -> Result<Option<PathBuf>> {
-    let path = dunce::canonicalize(path)?;
-    Ok(path
-        .ancestors()
-        .map(|dir| dir.join(".cargo/config.toml"))
-        .find(|dir| dir.is_file()))
-}
-
-pub fn get_target_dir_name(path: &Path) -> Result<String> {
-    if let Some(config_path) = find_cargo_config(path)? {
-        let config = Config::parse_from_toml(&config_path)?;
+/// Returns the [`target-dir`] configured in `.cargo/config.toml` or `"target"` if not set.
+///
+/// [`target-dir`]: https://doc.rust-lang.org/cargo/reference/config.html#buildtarget-dir
+pub fn get_target_dir_name(config: Option<&Config>) -> Result<String> {
+    if let Some(config) = config {
         if let Some(build) = config.build.as_ref() {
             if let Some(target_dir) = &build.target_dir {
                 return Ok(target_dir.clone());