From 68b8a0987067d9ef41f54796c0f35c0581d41c51 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Fri, 25 Nov 2022 17:20:50 +0100 Subject: [PATCH] Use `$PWD` and `--manifest-path` to select a default package in workspaces This restores original, prevalent `--manifest-path` usage to select a package containing the APK crate in a `[workspace]`, instead of using `-p` for that. --- src/error.rs | 29 +++++++++++++---- src/manifest.rs | 4 +++ src/subcommand.rs | 31 +++++++++++------- src/utils.rs | 80 +++++++++++++++++++++++------------------------ 4 files changed, 86 insertions(+), 58 deletions(-) diff --git a/src/error.rs b/src/error.rs index 4bab10c..b19f731 100644 --- a/src/error.rs +++ b/src/error.rs @@ -11,10 +11,11 @@ pub enum Error { ManifestNotFound, RustcNotFound, ManifestPathNotFound, - MultiplePackagesNotSupported, GlobPatternError(&'static str), Glob(GlobError), UnexpectedWorkspace(PathBuf), + NoPackageInManifest(PathBuf), + PackageNotFound(PathBuf, String), Io(PathBuf, IoError), Toml(PathBuf, TomlError), } @@ -25,16 +26,32 @@ impl Display for Error { fn fmt(&self, f: &mut Formatter) -> FmtResult { f.write_str(match self { Self::InvalidArgs => "Invalid args.", - Self::ManifestNotAWorkspace => "The provided Cargo.toml does not contain a `[workspace]`", + Self::ManifestNotAWorkspace => { + "The provided Cargo.toml does not contain a `[workspace]`" + } Self::ManifestNotFound => "Didn't find Cargo.toml.", Self::ManifestPathNotFound => "The manifest-path must be a path to a Cargo.toml file", - Self::MultiplePackagesNotSupported => { - "Multiple packages are not yet supported (ie. in a `[workspace]`). Use `-p` to select a specific package." - } Self::RustcNotFound => "Didn't find rustc.", Self::GlobPatternError(error) => error, Self::Glob(error) => return error.fmt(f), - Self::UnexpectedWorkspace(path) => return write!(f, "Did not expect a `[workspace]` at {}", path.display()), + Self::UnexpectedWorkspace(path) => { + return write!(f, "Did not expect a `[workspace]` at `{}`", path.display()) + } + Self::NoPackageInManifest(manifest) => { + return write!( + f, + "Failed to parse manifest at `{}`: virtual manifests must be configured with `[workspace]`", + manifest.display() + ) + } + Self::PackageNotFound(workspace, name) => { + return write!( + f, + "package `{}` not found in workspace `{}`", + name, + workspace.display() + ) + } Self::Io(path, error) => return write!(f, "{}: {}", path.display(), error), Self::Toml(file, error) => return write!(f, "{}: {}", file.display(), error), }) diff --git a/src/manifest.rs b/src/manifest.rs index bdcc64a..6552a6b 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -16,7 +16,11 @@ impl Manifest { } #[derive(Clone, Debug, Deserialize)] +#[serde(rename_all = "kebab-case")] pub struct Workspace { + #[serde(default)] + pub default_members: Vec, + #[serde(default)] pub members: Vec, } diff --git a/src/subcommand.rs b/src/subcommand.rs index 20142a1..059028c 100644 --- a/src/subcommand.rs +++ b/src/subcommand.rs @@ -54,19 +54,28 @@ impl Subcommand { ); // Scan the given and all parent directories for a Cargo.toml containing a workspace - // TODO: If set, validate that the found workspace is either the given `--manifest-path`, - // or contains the given `--manifest-path` as member. 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 - utils::find_package_manifest_in_workspace(workspace_manifest_path, workspace, package)? - } else { - // Otherwise scan up the directories - utils::find_package_manifest(&search_path, package)? - }; + 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; diff --git a/src/utils.rs b/src/utils.rs index 7affe1d..6988deb 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -19,20 +19,6 @@ pub fn list_rust_files(dir: &Path) -> Result> { Ok(files) } -fn match_package_name(manifest: &Manifest, name: Option<&str>) -> bool { - if let Some(p) = &manifest.package { - if let Some(name) = name { - if name == p.name { - return true; - } - } else { - return true; - } - } - - false -} - /// Tries to find a package by the given `name` in the [workspace root] or member /// of the given [workspace] [`Manifest`]. /// @@ -43,25 +29,21 @@ fn match_package_name(manifest: &Manifest, name: Option<&str>) -> bool { pub fn find_package_manifest_in_workspace( workspace_manifest_path: &Path, workspace_manifest: &Manifest, - name: Option<&str>, + name: &str, ) -> Result<(PathBuf, Manifest)> { let workspace = workspace_manifest .workspace .as_ref() .ok_or(Error::ManifestNotAWorkspace)?; - // When building inside a workspace, require a package to be selected on the cmdline - // as selecting the right package is non-trivial and selecting multiple packages - // isn't currently supported yet. See the selection mechanism: - // https://doc.rust-lang.org/cargo/reference/workspaces.html#package-selection - let name = name.ok_or(Error::MultiplePackagesNotSupported)?; - // Check if the workspace manifest also contains a [package] - if match_package_name(workspace_manifest, Some(name)) { - return Ok(( - workspace_manifest_path.to_owned(), - workspace_manifest.clone(), - )); + 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 @@ -76,13 +58,20 @@ pub fn find_package_manifest_in_workspace( return Err(Error::UnexpectedWorkspace(manifest_path)); } - if match_package_name(&manifest, Some(name)) { - return Ok((manifest_path, manifest)); + if let Some(package) = &manifest.package { + if package.name == name { + return Ok((manifest_path, manifest)); + } + } else { + return Err(Error::NoPackageInManifest(manifest_path)); } } } - Err(Error::ManifestNotFound) + Err(Error::PackageNotFound( + workspace_manifest_path.into(), + name.into(), + )) } /// Recursively walk up the directories until finding a `Cargo.toml` @@ -91,23 +80,32 @@ pub fn find_package_manifest_in_workspace( /// 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).map_err(|e| Error::Io(path.to_owned(), e))?; - 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)?; + .find(|dir| dir.exists()) + .ok_or(Error::ManifestNotFound)?; - // This function shouldn't be called when a workspace exists. - if manifest.workspace.is_some() { - return Err(Error::UnexpectedWorkspace(manifest_path)); - } + let manifest = Manifest::parse_from_toml(&manifest_path)?; - if match_package_name(&manifest, name) { - return Ok((manifest_path, manifest)); + // 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)) } - Err(Error::ManifestNotFound) } /// Find the first `Cargo.toml` that contains a `[workspace]` @@ -115,7 +113,7 @@ pub fn find_workspace(potential_root: &Path) -> Result