Skip to content

Commit

Permalink
Use $PWD and --manifest-path to select a default package in works…
Browse files Browse the repository at this point in the history
…paces

This restores original, prevalent `--manifest-path` usage to select a
package containing the APK crate in a `[workspace]`, instead of using
`-p` for that.
  • Loading branch information
MarijnS95 committed Nov 25, 2022
1 parent fb7a414 commit 2b8ac40
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 22 deletions.
4 changes: 0 additions & 4 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ pub enum Error {
ManifestNotFound,
RustcNotFound,
ManifestPathNotFound,
MultiplePackagesNotSupported,
GlobPatternError(&'static str),
Glob(GlobError),
UnexpectedWorkspace(PathBuf),
Expand All @@ -28,9 +27,6 @@ impl Display for Error {
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),
Expand Down
4 changes: 4 additions & 0 deletions src/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ impl Manifest {
}

#[derive(Clone, Debug, Deserialize)]
#[serde(rename_all = "kebab-case")]
pub struct Workspace {
#[serde(default)]
pub default_members: Vec<String>,
#[serde(default)]
pub members: Vec<String>,
}

Expand Down
29 changes: 18 additions & 11 deletions src/subcommand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,19 +54,26 @@ 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
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?
// See also the TODO in find_package_manifest() about bailing on the first Cargo.toml.
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;
Expand Down
10 changes: 3 additions & 7 deletions src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,19 +43,13 @@ 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((
Expand Down Expand Up @@ -106,6 +100,8 @@ pub fn find_package_manifest(path: &Path, name: Option<&str>) -> Result<(PathBuf
if match_package_name(&manifest, name) {
return Ok((manifest_path, manifest));
}
// TODO: The above always returns true if no name is set. Should we still keep scanning upwards
// when a name is set, and we encountered a Cargo.toml, but it did not contain a package by that name?
}
Err(Error::ManifestNotFound)
}
Expand Down

0 comments on commit 2b8ac40

Please sign in to comment.