From ce5146a3ce2ef30a76962aa9c85339f2f9d06572 Mon Sep 17 00:00:00 2001 From: nasa Date: Thu, 24 Jan 2019 10:49:11 +0900 Subject: [PATCH 1/8] test: Add ensure_install test --- tests/testsuite/install.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index b7955461f68..9b023022926 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -469,6 +469,27 @@ Add --force to overwrite .run(); } +#[test] +fn install_ensure() { + let p = project() + .file("src/bin/foo-bin1.rs", "fn main() {}") + .file("src/bin/foo-bin2.rs", "fn main() {}") + .build(); + + cargo_process("install --ensure --path").arg(p.root()).run(); + cargo_process("install --ensure --path") + .arg(p.root()) + .with_stderr( + "\ +[INSTALLING] foo v0.0.1 [..] +binary `foo-bin1[..]` already exists in destination as part of `foo v0.0.1 ([..])` +binary `foo-bin2[..]` already exists in destination as part of `foo v0.0.1 ([..])` +Add --force to overwrite +", + ) + .run(); +} + #[test] fn install_force() { let p = project().file("src/main.rs", "fn main() {}").build(); From 08865c1c95b52e771327ba9747c9160741183c99 Mon Sep 17 00:00:00 2001 From: nasa Date: Thu, 24 Jan 2019 10:49:34 +0900 Subject: [PATCH 2/8] test: Add a test of being overwritten --- tests/testsuite/install.rs | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index 9b023022926..4df6c4cafe3 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -490,6 +490,41 @@ Add --force to overwrite .run(); } +#[test] +fn install_ensure_force() { + let p = project() + .at("foo1") + .file("Cargo.toml", &basic_manifest("foo", "0.1.0")) + .file("src/main.rs", "fn main() {}") + .build(); + + cargo_process("install --path").arg(p.root()).run(); + cargo_process("install --list") + .with_stdout( + "\ +foo v0.1.0 ([..]): + foo[..] +", + ) + .run(); + + let p = project() + .at("foo2") + .file("Cargo.toml", &basic_manifest("foo", "0.2.0")) + .file("src/main.rs", "fn main() {}") + .build(); + + cargo_process("install --ensure --path").arg(p.root()).run(); + cargo_process("install --list") + .with_stdout( + "\ +foo v0.2.0 ([..]): + foo[..] +", + ) + .run(); +} + #[test] fn install_force() { let p = project().file("src/main.rs", "fn main() {}").build(); From 7f52d8a5273520f7afd859934cbd3935fd795f32 Mon Sep 17 00:00:00 2001 From: nasa Date: Thu, 24 Jan 2019 10:53:01 +0900 Subject: [PATCH 3/8] refacotr: Make error message formatting a separate function --- src/cargo/ops/cargo_install.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 4e1036abb52..0007c877f04 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -419,11 +419,20 @@ fn check_overwrites( failure::bail!("specified package has no binaries") } let duplicates = find_duplicates(dst, pkg, filter, prev); + if force || duplicates.is_empty() { return Ok(duplicates); } - // Format the error message. + + let msg = check_overwrites_format_error_message(&duplicates); + Err(failure::format_err!("{}", msg)) +} + +fn check_overwrites_format_error_message( + duplicates: &BTreeMap>, +) -> String { let mut msg = String::new(); + for (bin, p) in duplicates.iter() { msg.push_str(&format!("binary `{}` already exists in destination", bin)); if let Some(p) = p.as_ref() { @@ -433,7 +442,8 @@ fn check_overwrites( } } msg.push_str("Add --force to overwrite"); - Err(failure::format_err!("{}", msg)) + + msg } fn find_duplicates( From 184210d90287faadd598a24ccfe34dc804eee3a3 Mon Sep 17 00:00:00 2001 From: nasa Date: Thu, 24 Jan 2019 10:53:20 +0900 Subject: [PATCH 4/8] feat: Add ensure option to cargo_install --- src/bin/cargo/commands/install.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/bin/cargo/commands/install.rs b/src/bin/cargo/commands/install.rs index 8702fd99a39..2c222323eed 100644 --- a/src/bin/cargo/commands/install.rs +++ b/src/bin/cargo/commands/install.rs @@ -24,6 +24,13 @@ pub fn cli() -> App { )) .arg_jobs() .arg(opt("force", "Force overwriting existing crates or binaries").short("f")) + .arg( + opt( + "ensure", + "If you already have a suitable version, simply leaves it as-is.", + ) + .short("e"), + ) .arg_features() .arg(opt("debug", "Build in debug mode instead of release mode")) .arg_targets_bins_examples( From 53303823ab6bfe4fdb20178b8e6e9c20ea6589f9 Mon Sep 17 00:00:00 2001 From: nasa Date: Thu, 24 Jan 2019 10:54:39 +0900 Subject: [PATCH 5/8] feat: Add ensure option to cargo_install main logic --- src/bin/cargo/commands/install.rs | 1 + src/cargo/ops/cargo_install.rs | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/bin/cargo/commands/install.rs b/src/bin/cargo/commands/install.rs index 2c222323eed..64cf1209ec8 100644 --- a/src/bin/cargo/commands/install.rs +++ b/src/bin/cargo/commands/install.rs @@ -135,6 +135,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { version, &compile_opts, args.is_present("force"), + args.is_present("ensure"), )?; } Ok(()) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 0007c877f04..8483dd75e23 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -42,6 +42,7 @@ pub fn install( vers: Option<&str>, opts: &ops::CompileOptions<'_>, force: bool, + ensure: bool, ) -> CargoResult<()> { let root = resolve_root(root, opts.config)?; let map = SourceConfigMap::new(opts.config)?; @@ -56,6 +57,7 @@ pub fn install( vers, opts, force, + ensure, true, )?; (true, false) @@ -75,6 +77,7 @@ pub fn install( vers, opts, force, + ensure, first, ) { Ok(()) => succeeded.push(krate), @@ -137,6 +140,7 @@ fn install_one( vers: Option<&str>, opts: &ops::CompileOptions<'_>, force: bool, + ensure: bool, is_first_install: bool, ) -> CargoResult<()> { let config = opts.config; @@ -248,7 +252,7 @@ fn install_one( let metadata = metadata(config, root)?; let list = read_crate_list(&metadata)?; let dst = metadata.parent().join("bin"); - check_overwrites(&dst, pkg, &opts.filter, &list, force)?; + check_overwrites(&dst, pkg, &opts.filter, &list, force, ensure)?; } let exec: Arc = Arc::new(DefaultExecutor); @@ -287,7 +291,7 @@ fn install_one( let metadata = metadata(config, root)?; let mut list = read_crate_list(&metadata)?; let dst = metadata.parent().join("bin"); - let duplicates = check_overwrites(&dst, pkg, &opts.filter, &list, force)?; + let duplicates = check_overwrites(&dst, pkg, &opts.filter, &list, force, ensure)?; fs::create_dir_all(&dst)?; @@ -411,6 +415,7 @@ fn check_overwrites( filter: &ops::CompileFilter, prev: &CrateListingV1, force: bool, + ensure: bool, ) -> CargoResult>> { // If explicit --bin or --example flags were passed then those'll // get checked during cargo_compile, we only care about the "build From 5b2d0b0a1d5b04ab7080bda4c6bdd5809438326d Mon Sep 17 00:00:00 2001 From: nasa Date: Thu, 24 Jan 2019 10:55:16 +0900 Subject: [PATCH 6/8] feat: Add 'ensure' implementation --- src/cargo/ops/cargo_install.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 8483dd75e23..cefd54af6bd 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -430,6 +430,21 @@ fn check_overwrites( } let msg = check_overwrites_format_error_message(&duplicates); + + if ensure { + let is_installed_old = duplicates + .iter() + .filter(|(_, v)| v.is_some()) + .all(|(_, v)| v.unwrap().version() < pkg.version()); + + if is_installed_old { + return Ok(duplicates); + } + + eprintln!("{}", msg); + std::process::exit(0) + } + Err(failure::format_err!("{}", msg)) } From 9780f8c6ce72020645d83a4e43f68715584d0d42 Mon Sep 17 00:00:00 2001 From: nasa Date: Sun, 27 Jan 2019 12:18:43 +0900 Subject: [PATCH 7/8] feat: Make it the default behavior --- src/bin/cargo/commands/install.rs | 8 -------- src/cargo/ops/cargo_install.rs | 30 ++++++++++-------------------- 2 files changed, 10 insertions(+), 28 deletions(-) diff --git a/src/bin/cargo/commands/install.rs b/src/bin/cargo/commands/install.rs index 64cf1209ec8..8702fd99a39 100644 --- a/src/bin/cargo/commands/install.rs +++ b/src/bin/cargo/commands/install.rs @@ -24,13 +24,6 @@ pub fn cli() -> App { )) .arg_jobs() .arg(opt("force", "Force overwriting existing crates or binaries").short("f")) - .arg( - opt( - "ensure", - "If you already have a suitable version, simply leaves it as-is.", - ) - .short("e"), - ) .arg_features() .arg(opt("debug", "Build in debug mode instead of release mode")) .arg_targets_bins_examples( @@ -135,7 +128,6 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { version, &compile_opts, args.is_present("force"), - args.is_present("ensure"), )?; } Ok(()) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index cefd54af6bd..134b9eb6bed 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -42,7 +42,6 @@ pub fn install( vers: Option<&str>, opts: &ops::CompileOptions<'_>, force: bool, - ensure: bool, ) -> CargoResult<()> { let root = resolve_root(root, opts.config)?; let map = SourceConfigMap::new(opts.config)?; @@ -57,7 +56,6 @@ pub fn install( vers, opts, force, - ensure, true, )?; (true, false) @@ -77,7 +75,6 @@ pub fn install( vers, opts, force, - ensure, first, ) { Ok(()) => succeeded.push(krate), @@ -140,7 +137,6 @@ fn install_one( vers: Option<&str>, opts: &ops::CompileOptions<'_>, force: bool, - ensure: bool, is_first_install: bool, ) -> CargoResult<()> { let config = opts.config; @@ -252,7 +248,7 @@ fn install_one( let metadata = metadata(config, root)?; let list = read_crate_list(&metadata)?; let dst = metadata.parent().join("bin"); - check_overwrites(&dst, pkg, &opts.filter, &list, force, ensure)?; + check_overwrites(&dst, pkg, &opts.filter, &list, force)?; } let exec: Arc = Arc::new(DefaultExecutor); @@ -291,7 +287,7 @@ fn install_one( let metadata = metadata(config, root)?; let mut list = read_crate_list(&metadata)?; let dst = metadata.parent().join("bin"); - let duplicates = check_overwrites(&dst, pkg, &opts.filter, &list, force, ensure)?; + let duplicates = check_overwrites(&dst, pkg, &opts.filter, &list, force)?; fs::create_dir_all(&dst)?; @@ -415,7 +411,6 @@ fn check_overwrites( filter: &ops::CompileFilter, prev: &CrateListingV1, force: bool, - ensure: bool, ) -> CargoResult>> { // If explicit --bin or --example flags were passed then those'll // get checked during cargo_compile, we only care about the "build @@ -431,28 +426,23 @@ fn check_overwrites( let msg = check_overwrites_format_error_message(&duplicates); - if ensure { - let is_installed_old = duplicates - .iter() - .filter(|(_, v)| v.is_some()) - .all(|(_, v)| v.unwrap().version() < pkg.version()); - - if is_installed_old { - return Ok(duplicates); - } + let is_installed_old = duplicates + .iter() + .filter(|(_, v)| v.is_some()) + .all(|(_, v)| v.unwrap().version() < pkg.version()); - eprintln!("{}", msg); - std::process::exit(0) + if is_installed_old { + return Ok(duplicates); } - Err(failure::format_err!("{}", msg)) + eprintln!("{}", msg); + std::process::exit(0) } fn check_overwrites_format_error_message( duplicates: &BTreeMap>, ) -> String { let mut msg = String::new(); - for (bin, p) in duplicates.iter() { msg.push_str(&format!("binary `{}` already exists in destination", bin)); if let Some(p) = p.as_ref() { From 554481492d98b0f3b905125ca3506a9814f36a1b Mon Sep 17 00:00:00 2001 From: nasa Date: Sun, 27 Jan 2019 12:18:53 +0900 Subject: [PATCH 8/8] test: Make it the default behavior --- tests/testsuite/concurrent.rs | 11 ++--------- tests/testsuite/install.rs | 29 +++-------------------------- 2 files changed, 5 insertions(+), 35 deletions(-) diff --git a/tests/testsuite/concurrent.rs b/tests/testsuite/concurrent.rs index 90e7847807c..c1d6cd6c6d9 100644 --- a/tests/testsuite/concurrent.rs +++ b/tests/testsuite/concurrent.rs @@ -105,15 +105,8 @@ fn one_install_should_be_bad() { } else { (b, a) }; - execs() - .with_status(101) - .with_stderr_contains( - "[ERROR] binary `foo[..]` already exists in destination as part of `[..]`", - ) - .run_output(&bad); - execs() - .with_stderr_contains("warning: be sure to add `[..]` to your PATH [..]") - .run_output(&good); + execs().run_output(&bad); + execs().run_output(&good); assert_has_installed_exe(cargo_home(), "foo"); } diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index 4df6c4cafe3..5bca026abed 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -247,11 +247,10 @@ fn install_path() { cargo_process("install --path").arg(p.root()).run(); assert_has_installed_exe(cargo_home(), "foo"); p.cargo("install --path .") - .with_status(101) .with_stderr( "\ [INSTALLING] foo v0.0.1 [..] -[ERROR] binary `foo[..]` already exists in destination as part of `foo v0.0.1 [..]` +binary `foo[..]` already exists in destination as part of `foo v0.0.1 [..]` Add --force to overwrite ", ) @@ -456,28 +455,6 @@ fn install_twice() { cargo_process("install --path").arg(p.root()).run(); cargo_process("install --path") - .arg(p.root()) - .with_status(101) - .with_stderr( - "\ -[INSTALLING] foo v0.0.1 [..] -[ERROR] binary `foo-bin1[..]` already exists in destination as part of `foo v0.0.1 ([..])` -binary `foo-bin2[..]` already exists in destination as part of `foo v0.0.1 ([..])` -Add --force to overwrite -", - ) - .run(); -} - -#[test] -fn install_ensure() { - let p = project() - .file("src/bin/foo-bin1.rs", "fn main() {}") - .file("src/bin/foo-bin2.rs", "fn main() {}") - .build(); - - cargo_process("install --ensure --path").arg(p.root()).run(); - cargo_process("install --ensure --path") .arg(p.root()) .with_stderr( "\ @@ -491,7 +468,7 @@ Add --force to overwrite } #[test] -fn install_ensure_force() { +fn install_version_update() { let p = project() .at("foo1") .file("Cargo.toml", &basic_manifest("foo", "0.1.0")) @@ -514,7 +491,7 @@ foo v0.1.0 ([..]): .file("src/main.rs", "fn main() {}") .build(); - cargo_process("install --ensure --path").arg(p.root()).run(); + cargo_process("install --path").arg(p.root()).run(); cargo_process("install --list") .with_stdout( "\