From 09be0eef54a1e66d38012f1c1e7fd3db424e9938 Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Tue, 28 Nov 2023 17:35:20 +0800 Subject: [PATCH 1/5] Fixed `cargo uninstall` behavior in Windows --- src/cargo/ops/cargo_uninstall.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cargo/ops/cargo_uninstall.rs b/src/cargo/ops/cargo_uninstall.rs index 1f22e191e..57d72f9c9 100644 --- a/src/cargo/ops/cargo_uninstall.rs +++ b/src/cargo/ops/cargo_uninstall.rs @@ -146,11 +146,13 @@ fn uninstall_pkgid( } tracker.remove(pkgid, &bins); } - tracker.save()?; + for bin in to_remove { config.shell().status("Removing", bin.display())?; paths::remove_file(bin)?; } + // Only Save the tracker when remove Bin successfully. + tracker.save()?; Ok(()) } From 0fd4fd357b440d4155661c3b770ed8ddee43d13e Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Wed, 29 Nov 2023 17:37:01 +0800 Subject: [PATCH 2/5] Add test case for `cargo uninstall` while try to uninstalling a running Bin. --- src/cargo/ops/cargo_uninstall.rs | 6 ++- tests/testsuite/install.rs | 80 ++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/src/cargo/ops/cargo_uninstall.rs b/src/cargo/ops/cargo_uninstall.rs index 57d72f9c9..caad4fa74 100644 --- a/src/cargo/ops/cargo_uninstall.rs +++ b/src/cargo/ops/cargo_uninstall.rs @@ -147,12 +147,14 @@ fn uninstall_pkgid( tracker.remove(pkgid, &bins); } + // This should have been handled last, but now restore it to reproduce the problem on the Windows platform. + // See issue #3364. + tracker.save()?; + for bin in to_remove { config.shell().status("Removing", bin.display())?; paths::remove_file(bin)?; } - // Only Save the tracker when remove Bin successfully. - tracker.save()?; Ok(()) } diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index fd53b607b..2cf967637 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -2507,3 +2507,83 @@ fn install_incompat_msrv() { ") .with_status(101).run(); } + +#[cfg(windows)] +#[cargo_test] +fn uninstall_running_binary() { + Package::new("foo", "0.0.1") + .file("src/lib.rs", "") + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + + [[bin]] + name = "foo" + path = "src/main.rs" +"#, + ) + .file( + "src/main.rs", + r#" + use std::{{thread, time}}; + fn main() { + println!("start longrunning job."); + thread::sleep(time::Duration::from_secs(3)); + println!("end longrunning job."); + } +"#, + ) + .publish(); + + cargo_process("install foo") + .with_stderr( + "\ +[UPDATING] `[..]` index +[DOWNLOADING] crates ... +[DOWNLOADED] foo v0.0.1 (registry [..]) +[INSTALLING] foo v0.0.1 +[COMPILING] foo v0.0.1 +[FINISHED] release [optimized] target(s) in [..] +[INSTALLING] [CWD]/home/.cargo/bin/foo[EXE] +[INSTALLED] package `foo v0.0.1` (executable `foo[EXE]`) +[WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries +", + ) + .run(); + assert_has_installed_exe(cargo_home(), "foo"); + + use cargo_util::ProcessBuilder; + use std::thread; + let foo_bin = cargo_test_support::install::cargo_home() + .join("bin") + .join(cargo_test_support::install::exe("foo")); + + let t = thread::spawn(|| ProcessBuilder::new(foo_bin).exec().unwrap()); + + cargo_process("uninstall foo") + .with_status(101) + .with_stderr_contains("[ERROR] failed to remove file `[CWD]/home/.cargo/bin/foo[EXE]`") + .run(); + assert_has_installed_exe(cargo_home(), "foo"); + + t.join().unwrap(); + cargo_process("uninstall foo") + .with_stderr("[REMOVING] [CWD]/home/.cargo/bin/foo[EXE]") + .run(); + cargo_process("install foo") + .with_stderr( + "\ +[UPDATING] `[..]` index +[INSTALLING] foo v0.0.1 +[COMPILING] foo v0.0.1 +[FINISHED] release [optimized] target(s) in [..] +[INSTALLING] [CWD]/home/.cargo/bin/foo[EXE] +[INSTALLED] package `foo v0.0.1` (executable `foo[EXE]`) +[WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries +", + ) + .run(); +} From 26cf2740d78cfc97c2343355da7a8515de082699 Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Thu, 30 Nov 2023 15:29:51 +0800 Subject: [PATCH 3/5] Combine the behavior for `cargo uninstall` --- src/cargo/ops/cargo_uninstall.rs | 17 +++------ .../ops/common_for_install_and_uninstall.rs | 37 +++++++++++++++++++ 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/cargo/ops/cargo_uninstall.rs b/src/cargo/ops/cargo_uninstall.rs index caad4fa74..f040ad2f2 100644 --- a/src/cargo/ops/cargo_uninstall.rs +++ b/src/cargo/ops/cargo_uninstall.rs @@ -138,22 +138,15 @@ fn uninstall_pkgid( } if bins.is_empty() { - to_remove.extend(installed.iter().map(|b| dst.join(b))); - tracker.remove(pkgid, &installed); + to_remove = installed.iter().collect(); } else { - for bin in bins.iter() { - to_remove.push(dst.join(bin)); - } - tracker.remove(pkgid, &bins); + to_remove = bins.iter().collect(); } - // This should have been handled last, but now restore it to reproduce the problem on the Windows platform. - // See issue #3364. - tracker.save()?; - for bin in to_remove { - config.shell().status("Removing", bin.display())?; - paths::remove_file(bin)?; + let bin_path = dst.join(bin); + config.shell().status("Removing", bin_path.display())?; + tracker.remove_bin_then_save(pkgid, bin, &bin_path)?; } Ok(()) diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index 77eb431c9..117368957 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -7,6 +7,7 @@ use std::rc::Rc; use std::task::Poll; use anyhow::{bail, format_err, Context as _}; +use cargo_util::paths; use ops::FilterRule; use serde::{Deserialize, Serialize}; @@ -319,6 +320,20 @@ impl InstallTracker { self.v1.remove(pkg_id, bins); self.v2.remove(pkg_id, bins); } + + /// Remove a bin after it successfully had been removed in disk and the save the tracker at last. + pub fn remove_bin_then_save( + &mut self, + pkg_id: PackageId, + bin: &str, + bin_path: &PathBuf, + ) -> CargoResult<()> { + paths::remove_file(bin_path)?; + self.v1.remove_bin(pkg_id, bin); + self.v2.remove_bin(pkg_id, bin); + self.save()?; + Ok(()) + } } impl CrateListingV1 { @@ -359,6 +374,17 @@ impl CrateListingV1 { } } + fn remove_bin(&mut self, pkg_id: PackageId, bin: &str) { + let mut installed = match self.v1.entry(pkg_id) { + btree_map::Entry::Occupied(e) => e, + btree_map::Entry::Vacant(..) => panic!("v1 unexpected missing `{}`", pkg_id), + }; + installed.get_mut().remove(bin); + if installed.get().is_empty() { + installed.remove(); + } + } + fn save(&self, lock: &FileLock) -> CargoResult<()> { let mut file = lock.file(); file.seek(SeekFrom::Start(0))?; @@ -468,6 +494,17 @@ impl CrateListingV2 { } } + fn remove_bin(&mut self, pkg_id: PackageId, bin: &str) { + let mut info_entry = match self.installs.entry(pkg_id) { + btree_map::Entry::Occupied(e) => e, + btree_map::Entry::Vacant(..) => panic!("v1 unexpected missing `{}`", pkg_id), + }; + info_entry.get_mut().bins.remove(bin); + if info_entry.get().bins.is_empty() { + info_entry.remove(); + } + } + fn save(&self, lock: &FileLock) -> CargoResult<()> { let mut file = lock.file(); file.seek(SeekFrom::Start(0))?; From 1e3b4231dacd79c75d331669384638d6af4532c7 Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Thu, 30 Nov 2023 15:46:02 +0800 Subject: [PATCH 4/5] chore: fixed clippy --- src/cargo/ops/cargo_uninstall.rs | 14 +++++++------- tests/testsuite/install.rs | 2 -- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/cargo/ops/cargo_uninstall.rs b/src/cargo/ops/cargo_uninstall.rs index f040ad2f2..7da773f61 100644 --- a/src/cargo/ops/cargo_uninstall.rs +++ b/src/cargo/ops/cargo_uninstall.rs @@ -6,7 +6,6 @@ use crate::util::errors::CargoResult; use crate::util::Config; use crate::util::Filesystem; use anyhow::bail; -use cargo_util::paths; use std::collections::BTreeSet; use std::env; @@ -103,7 +102,6 @@ fn uninstall_pkgid( bins: &[String], config: &Config, ) -> CargoResult<()> { - let mut to_remove = Vec::new(); let installed = match tracker.installed_bins(pkgid) { Some(bins) => bins.clone(), None => bail!("package `{}` is not installed", pkgid), @@ -137,11 +135,13 @@ fn uninstall_pkgid( } } - if bins.is_empty() { - to_remove = installed.iter().collect(); - } else { - to_remove = bins.iter().collect(); - } + let to_remove: Vec<&String> = { + if bins.is_empty() { + installed.iter().collect() + } else { + bins.iter().collect() + } + }; for bin in to_remove { let bin_path = dst.join(bin); diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index 2cf967637..b2ffeed79 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -2530,9 +2530,7 @@ fn uninstall_running_binary() { r#" use std::{{thread, time}}; fn main() { - println!("start longrunning job."); thread::sleep(time::Duration::from_secs(3)); - println!("end longrunning job."); } "#, ) From db144c9d0aed5567def7bab22e0343baaadb556f Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Fri, 1 Dec 2023 18:18:05 +0800 Subject: [PATCH 5/5] Add tracker verification for test case `uninstall_running_binary` --- src/cargo/ops/cargo_uninstall.rs | 10 ++-- .../ops/common_for_install_and_uninstall.rs | 2 +- tests/testsuite/install.rs | 56 +++++++++++++------ 3 files changed, 44 insertions(+), 24 deletions(-) diff --git a/src/cargo/ops/cargo_uninstall.rs b/src/cargo/ops/cargo_uninstall.rs index 7da773f61..7b9fdccd4 100644 --- a/src/cargo/ops/cargo_uninstall.rs +++ b/src/cargo/ops/cargo_uninstall.rs @@ -135,18 +135,18 @@ fn uninstall_pkgid( } } - let to_remove: Vec<&String> = { + let to_remove = { if bins.is_empty() { - installed.iter().collect() + installed } else { - bins.iter().collect() + bins } }; for bin in to_remove { - let bin_path = dst.join(bin); + let bin_path = dst.join(&bin); config.shell().status("Removing", bin_path.display())?; - tracker.remove_bin_then_save(pkgid, bin, &bin_path)?; + tracker.remove_bin_then_save(pkgid, &bin, &bin_path)?; } Ok(()) diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index 117368957..3da593403 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -321,7 +321,7 @@ impl InstallTracker { self.v2.remove(pkg_id, bins); } - /// Remove a bin after it successfully had been removed in disk and the save the tracker at last. + /// Remove a bin after it successfully had been removed in disk and then save the tracker at last. pub fn remove_bin_then_save( &mut self, pkg_id: PackageId, diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index b2ffeed79..db35d6996 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -3,6 +3,7 @@ use std::fs::{self, OpenOptions}; use std::io::prelude::*; use std::path::Path; +use std::thread; use cargo_test_support::compare; use cargo_test_support::cross_compile; @@ -11,10 +12,10 @@ use cargo_test_support::registry::{self, registry_path, Package}; use cargo_test_support::{ basic_manifest, cargo_process, no_such_file_err_msg, project, project_in, symlink_supported, t, }; -use cargo_util::ProcessError; +use cargo_util::{ProcessBuilder, ProcessError}; use cargo_test_support::install::{ - assert_has_installed_exe, assert_has_not_installed_exe, cargo_home, + assert_has_installed_exe, assert_has_not_installed_exe, cargo_home, exe, }; use cargo_test_support::paths::{self, CargoPathExt}; use std::env; @@ -2508,7 +2509,17 @@ fn install_incompat_msrv() { .with_status(101).run(); } -#[cfg(windows)] +fn assert_tracker_noexistence(key: &str) { + let v1_data: toml::Value = + toml::from_str(&fs::read_to_string(cargo_home().join(".crates.toml")).unwrap()).unwrap(); + let v2_data: serde_json::Value = + serde_json::from_str(&fs::read_to_string(cargo_home().join(".crates2.json")).unwrap()) + .unwrap(); + + assert!(v1_data["v1"].get(key).is_none()); + assert!(v2_data["installs"][key].is_null()); +} + #[cargo_test] fn uninstall_running_binary() { Package::new("foo", "0.0.1") @@ -2553,24 +2564,33 @@ fn uninstall_running_binary() { .run(); assert_has_installed_exe(cargo_home(), "foo"); - use cargo_util::ProcessBuilder; - use std::thread; - let foo_bin = cargo_test_support::install::cargo_home() - .join("bin") - .join(cargo_test_support::install::exe("foo")); - + let foo_bin = cargo_home().join("bin").join(exe("foo")); let t = thread::spawn(|| ProcessBuilder::new(foo_bin).exec().unwrap()); + let key = "foo 0.0.1 (registry+https://github.com/rust-lang/crates.io-index)"; - cargo_process("uninstall foo") - .with_status(101) - .with_stderr_contains("[ERROR] failed to remove file `[CWD]/home/.cargo/bin/foo[EXE]`") - .run(); - assert_has_installed_exe(cargo_home(), "foo"); + #[cfg(windows)] + { + cargo_process("uninstall foo") + .with_status(101) + .with_stderr_contains("[ERROR] failed to remove file `[CWD]/home/.cargo/bin/foo[EXE]`") + .run(); + t.join().unwrap(); + cargo_process("uninstall foo") + .with_stderr("[REMOVING] [CWD]/home/.cargo/bin/foo[EXE]") + .run(); + }; + + #[cfg(not(windows))] + { + cargo_process("uninstall foo") + .with_stderr("[REMOVING] [CWD]/home/.cargo/bin/foo[EXE]") + .run(); + t.join().unwrap(); + }; + + assert_has_not_installed_exe(cargo_home(), "foo"); + assert_tracker_noexistence(key); - t.join().unwrap(); - cargo_process("uninstall foo") - .with_stderr("[REMOVING] [CWD]/home/.cargo/bin/foo[EXE]") - .run(); cargo_process("install foo") .with_stderr( "\