From c817b5a6afd6b4bef9de5f4acfc7db41d8c9e5cd Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Wed, 7 Dec 2022 22:36:37 +0100 Subject: [PATCH] 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 | 4 +- xbuild/src/cargo/manifest.rs | 88 +++++++++++++++++++- xbuild/src/cargo/mod.rs | 27 +++--- xbuild/src/cargo/utils.rs | 157 ++++++++++++----------------------- xbuild/src/command/build.rs | 4 +- 5 files changed, 157 insertions(+), 123 deletions(-) diff --git a/README.md b/README.md index efc0dc4c..ac5696e3 100644 --- a/README.md +++ b/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] diff --git a/xbuild/src/cargo/manifest.rs b/xbuild/src/cargo/manifest.rs index bece8227..b6415d38 100644 --- a/xbuild/src/cargo/manifest.rs +++ b/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> { + 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)] diff --git a/xbuild/src/cargo/mod.rs b/xbuild/src/cargo/mod.rs index 79e93570..d90c0158 100644 --- a/xbuild/src/cargo/mod.rs +++ b/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 diff --git a/xbuild/src/cargo/utils.rs b/xbuild/src/cargo/utils.rs index 72cd29ea..07a2b6ba 100644 --- a/xbuild/src/cargo/utils.rs +++ b/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,58 +27,46 @@ 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`]. -/// -/// 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, - 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> { + let potential_root = canonicalize(potential_root)?; for manifest_path in potential_root .ancestors() .map(|dir| dir.join("Cargo.toml")) diff --git a/xbuild/src/command/build.rs b/xbuild/src/command/build.rs index aa3ded09..d0ca39bd 100644 --- a/xbuild/src/command/build.rs +++ b/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 {