From c8554bb9514f6e0216b5ef0dc425dce0866c2c91 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 21 Sep 2021 12:52:01 -0500 Subject: [PATCH] feat(add): Workspace support If the user doesn't specify anything beyond a positional name or path, we'll check if its a workspace member and add it according to workspace best practices - Use `version` and `path` for published dependencies so it uses the local version for dev and the upcoming version on publish - Use `path` for unpublished dependencies for development without creating version bumping cycles. Fixes #480 --- src/bin/add/args.rs | 128 ++++++++++++++++++++++++++++++++--------- src/bin/add/main.rs | 1 + tests/cargo-add.rs | 135 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 232 insertions(+), 32 deletions(-) diff --git a/src/bin/add/args.rs b/src/bin/add/args.rs index 5e4e1168a3..fe529704eb 100644 --- a/src/bin/add/args.rs +++ b/src/bin/add/args.rs @@ -4,6 +4,8 @@ use cargo_edit::{find, registry_url, Dependency}; use cargo_edit::{get_latest_dependency, CrateName}; +use cargo_metadata::Package; +use std::convert::TryInto; use std::path::PathBuf; use structopt::{clap::AppSettings, StructOpt}; @@ -168,7 +170,11 @@ impl Args { } } - fn parse_single_dependency(&self, crate_name: &str) -> Result { + fn parse_single_dependency( + &self, + crate_name: &str, + workspace_members: &[Package], + ) -> Result { let crate_name = CrateName::new(crate_name); if let Some(mut dependency) = crate_name.parse_as_version()? { @@ -185,7 +191,28 @@ impl Args { Ok(dependency) } else if crate_name.is_url_or_path() { - Ok(crate_name.parse_crate_name_from_uri()?) + let mut dependency = crate_name.parse_crate_name_from_uri()?; + // dev-dependencies do not need the version populated + if !self.dev { + let dep_path = dependency.path().map(ToOwned::to_owned); + if let Some(dep_path) = dep_path { + if let Some(package) = workspace_members.iter().find(|p| { + p.manifest_path.parent().map(|p| p.as_std_path()) + == Some(dep_path.as_path()) + }) { + let v = format!( + "{prefix}{version}", + prefix = self.get_upgrade_prefix(), + version = package.version + ); + dependency = dependency.set_version(&v); + if let Some(registry) = &self.registry { + dependency = dependency.set_registry(registry); + } + } + } + } + Ok(dependency) } else { assert_eq!(self.git.is_some() && self.vers.is_some(), false); assert_eq!(self.git.is_some() && self.path.is_some(), false); @@ -210,20 +237,45 @@ impl Args { }; if self.git.is_none() && self.path.is_none() && self.vers.is_none() { - let dep = get_latest_dependency( - crate_name.name(), - self.allow_prerelease, - &find(&self.manifest_path)?, - ®istry_url, - )?; - let v = format!( - "{prefix}{version}", - prefix = self.get_upgrade_prefix(), - // If version is unavailable `get_latest_dependency` must have - // returned `Err(FetchVersionError::GetVersion)` - version = dep.version().unwrap_or_else(|| unreachable!()) - ); - dependency = dep.set_version(&v); + // Only special-case workspaces when the user doesn't provide any extra + // information, otherwise, trust the user. + if let Some(package) = workspace_members + .iter() + .find(|p| p.name == crate_name.name()) + { + dependency = dependency.set_path( + package + .manifest_path + .parent() + .expect("at least parent dir") + .as_std_path() + .to_owned(), + ); + // dev-dependencies do not need the version populated + if !self.dev { + let v = format!( + "{prefix}{version}", + prefix = self.get_upgrade_prefix(), + version = package.version + ); + dependency = dependency.set_version(&v); + } + } else { + dependency = get_latest_dependency( + crate_name.name(), + self.allow_prerelease, + &find(&self.manifest_path)?, + ®istry_url, + )?; + let v = format!( + "{prefix}{version}", + prefix = self.get_upgrade_prefix(), + // If version is unavailable `get_latest_dependency` must have + // returned `Err(FetchVersionError::GetVersion)` + version = dependency.version().unwrap_or_else(|| unreachable!()) + ); + dependency = dependency.set_version(&v); + } } // Set the registry after getting the latest version as @@ -237,6 +289,25 @@ impl Args { /// Build dependencies from arguments pub fn parse_dependencies(&self) -> Result> { + let mut cmd = cargo_metadata::MetadataCommand::new(); + cmd.no_deps(); + let result = cmd.exec().chain_err(|| "Invalid manifest")?; + let workspace_members: std::collections::HashSet<_> = + result.workspace_members.into_iter().collect(); + let workspace_members: Vec<_> = result + .packages + .into_iter() + .filter(|p| workspace_members.contains(&p.id)) + .map(|mut p| { + if let Ok(manifest_path) = p.manifest_path.canonicalize() { + if let Ok(manifest_path) = manifest_path.try_into() { + p.manifest_path = manifest_path; + } + } + p + }) + .collect(); + if self.crates.len() > 1 && (self.git.is_some() || self.path.is_some() || self.vers.is_some()) { @@ -254,16 +325,17 @@ impl Args { self.crates .iter() .map(|crate_name| { - self.parse_single_dependency(crate_name).map(|x| { - let mut x = x - .set_optional(self.optional) - .set_features(self.features.clone()) - .set_default_features(!self.no_default_features); - if let Some(ref rename) = self.rename { - x = x.set_rename(rename); - } - x - }) + self.parse_single_dependency(crate_name, &workspace_members) + .map(|x| { + let mut x = x + .set_optional(self.optional) + .set_features(self.features.clone()) + .set_default_features(!self.no_default_features); + if let Some(ref rename) = self.rename { + x = x.set_rename(rename); + } + x + }) }) .collect() } @@ -359,8 +431,8 @@ mod tests { ..Args::default() }; assert_eq!( - args_path.parse_dependencies().unwrap(), - vec![Dependency::new("cargo-edit").set_path(std::path::PathBuf::from(self_path))] + args_path.parse_dependencies().unwrap()[0].path().unwrap(), + self_path ); } } diff --git a/src/bin/add/main.rs b/src/bin/add/main.rs index 3fdaa37498..6bad4e54d8 100644 --- a/src/bin/add/main.rs +++ b/src/bin/add/main.rs @@ -60,6 +60,7 @@ mod errors { CargoEditLib(::cargo_edit::Error, ::cargo_edit::ErrorKind); } foreign_links { + CargoMetadata(::cargo_metadata::Error)#[doc = "An error from the cargo_metadata crate"]; Io(::std::io::Error); } } diff --git a/tests/cargo-add.rs b/tests/cargo-add.rs index ed6a0818ba..c4a319f3a4 100644 --- a/tests/cargo-add.rs +++ b/tests/cargo-add.rs @@ -429,6 +429,14 @@ fn adds_git_branch_using_flag() { fn adds_local_source_using_flag() { let tmpdir = assert_fs::TempDir::new().expect("failed to construct temporary directory"); let manifest_path = tmpdir.child("primary/Cargo.toml"); + tmpdir + .child("Cargo.toml") + .write_str( + r#"[workspace] +members = ["primary", "dependency"] +"#, + ) + .expect("Manifest is writeable"); manifest_path .write_str( r#"[package] @@ -453,7 +461,7 @@ path = "dummy.rs" ) .expect("Manifest is writeable"); - Command::cargo_bin("cargo-add") + let assert = Command::cargo_bin("cargo-add") .expect("can find bin") .args(&[ "add", @@ -465,6 +473,7 @@ path = "dummy.rs" .current_dir(manifest_path.path().parent().expect("there is a parent")) .assert() .success(); + println!("Succeeded: {}", assert); manifest_path.assert( r#"[package] name = "cargo-list-test-fixture" @@ -479,7 +488,7 @@ cargo-list-test-fixture-dependency = { path = "../dependency" } ); // check this works with other flags (e.g. --dev) as well - Command::cargo_bin("cargo-add") + let assert = Command::cargo_bin("cargo-add") .expect("can find bin") .args(&[ "add", @@ -492,6 +501,7 @@ cargo-list-test-fixture-dependency = { path = "../dependency" } .current_dir(manifest_path.path().parent().expect("there is a parent")) .assert() .success(); + println!("Succeeded: {}", assert); manifest_path.assert( r#"[package] name = "cargo-list-test-fixture" @@ -556,6 +566,14 @@ fn adds_git_source_without_flag() { fn adds_local_source_without_flag() { let tmpdir = assert_fs::TempDir::new().expect("failed to construct temporary directory"); let manifest_path = tmpdir.child("primary/Cargo.toml"); + tmpdir + .child("Cargo.toml") + .write_str( + r#"[workspace] +members = ["primary", "dependency"] +"#, + ) + .expect("Manifest is writeable"); manifest_path .write_str( r#"[package] @@ -580,13 +598,89 @@ path = "dummy.rs" ) .expect("Manifest is writeable"); - Command::cargo_bin("cargo-add") + let assert = Command::cargo_bin("cargo-add") + .expect("can find bin") + .args(&["add", "../dependency"]) + .env("CARGO_IS_TEST", "1") + .current_dir(manifest_path.path().parent().expect("there is a parent")) + .assert() + .success(); + println!("Succeeded: {}", assert); + manifest_path.assert( + r#"[package] +name = "cargo-list-test-fixture" +version = "0.0.0" + +[lib] +path = "dummy.rs" + +[dependencies] +cargo-list-test-fixture-dependency = { version = "0.0.0", path = "../dependency" } +"#, + ); + + // check this works with other flags (e.g. --dev) as well + let assert = Command::cargo_bin("cargo-add") + .expect("can find bin") + .args(&["add", "../dependency", "--dev"]) + .env("CARGO_IS_TEST", "1") + .current_dir(manifest_path.path().parent().expect("there is a parent")) + .assert() + .success(); + println!("Succeeded: {}", assert); + manifest_path.assert( + r#"[package] +name = "cargo-list-test-fixture" +version = "0.0.0" + +[lib] +path = "dummy.rs" + +[dependencies] +cargo-list-test-fixture-dependency = { version = "0.0.0", path = "../dependency" } + +[dev-dependencies] +cargo-list-test-fixture-dependency = { path = "../dependency" } +"#, + ); +} + +#[test] +fn adds_local_source_without_flag_without_workspace() { + let tmpdir = assert_fs::TempDir::new().expect("failed to construct temporary directory"); + let manifest_path = tmpdir.child("primary/Cargo.toml"); + manifest_path + .write_str( + r#"[package] +name = "cargo-list-test-fixture" +version = "0.0.0" + +[lib] +path = "dummy.rs" +"#, + ) + .expect("Manifest is writeable"); + tmpdir + .child("dependency/Cargo.toml") + .write_str( + r#"[package] +name = "cargo-list-test-fixture-dependency" +version = "0.0.0" + +[lib] +path = "dummy.rs" +"#, + ) + .expect("Manifest is writeable"); + + let assert = Command::cargo_bin("cargo-add") .expect("can find bin") .args(&["add", "../dependency"]) .env("CARGO_IS_TEST", "1") .current_dir(manifest_path.path().parent().expect("there is a parent")) .assert() .success(); + println!("Succeeded: {}", assert); manifest_path.assert( r#"[package] name = "cargo-list-test-fixture" @@ -601,13 +695,14 @@ cargo-list-test-fixture-dependency = { path = "../dependency" } ); // check this works with other flags (e.g. --dev) as well - Command::cargo_bin("cargo-add") + let assert = Command::cargo_bin("cargo-add") .expect("can find bin") .args(&["add", "../dependency", "--dev"]) .env("CARGO_IS_TEST", "1") .current_dir(manifest_path.path().parent().expect("there is a parent")) .assert() .success(); + println!("Succeeded: {}", assert); manifest_path.assert( r#"[package] name = "cargo-list-test-fixture" @@ -629,6 +724,14 @@ cargo-list-test-fixture-dependency = { path = "../dependency" } fn adds_local_source_with_version_flag() { let tmpdir = assert_fs::TempDir::new().expect("failed to construct temporary directory"); let manifest_path = tmpdir.child("primary/Cargo.toml"); + tmpdir + .child("Cargo.toml") + .write_str( + r#"[workspace] +members = ["primary", "dependency"] +"#, + ) + .expect("Manifest is writeable"); manifest_path .write_str( r#"[package] @@ -715,6 +818,14 @@ cargo-list-test-fixture-dependency = { version = "0.4.3", path = "../dependency" fn adds_local_source_with_version_flag_and_semver_metadata() { let tmpdir = assert_fs::TempDir::new().expect("failed to construct temporary directory"); let manifest_path = tmpdir.child("primary/Cargo.toml"); + tmpdir + .child("Cargo.toml") + .write_str( + r#"[workspace] +members = ["primary", "dependency"] +"#, + ) + .expect("Manifest is writeable"); manifest_path .write_str( r#"[package] @@ -801,6 +912,14 @@ cargo-list-test-fixture-dependency = { version = "0.4.3", path = "../dependency" fn adds_local_source_with_inline_version_notation() { let tmpdir = assert_fs::TempDir::new().expect("failed to construct temporary directory"); let manifest_path = tmpdir.child("primary/Cargo.toml"); + tmpdir + .child("Cargo.toml") + .write_str( + r#"[workspace] +members = ["primary", "dependency"] +"#, + ) + .expect("Manifest is writeable"); manifest_path .write_str( r#"[package] @@ -1417,6 +1536,14 @@ path = "dummy.rs" let tmpdir = assert_fs::TempDir::new().expect("failed to construct temporary directory"); let manifest_path = tmpdir.child("primary/Cargo.toml"); + tmpdir + .child("Cargo.toml") + .write_str( + r#"[workspace] +members = ["primary", "dependency"] +"#, + ) + .expect("Manifest is writeable"); manifest_path .write_str(orig_manifest) .expect("Manifest is writeable");