Merge pull request #732 from epage/resolve

fix: Allow relative manifest paths

Author: epage

src/bin/set-version/set_version.rs

@@ -3,8 +3,7 @@ use std::path::Path;
 use std::path::PathBuf;
 
 use cargo_edit::{
-    colorize_stderr, find, manifest_from_pkgid, upgrade_requirement, workspace_members,
-    LocalManifest,
+    colorize_stderr, resolve_manifests, upgrade_requirement, workspace_members, LocalManifest,
 };
 use clap::Args;
 use termcolor::{BufferWriter, Color, ColorSpec, WriteColor};
@@ -110,13 +109,11 @@ fn exec(args: VersionArgs) -> CargoResult<()> {
         deprecated_message("The flag `--all` has been deprecated in favor of `--workspace`")?;
     }
     let all = workspace || all;
-    let manifests = if all {
-        Manifests::get_all(manifest_path.as_deref())
-    } else if let Some(ref pkgid) = pkgid {
-        Manifests::get_pkgid(manifest_path.as_deref(), pkgid)
-    } else {
-        Manifests::get_local_one(manifest_path.as_deref())
-    }?;
+    let manifests = Manifests(resolve_manifests(
+        manifest_path.as_deref(),
+        all,
+        pkgid.as_deref().into_iter().collect::<Vec<_>>(),
+    )?);
 
     if dry_run {
         dry_run_message()?;
@@ -190,51 +187,6 @@ fn exec(args: VersionArgs) -> CargoResult<()> {
 /// A collection of manifests.
 struct Manifests(Vec<cargo_metadata::Package>);
 
-impl Manifests {
-    /// Get all manifests in the workspace.
-    fn get_all(manifest_path: Option<&Path>) -> CargoResult<Self> {
-        let mut cmd = cargo_metadata::MetadataCommand::new();
-        cmd.no_deps();
-        if let Some(path) = manifest_path {
-            cmd.manifest_path(path);
-        }
-        let result = cmd
-            .exec()
-            .with_context(|| "Failed to get workspace metadata")?;
-        Ok(Self(result.packages))
-    }
-
-    fn get_pkgid(manifest_path: Option<&Path>, pkgid: &str) -> CargoResult<Self> {
-        let package = manifest_from_pkgid(manifest_path, pkgid)?;
-        Ok(Manifests(vec![package]))
-    }
-
-    /// Get the manifest specified by the manifest path. Try to make an educated guess if no path is
-    /// provided.
-    fn get_local_one(manifest_path: Option<&Path>) -> CargoResult<Self> {
-        let resolved_manifest_path: String = find(manifest_path)?.to_string_lossy().into();
-
-        let mut cmd = cargo_metadata::MetadataCommand::new();
-        cmd.no_deps();
-        if let Some(path) = manifest_path {
-            cmd.manifest_path(path);
-        }
-        let result = cmd.exec().with_context(|| "Invalid manifest")?;
-        let packages = result.packages;
-        let package = packages
-            .iter()
-            .find(|p| p.manifest_path == resolved_manifest_path)
-            // If we have successfully got metadata, but our manifest path does not correspond to a
-            // package, we must have been called against a virtual manifest.
-            .with_context(|| {
-                "Found virtual manifest, but this command requires running against an \
-                 actual package in this workspace. Try adding `--workspace`."
-            })?;
-
-        Ok(Manifests(vec![package.to_owned()]))
-    }
-}
-
 fn dry_run_message() -> CargoResult<()> {
     let colorchoice = colorize_stderr();
     let bufwtr = BufferWriter::stderr(colorchoice);

src/bin/upgrade/upgrade.rs

@@ -1,11 +1,11 @@
 use std::collections::BTreeSet;
 use std::io::Write;
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 
 use cargo_edit::{
-    colorize_stderr, find, get_latest_dependency, manifest_from_pkgid, registry_url,
-    set_dep_version, shell_status, shell_warn, update_registry_index, CargoResult, Context,
-    CrateSpec, Dependency, LocalManifest,
+    colorize_stderr, find, get_latest_dependency, registry_url, resolve_manifests, set_dep_version,
+    shell_status, shell_warn, update_registry_index, CargoResult, Context, CrateSpec, Dependency,
+    LocalManifest,
 };
 use clap::Args;
 use indexmap::IndexMap;
@@ -48,7 +48,7 @@ pub struct UpgradeArgs {
         conflicts_with = "all",
         conflicts_with = "workspace"
     )]
-    pkgid: Option<String>,
+    pkgid: Vec<String>,
 
     /// Upgrade all packages in the workspace.
     #[clap(
@@ -114,14 +114,12 @@ impl UpgradeArgs {
         self.all || self.workspace
     }
 
-    fn resolve_targets(&self) -> CargoResult<Vec<(LocalManifest, cargo_metadata::Package)>> {
-        if self.workspace() {
-            resolve_all(self.manifest_path.as_deref())
-        } else if let Some(pkgid) = self.pkgid.as_deref() {
-            resolve_pkgid(self.manifest_path.as_deref(), pkgid)
-        } else {
-            resolve_local_one(self.manifest_path.as_deref())
-        }
+    fn resolve_targets(&self) -> CargoResult<Vec<cargo_metadata::Package>> {
+        resolve_manifests(
+            self.manifest_path.as_deref(),
+            self.workspace(),
+            self.pkgid.iter().map(|s| s.as_str()).collect::<Vec<_>>(),
+        )
     }
 
     fn verbose<F>(&self, mut callback: F) -> CargoResult<()>
@@ -170,7 +168,8 @@ fn exec(args: UpgradeArgs) -> CargoResult<()> {
 
     let mut updated_registries = BTreeSet::new();
     let mut any_crate_modified = false;
-    for (mut manifest, package) in manifests {
+    for package in manifests {
+        let mut manifest = LocalManifest::try_new(package.manifest_path.as_std_path())?;
         let mut crate_modified = false;
         let manifest_path = manifest.path.clone();
         shell_status("Checking", &format!("{}'s dependencies", package.name))?;
@@ -363,17 +362,15 @@ fn exec(args: UpgradeArgs) -> CargoResult<()> {
     Ok(())
 }
 
-fn load_lockfile(
-    targets: &[(LocalManifest, cargo_metadata::Package)],
-) -> CargoResult<Vec<cargo_metadata::Package>> {
+fn load_lockfile(targets: &[cargo_metadata::Package]) -> CargoResult<Vec<cargo_metadata::Package>> {
     // Get locked dependencies. For workspaces with multiple Cargo.toml
     // files, there is only a single lockfile, so it suffices to get
     // metadata for any one of Cargo.toml files.
-    let (manifest, _package) = targets
+    let package = targets
         .get(0)
         .ok_or_else(|| anyhow::format_err!("Invalid cargo config"))?;
     let mut cmd = cargo_metadata::MetadataCommand::new();
-    cmd.manifest_path(manifest.path.clone());
+    cmd.manifest_path(package.manifest_path.clone());
     cmd.features(cargo_metadata::CargoOpt::AllFeatures);
     cmd.other_options(vec!["--locked".to_string()]);
 
@@ -402,68 +399,6 @@ fn find_locked_version(
     None
 }
 
-/// Get all manifests in the workspace.
-fn resolve_all(
-    manifest_path: Option<&Path>,
-) -> CargoResult<Vec<(LocalManifest, cargo_metadata::Package)>> {
-    let mut cmd = cargo_metadata::MetadataCommand::new();
-    cmd.no_deps();
-    if let Some(path) = manifest_path {
-        cmd.manifest_path(path);
-    }
-    let result = cmd
-        .exec()
-        .with_context(|| "Failed to get workspace metadata")?;
-    result
-        .packages
-        .into_iter()
-        .map(|package| {
-            Ok((
-                LocalManifest::try_new(Path::new(&package.manifest_path))?,
-                package,
-            ))
-        })
-        .collect::<CargoResult<Vec<_>>>()
-}
-
-fn resolve_pkgid(
-    manifest_path: Option<&Path>,
-    pkgid: &str,
-) -> CargoResult<Vec<(LocalManifest, cargo_metadata::Package)>> {
-    let package = manifest_from_pkgid(manifest_path, pkgid)?;
-    let manifest = LocalManifest::try_new(Path::new(&package.manifest_path))?;
-    Ok(vec![(manifest, package)])
-}
-
-/// Get the manifest specified by the manifest path. Try to make an educated guess if no path is
-/// provided.
-fn resolve_local_one(
-    manifest_path: Option<&Path>,
-) -> CargoResult<Vec<(LocalManifest, cargo_metadata::Package)>> {
-    let resolved_manifest_path: String = find(manifest_path)?.to_string_lossy().into();
-
-    let manifest = LocalManifest::find(manifest_path)?;
-
-    let mut cmd = cargo_metadata::MetadataCommand::new();
-    cmd.no_deps();
-    if let Some(path) = manifest_path {
-        cmd.manifest_path(path);
-    }
-    let result = cmd.exec().with_context(|| "Invalid manifest")?;
-    let packages = result.packages;
-    let package = packages
-        .iter()
-        .find(|p| p.manifest_path == resolved_manifest_path)
-        // If we have successfully got metadata, but our manifest path does not correspond to a
-        // package, we must have been called against a virtual manifest.
-        .with_context(|| {
-            "Found virtual manifest, but this command requires running against an \
-                 actual package in this workspace. Try adding `--workspace`."
-        })?;
-
-    Ok(vec![(manifest, package.to_owned())])
-}
-
 fn old_version_compatible(old_version_req: &str, new_version: &str) -> bool {
     let old_version_req = match VersionReq::parse(old_version_req) {
         Ok(req) => req,

src/lib.rs

@@ -34,7 +34,7 @@ pub use dependency::Source;
 pub use errors::*;
 pub use fetch::{get_latest_dependency, update_registry_index};
 pub use manifest::{find, get_dep_version, set_dep_version, LocalManifest, Manifest};
-pub use metadata::{manifest_from_pkgid, workspace_members};
+pub use metadata::{manifest_from_pkgid, resolve_manifests, workspace_members};
 pub use registry::registry_url;
 pub use util::{colorize_stderr, shell_print, shell_status, shell_warn, Color, ColorChoice};
 pub use version::{upgrade_requirement, VersionExt};

src/manifest.rs

@@ -6,6 +6,7 @@ use std::{env, str};
 use semver::Version;
 
 use super::errors::*;
+use super::metadata::find_manifest_path;
 
 #[derive(PartialEq, Eq, Hash, Ord, PartialOrd, Clone, Debug, Copy)]
 pub enum DepKind {
@@ -69,8 +70,6 @@ impl From<DepKind> for DepTable {
     }
 }
 
-const MANIFEST_FILENAME: &str = "Cargo.toml";
-
 /// A Cargo manifest
 #[derive(Debug, Clone)]
 pub struct Manifest {
@@ -423,27 +422,10 @@ pub fn find(specified: Option<&Path>) -> CargoResult<PathBuf> {
         {
             Ok(path.to_owned())
         }
-        Some(path) => search(path),
-        None => search(&env::current_dir().with_context(|| "Failed to get current directory")?),
-    }
-}
-
-/// Search for Cargo.toml in this directory and recursively up the tree until one is found.
-fn search(dir: &Path) -> CargoResult<PathBuf> {
-    let mut current_dir = dir;
-
-    loop {
-        let manifest = current_dir.join(MANIFEST_FILENAME);
-        if fs::metadata(&manifest).is_ok() {
-            return Ok(manifest);
-        }
-
-        current_dir = match current_dir.parent() {
-            Some(current_dir) => current_dir,
-            None => {
-                anyhow::bail!("Unable to find Cargo.toml for {}", dir.display());
-            }
-        };
+        Some(path) => find_manifest_path(path),
+        None => find_manifest_path(
+            &env::current_dir().with_context(|| "Failed to get current directory")?,
+        ),
     }
 }
 

src/metadata.rs

@@ -58,3 +58,57 @@ fn canonicalize_path(
 
     path
 }
+
+/// Determine packages selected by user
+pub fn resolve_manifests(
+    manifest_path: Option<&Path>,
+    workspace: bool,
+    pkgid: Vec<&str>,
+) -> CargoResult<Vec<Package>> {
+    let manifest_path = manifest_path.map(|p| Ok(p.to_owned())).unwrap_or_else(|| {
+        find_manifest_path(
+            &std::env::current_dir().with_context(|| "Failed to get current directory")?,
+        )
+    })?;
+    let manifest_path = dunce::canonicalize(manifest_path)?;
+
+    let mut cmd = cargo_metadata::MetadataCommand::new();
+    cmd.no_deps();
+    cmd.manifest_path(&manifest_path);
+    let result = cmd.exec().with_context(|| "Invalid manifest")?;
+    let pkgs = if workspace {
+        result.packages
+    } else if !pkgid.is_empty() {
+        pkgid
+            .into_iter()
+            .map(|id| {
+                result
+                    .packages
+                    .iter()
+                    .find(|pkg| pkg.name == id)
+                    .map(|p| p.to_owned())
+                    .with_context(|| format!("could not find pkgid {}", id))
+            })
+            .collect::<Result<Vec<_>, anyhow::Error>>()?
+    } else {
+        result
+            .packages
+            .iter()
+            .find(|p| p.manifest_path == manifest_path)
+            .map(|p| vec![(p.to_owned())])
+            .unwrap_or_else(|| result.packages)
+    };
+    Ok(pkgs)
+}
+
+/// Search for Cargo.toml in this directory and recursively up the tree until one is found.
+pub(crate) fn find_manifest_path(dir: &Path) -> CargoResult<std::path::PathBuf> {
+    const MANIFEST_FILENAME: &str = "Cargo.toml";
+    for path in dir.ancestors() {
+        let manifest = path.join(MANIFEST_FILENAME);
+        if std::fs::metadata(&manifest).is_ok() {
+            return Ok(manifest);
+        }
+    }
+    anyhow::bail!("Unable to find Cargo.toml for {}", dir.display());
+}

tests/cmd/upgrade/invalid_manifest.toml

@@ -3,17 +3,21 @@ args = ["upgrade"]
 status.code = 1
 stdout = ""
 stderr = """
-Error: Unable to parse Cargo.toml
+Error: Invalid manifest
 
 Caused by:
-    0: Manifest not valid TOML
-    1: TOML parse error at line 1, column 6
-         |
-       1 | This is clearly not a valid Cargo.toml.
-         |      ^
-       Unexpected `i`
-       Expected `.` or `=`
-       
+    `cargo metadata` exited with an error: error: failed to parse manifest at `[CWD]/Cargo.toml`
+    
+    Caused by:
+      could not parse input as TOML
+    
+    Caused by:
+      TOML parse error at line 1, column 6
+        |
+      1 | This is clearly not a valid Cargo.toml.
+        |      ^
+      Unexpected `i`
+      Expected `.` or `=`
 """
 fs.sandbox = true
 

tests/cmd/upgrade/invalid_virtual_manifest.toml

@@ -3,7 +3,7 @@ args = ["upgrade", "--workspace"]
 status.code = 1
 stdout = ""
 stderr = """
-Error: Failed to get workspace metadata
+Error: Invalid manifest
 
 Caused by:
     `cargo metadata` exited with an error: error: failed to parse manifest at `[CWD]/Cargo.toml`