From 7bdc34df968857a5f40d7f6ee19d92ec96db34e2 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Thu, 1 Dec 2022 12:33:35 +0100 Subject: [PATCH 1/6] xbuild/cargo: Linearize workspace detection --- xbuild/src/cargo/config.rs | 14 +++- xbuild/src/cargo/manifest.rs | 10 ++- xbuild/src/cargo/mod.rs | 96 ++++++++++++++++++----- xbuild/src/cargo/utils.rs | 145 ++++++++++++++++++++++++----------- xbuild/src/gradle/mod.rs | 4 +- xbuild/src/lib.rs | 16 ++-- 6 files changed, 206 insertions(+), 79 deletions(-) diff --git a/xbuild/src/cargo/config.rs b/xbuild/src/cargo/config.rs index 8a495678..8ad539f9 100644 --- a/xbuild/src/cargo/config.rs +++ b/xbuild/src/cargo/config.rs @@ -8,10 +8,22 @@ pub struct Config { } impl Config { - pub fn parse_from_toml(path: &Path) -> Result { + pub fn parse_from_toml(path: impl AsRef) -> Result { 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) -> Result> { + 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)] diff --git a/xbuild/src/cargo/manifest.rs b/xbuild/src/cargo/manifest.rs index 700cf81c..588584eb 100644 --- a/xbuild/src/cargo/manifest.rs +++ b/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, pub package: Option, @@ -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, + #[serde(default)] pub members: Vec, } -#[derive(Debug, Deserialize)] +#[derive(Clone, Debug, Deserialize)] pub struct Package { pub name: String, pub version: String, diff --git a/xbuild/src/cargo/mod.rs b/xbuild/src/cargo/mod.rs index 4901a3e3..744ec79c 100644 --- a/xbuild/src/cargo/mod.rs +++ b/xbuild/src/cargo/mod.rs @@ -1,5 +1,5 @@ -use crate::{CompileTarget, Opt}; use anyhow::{Context, Result}; +use std::ffi::OsStr; use std::path::{Path, PathBuf}; use std::process::Command; @@ -10,10 +10,16 @@ mod utils; pub use artifact::{Artifact, CrateType}; +use self::config::Config; +use self::manifest::Manifest; +use crate::{CompileTarget, Opt}; + pub struct Cargo { package: String, features: Vec, - manifest: PathBuf, + _workspace_manifest: Option, + manifest: Manifest, + package_root: PathBuf, target_dir: PathBuf, offline: bool, } @@ -26,11 +32,60 @@ impl Cargo { target_dir: Option, offline: bool, ) -> Result { - 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") @@ -44,18 +99,23 @@ 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(), features, + _workspace_manifest: workspace_manifest.map(|(_path, manifest)| manifest), manifest, + package_root: package_root.to_owned(), target_dir, offline, }) @@ -69,17 +129,17 @@ 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> { 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) @@ -87,7 +147,7 @@ impl Cargo { pub fn bins(&self) -> Result> { 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) @@ -97,7 +157,7 @@ impl Cargo { CargoBuild::new( target, &self.features, - self.root_dir(), + self.package_root(), target_dir, self.offline, ) diff --git a/xbuild/src/cargo/utils.rs b/xbuild/src/cargo/utils.rs index fbc6e58b..b571d16a 100644 --- a/xbuild/src/cargo/utils.rs +++ b/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> { Ok(files) } -fn member(manifest: &Path, members: &[String], package: &str) -> Result> { - 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, use [`find_package_manifest_in_workspace()`] to find packages +/// instead (that are members of the given workspace) when the user specified a package name (with `-p`). +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(|manifest| manifest.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> { - 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> { + 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> { - 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 { - 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 { + 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()); diff --git a/xbuild/src/gradle/mod.rs b/xbuild/src/gradle/mod.rs index 2025fd01..5f4ca155 100644 --- a/xbuild/src/gradle/mod.rs +++ b/xbuild/src/gradle/mod.rs @@ -14,7 +14,7 @@ pub fn prepare(env: &BuildEnv) -> Result<()> { let package = config.manifest.package.as_ref().unwrap(); let wry = env.platform_dir().join("wry"); std::fs::create_dir_all(&wry)?; - if !env.cargo().root_dir().join("kotlin").exists() { + if !env.cargo().package_root().join("kotlin").exists() { let main_activity = format!( r#" package {} @@ -101,7 +101,7 @@ pub fn build(env: &BuildEnv, apk: &Path) -> Result<()> { )?; let srcs = [ - env.cargo().root_dir().join("kotlin"), + env.cargo().package_root().join("kotlin"), env.platform_dir().join("wry"), ]; for src in srcs { diff --git a/xbuild/src/lib.rs b/xbuild/src/lib.rs index f3364811..2a522bbc 100644 --- a/xbuild/src/lib.rs +++ b/xbuild/src/lib.rs @@ -2,7 +2,6 @@ use crate::cargo::{Cargo, CargoBuild, CrateType}; use crate::config::Config; use crate::devices::Device; use anyhow::Result; -use cargo::manifest::Manifest; use clap::Parser; use std::path::{Path, PathBuf}; use xcommon::Signer; @@ -584,17 +583,16 @@ impl BuildEnv { let build_target = args.build_target.build_target()?; let build_dir = cargo.target_dir().join("x"); let cache_dir = dirs::cache_dir().unwrap().join("x"); - let cargo_toml = cargo.manifest(); - let manifest = cargo_toml.parent().unwrap().join("manifest.yaml"); - let cargo_toml = Manifest::parse_from_toml(cargo_toml)?.package.unwrap(); + let cargo_manifest = cargo.manifest(); + let package = cargo_manifest.package.as_ref().unwrap(); // Caller should guarantee that this is a valid package + let manifest = cargo.package_root().join("manifest.yaml"); let mut config = Config::parse(&manifest)?; - config.apply_rust_package(&cargo_toml, build_target.opt()); + config.apply_rust_package(package, build_target.opt()); let icon = config .icon(build_target.platform()) - .map(|icon| cargo.root_dir().join(icon)); - let name = cargo_toml.name; + .map(|icon| cargo.package_root().join(icon)); Ok(Self { - name, + name: package.name.clone(), build_target, icon, cargo, @@ -623,7 +621,7 @@ impl BuildEnv { } pub fn root_dir(&self) -> &Path { - self.cargo.root_dir() + self.cargo.package_root() } pub fn build_dir(&self) -> &Path { From 837802d83730d33e1b0229a3c2fa32d298c3ab87 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Tue, 6 Dec 2022 11:35:59 +0100 Subject: [PATCH 2/6] 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". --- xbuild/src/cargo/utils.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/xbuild/src/cargo/utils.rs b/xbuild/src/cargo/utils.rs index b571d16a..3f6f25d5 100644 --- a/xbuild/src/cargo/utils.rs +++ b/xbuild/src/cargo/utils.rs @@ -19,6 +19,14 @@ pub fn list_rust_files(dir: &Path) -> Result> { Ok(files) } +fn canonicalize(mut path: &Path) -> Result { + if path == Path::new("") { + path = Path::new("."); + } + dunce::canonicalize(path) + .with_context(|| format!("Failed to canonicalize `{}`", path.display())) +} + /// Tries to find a package by the given `name` in the [workspace root] or member /// of the given [workspace] [`Manifest`]. /// @@ -85,7 +93,7 @@ pub fn find_package_manifest_in_workspace( /// When a workspace has been detected, use [`find_package_manifest_in_workspace()`] to find packages /// instead (that are members of the given workspace) when the user specified a package name (with `-p`). pub fn find_package_manifest(path: &Path, name: Option<&str>) -> Result<(PathBuf, Manifest)> { - let path = dunce::canonicalize(path)?; + let path = canonicalize(path)?; let manifest_path = path .ancestors() .map(|dir| dir.join("Cargo.toml")) From 96f07416c45669ad655116609648d36f09f750de Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Tue, 6 Dec 2022 11:41:46 +0100 Subject: [PATCH 3/6] 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. --- xbuild/src/cargo/mod.rs | 6 +++--- xbuild/src/cargo/utils.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/xbuild/src/cargo/mod.rs b/xbuild/src/cargo/mod.rs index 744ec79c..41c7a4a1 100644 --- a/xbuild/src/cargo/mod.rs +++ b/xbuild/src/cargo/mod.rs @@ -45,9 +45,9 @@ impl Cargo { .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().context("Could not retrieve current directory"), + |manifest_path| utils::canonicalize(manifest_path.parent().unwrap()), + )?; // Scan the given and all parent directories for a Cargo.toml containing a workspace let workspace_manifest = utils::find_workspace(&search_path)?; diff --git a/xbuild/src/cargo/utils.rs b/xbuild/src/cargo/utils.rs index 3f6f25d5..f73b7b19 100644 --- a/xbuild/src/cargo/utils.rs +++ b/xbuild/src/cargo/utils.rs @@ -19,7 +19,7 @@ pub fn list_rust_files(dir: &Path) -> Result> { Ok(files) } -fn canonicalize(mut path: &Path) -> Result { +pub fn canonicalize(mut path: &Path) -> Result { if path == Path::new("") { path = Path::new("."); } From 948a76f588377ad75bd9458b08d7e3ee0ac1c289 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Tue, 6 Dec 2022 13:01:08 +0100 Subject: [PATCH 4/6] Use workspace members to select package by `--manifest-path` or `$PWD` Instead of relying on non-workspace logic. --- xbuild/src/cargo/mod.rs | 31 ++++----- xbuild/src/cargo/utils.rs | 131 +++++++++++++++++++++++++++----------- 2 files changed, 106 insertions(+), 56 deletions(-) diff --git a/xbuild/src/cargo/mod.rs b/xbuild/src/cargo/mod.rs index 41c7a4a1..c215e428 100644 --- a/xbuild/src/cargo/mod.rs +++ b/xbuild/src/cargo/mod.rs @@ -52,26 +52,19 @@ impl Cargo { // 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)? + 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)? + }; // The manifest is known to contain a package at this point let package = &manifest.package.as_ref().unwrap().name; diff --git a/xbuild/src/cargo/utils.rs b/xbuild/src/cargo/utils.rs index f73b7b19..5decd1e2 100644 --- a/xbuild/src/cargo/utils.rs +++ b/xbuild/src/cargo/utils.rs @@ -1,6 +1,7 @@ use super::config::Config; use super::manifest::Manifest; use anyhow::{Context, Result}; +use std::collections::HashSet; use std::ffi::OsStr; use std::path::{Path, PathBuf}; @@ -27,6 +28,12 @@ pub fn canonicalize(mut path: &Path) -> Result { .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`]. /// @@ -37,61 +44,111 @@ pub fn canonicalize(mut path: &Path) -> Result { pub fn find_package_manifest_in_workspace( workspace_manifest_path: &Path, workspace_manifest: &Manifest, - name: &str, + selector: PackageSelector<'_>, ) -> 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(); + + match selector { + PackageSelector::ByName(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(), + )); + } + } + + // Check all member packages inside the workspace + 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).with_context(|| { + format!( + "Failed to load manifest for workspace member `{}`", + manifest_path.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(), + ); + + 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(), + ); + } + } + } - // 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(), - )); + Err(anyhow::anyhow!( + "package `{}` not found in workspace `{}`", + workspace_manifest_path.display(), + name, + )) } - } + PackageSelector::ByPath(path) => { + let path = canonicalize(path)?; + let workspace_root = canonicalize(workspace_root)?; - // 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)?; - - // 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)); + // Check all member packages inside the workspace + let mut all_members = HashSet::new(); + + 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).with_context(|| { + format!( + "Failed to load manifest for workspace member `{}`", + manifest_path.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_path); } + } + + // Find the closest member based on the given path + if let Some(manifest_dir) = path.ancestors().find(|&dir| all_members.contains(dir)) { + let manifest_path = manifest_dir.join("Cargo.toml"); + let manifest = Manifest::parse_from_toml(&manifest_path)?; + Ok((manifest_path, manifest)) } else { - anyhow::bail!( - "Failed to parse manifest at `{}`: virtual manifests must be configured with `[workspace]`", - manifest_path.display(), - ); + Ok(( + workspace_manifest_path.to_owned(), + workspace_manifest.clone(), + )) } } } - - Err(anyhow::anyhow!( - "package `{}` not found in workspace `{}`", - workspace_manifest_path.display(), - name, - )) } /// 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) when the user specified a package name (with `-p`). +/// instead (that are members of the given workspace). pub fn find_package_manifest(path: &Path, name: Option<&str>) -> Result<(PathBuf, Manifest)> { let path = canonicalize(path)?; let manifest_path = path From 3618b01c350560c3fa6a92d75c3d3774f4104ca3 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Tue, 6 Dec 2022 13:21:10 +0100 Subject: [PATCH 5/6] Pre-parse all workspace manifests to check for errors --- xbuild/src/cargo/utils.rs | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/xbuild/src/cargo/utils.rs b/xbuild/src/cargo/utils.rs index 5decd1e2..db60146c 100644 --- a/xbuild/src/cargo/utils.rs +++ b/xbuild/src/cargo/utils.rs @@ -1,7 +1,7 @@ use super::config::Config; use super::manifest::Manifest; use anyhow::{Context, Result}; -use std::collections::HashSet; +use std::collections::HashMap; use std::ffi::OsStr; use std::path::{Path, PathBuf}; @@ -107,16 +107,17 @@ pub fn find_package_manifest_in_workspace( let workspace_root = canonicalize(workspace_root)?; // Check all member packages inside the workspace - let mut all_members = HashSet::new(); + 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_path = manifest_dir?.join("Cargo.toml"); + 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_path.display() + manifest_dir.display() ) })?; @@ -126,21 +127,22 @@ pub fn find_package_manifest_in_workspace( "Did not expect a `[workspace]` at `{}`", manifest_path.display(), ); - all_members.insert(manifest_path); + + all_members.insert(manifest_dir, (manifest_path, manifest)); } } // Find the closest member based on the given path - if let Some(manifest_dir) = path.ancestors().find(|&dir| all_members.contains(dir)) { - let manifest_path = manifest_dir.join("Cargo.toml"); - let manifest = Manifest::parse_from_toml(&manifest_path)?; - Ok((manifest_path, manifest)) - } else { - Ok(( - workspace_manifest_path.to_owned(), - workspace_manifest.clone(), - )) - } + 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(), + ) + })) } } } From 60e43e54793668ec800a734f1f9af3f98db56476 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Tue, 6 Dec 2022 13:23:16 +0100 Subject: [PATCH 6/6] Pre-parse all member manifests for `ByName` selection too --- xbuild/src/cargo/utils.rs | 87 +++++++++++++++------------------------ 1 file changed, 34 insertions(+), 53 deletions(-) diff --git a/xbuild/src/cargo/utils.rs b/xbuild/src/cargo/utils.rs index db60146c..72cd29ea 100644 --- a/xbuild/src/cargo/utils.rs +++ b/xbuild/src/cargo/utils.rs @@ -51,6 +51,32 @@ pub fn find_package_manifest_in_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)); + } + } match selector { PackageSelector::ByName(name) => { @@ -65,34 +91,16 @@ pub fn find_package_manifest_in_workspace( } // Check all member packages inside the workspace - 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).with_context(|| { - format!( - "Failed to load manifest for workspace member `{}`", - manifest_path.display() - ) - })?; - - // Workspace members cannot themselves be/contain a new workspace - anyhow::ensure!( - manifest.workspace.is_none(), - "Did not expect a `[workspace]` at `{}`", + 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(), ); - - 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(), - ); - } } } @@ -104,33 +112,6 @@ pub fn find_package_manifest_in_workspace( } PackageSelector::ByPath(path) => { let path = canonicalize(path)?; - 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)); - } - } // Find the closest member based on the given path Ok(path