Skip to content

Commit

Permalink
Reject subdirectory manifests that are not members of the workspace
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
MarijnS95 committed Dec 8, 2022
1 parent 67c888d commit f07e2bb
Show file tree
Hide file tree
Showing 4 changed files with 137 additions and 94 deletions.
20 changes: 20 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ pub enum Error {
UnexpectedWorkspace(PathBuf),
NoPackageInManifest(PathBuf),
PackageNotFound(PathBuf, String),
ManifestNotInWorkspace {
manifest: PathBuf,
workspace_manifest: PathBuf,
},
Io(PathBuf, IoError),
Toml(PathBuf, TomlError),
}
Expand Down Expand Up @@ -52,6 +56,22 @@ impl Display for Error {
workspace.display()
)
}
Self::ManifestNotInWorkspace {
manifest,
workspace_manifest,
} => {
return write!(f, "current package believes it's in a workspace when it's not:
current: {}
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"
manifest.display(),
package_subpath = manifest.parent().unwrap().strip_prefix(workspace_manifest.parent().unwrap()).unwrap().display(),
workspace_manifest_path = workspace_manifest.display(),
)
},
Self::Io(path, error) => return write!(f, "{}: {}", path.display(), error),
Self::Toml(file, error) => return write!(f, "{}: {}", file.display(), error),
})
Expand Down
70 changes: 68 additions & 2 deletions src/manifest.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
use crate::error::{Error, Result};
use serde::Deserialize;
use std::path::Path;
use std::{
collections::HashMap,
path::{Path, PathBuf},
};

use crate::error::{Error, Result};
use crate::utils;

#[derive(Clone, Debug, Deserialize)]
pub struct Manifest {
Expand All @@ -13,6 +18,67 @@ impl Manifest {
let contents = std::fs::read_to_string(path).map_err(|e| Error::Io(path.to_owned(), e))?;
toml::from_str(&contents).map_err(|e| Error::Toml(path.to_owned(), e))
}

/// 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()
.ok_or(Error::ManifestNotAWorkspace)?;
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)?;

// Workspace members cannot themselves be/contain a new workspace
if manifest.workspace.is_some() {
return Err(Error::UnexpectedWorkspace(manifest_path));
}

// And because they cannot contain a [workspace], they may not be a virtual manifest
// and must hence contain [package]
if manifest.package.is_none() {
return Err(Error::NoPackageInManifest(manifest_path));
}

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)> {
if self.workspace.is_some() {
return Err(Error::UnexpectedWorkspace(manifest_path));
}

if let Some(package) = &self.package {
if let Some(name) = name {
if package.name == name {
Ok((manifest_path, self))
} else {
Err(Error::PackageNotFound(manifest_path, name.into()))
}
} else {
Ok((manifest_path, self))
}
} else {
Err(Error::NoPackageInManifest(manifest_path))
}
}
}

#[derive(Clone, Debug, Deserialize)]
Expand Down
27 changes: 14 additions & 13 deletions src/subcommand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,21 +53,22 @@ impl Subcommand {
|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
Expand Down
114 changes: 35 additions & 79 deletions src/utils.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use crate::config::Config;
use crate::error::{Error, Result};
use crate::manifest::Manifest;
use std::collections::HashMap;
use std::ffi::OsStr;
use std::path::{Path, PathBuf};

Expand All @@ -27,12 +26,6 @@ pub fn canonicalize(mut path: &Path) -> Result<PathBuf> {
dunce::canonicalize(path).map_err(|e| Error::Io(path.to_owned(), e))
}

#[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`].
///
Expand All @@ -41,36 +34,27 @@ 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()
.ok_or(Error::ManifestNotAWorkspace)?;
let workspace_root = canonicalize(workspace_manifest_path.parent().unwrap())?;

// 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)?;

// Workspace members cannot themselves be/contain a new workspace
if manifest.workspace.is_some() {
return Err(Error::UnexpectedWorkspace(manifest_path));
}
let potential_manifest_dir = potential_manifest_path.parent().unwrap();
let workspace_manifest_dir = workspace_manifest_path.parent().unwrap();

all_members.insert(manifest_dir, (manifest_path, manifest));
}
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)
{
return Err(Error::ManifestNotInWorkspace {
manifest: potential_manifest_path,
workspace_manifest: workspace_manifest_path.clone(),
});
}

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 {
Expand All @@ -82,44 +66,31 @@ 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 {
return Err(Error::NoPackageInManifest(manifest_path));
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(Error::PackageNotFound(
workspace_manifest_path.into(),
name.into(),
workspace_manifest_path.clone(),
name.to_owned(),
))
}
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 => {
if potential_manifest.package.is_none() {
return Err(Error::NoPackageInManifest(potential_manifest_path));
}
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()
Expand All @@ -129,28 +100,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.
if manifest.workspace.is_some() {
return Err(Error::UnexpectedWorkspace(manifest_path));
}

if let Some(package) = &manifest.package {
if let Some(name) = name {
if package.name == name {
Ok((manifest_path, manifest))
} else {
Err(Error::PackageNotFound(manifest_path, name.into()))
}
} else {
Ok((manifest_path, manifest))
}
} else {
Err(Error::NoPackageInManifest(manifest_path))
}
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"))
Expand Down

0 comments on commit f07e2bb

Please sign in to comment.