From 3e1366092d588742a91f9f15640fdcf5dca77af0 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Thu, 1 Dec 2022 12:33:35 +0100 Subject: [PATCH] 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 b44ce0a2..ec745809 100644 --- a/xbuild/src/cargo/mod.rs +++ b/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, + package_root: PathBuf, target_dir: PathBuf, offline: bool, } @@ -24,11 +30,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") @@ -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,17 +126,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) @@ -84,14 +144,14 @@ 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) } pub fn build(&self, target: CompileTarget, target_dir: &Path) -> Result { - CargoBuild::new(target, self.root_dir(), target_dir, self.offline) + CargoBuild::new(target, self.package_root(), target_dir, self.offline) } pub fn artifact( diff --git a/xbuild/src/cargo/utils.rs b/xbuild/src/cargo/utils.rs index fbc6e58b..21512f17 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, [`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> { - 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 c6e74e33..8a27c34e 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; @@ -575,17 +574,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, @@ -614,7 +612,7 @@ impl BuildEnv { } pub fn root_dir(&self) -> &Path { - self.cargo.root_dir() + self.cargo.package_root() } pub fn build_dir(&self) -> &Path {