From 5a59b809669d3cf39784337d5a4123c82a0c49b6 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 12 Aug 2019 21:44:14 -0700 Subject: [PATCH] `cargo install`: Remove orphaned executables. --- src/cargo/ops/cargo_compile.rs | 27 ++++---- src/cargo/ops/cargo_install.rs | 66 ++++++++++++++++++- .../ops/common_for_install_and_uninstall.rs | 8 +++ tests/testsuite/install.rs | 3 +- tests/testsuite/install_upgrade.rs | 65 +++++++++++++++++- 5 files changed, 152 insertions(+), 17 deletions(-) diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index c68d4fc1b1d..dde087e0406 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -443,6 +443,9 @@ impl CompileFilter { all_bens: bool, all_targets: bool, ) -> CompileFilter { + if all_targets { + return CompileFilter::new_all_targets(); + } let rule_lib = if lib_only { LibRule::True } else { @@ -453,18 +456,7 @@ impl CompileFilter { let rule_exms = FilterRule::new(exms, all_exms); let rule_bens = FilterRule::new(bens, all_bens); - if all_targets { - CompileFilter::Only { - all_targets: true, - lib: LibRule::Default, - bins: FilterRule::All, - examples: FilterRule::All, - benches: FilterRule::All, - tests: FilterRule::All, - } - } else { - CompileFilter::new(rule_lib, rule_bins, rule_tsts, rule_exms, rule_bens) - } + CompileFilter::new(rule_lib, rule_bins, rule_tsts, rule_exms, rule_bens) } /// Construct a CompileFilter from underlying primitives. @@ -496,6 +488,17 @@ impl CompileFilter { } } + pub fn new_all_targets() -> CompileFilter { + CompileFilter::Only { + all_targets: true, + lib: LibRule::Default, + bins: FilterRule::All, + examples: FilterRule::All, + benches: FilterRule::All, + tests: FilterRule::All, + } + } + pub fn need_dev_deps(&self, mode: CompileMode) -> bool { match mode { CompileMode::Test | CompileMode::Doctest | CompileMode::Bench => true, diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 04f9befe33b..3373e703043 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, BTreeSet, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::path::{Path, PathBuf}; use std::sync::Arc; use std::{env, fs}; @@ -9,7 +9,7 @@ use tempfile::Builder as TempFileBuilder; use crate::core::compiler::Freshness; use crate::core::compiler::{DefaultExecutor, Executor}; use crate::core::resolver::ResolveOpts; -use crate::core::{Edition, PackageId, PackageIdSpec, Source, SourceId, Workspace}; +use crate::core::{Edition, Package, PackageId, PackageIdSpec, Source, SourceId, Workspace}; use crate::ops; use crate::ops::common_for_install_and_uninstall::*; use crate::sources::{GitSource, SourceConfigMap}; @@ -414,6 +414,13 @@ fn install_one( rustc.verbose_version, ); + if let Err(e) = remove_orphaned_bins(&ws, &mut tracker, &duplicates, pkg, &dst) { + // Don't hard error on remove. + config + .shell() + .warn(format!("failed to remove orphan: {:?}", e))?; + } + match tracker.save() { Err(err) => replace_result.chain_err(|| err)?, Ok(_) => replace_result?, @@ -521,3 +528,58 @@ pub fn install_list(dst: Option<&str>, config: &Config) -> CargoResult<()> { } Ok(()) } + +/// Removes executables that are no longer part of a package that was +/// previously installed. +fn remove_orphaned_bins( + ws: &Workspace<'_>, + tracker: &mut InstallTracker, + duplicates: &BTreeMap>, + pkg: &Package, + dst: &Path, +) -> CargoResult<()> { + let filter = ops::CompileFilter::new_all_targets(); + let all_self_names = exe_names(pkg, &filter); + let mut to_remove: HashMap> = HashMap::new(); + // For each package that we stomped on. + for other_pkg in duplicates.values() { + // Only for packages with the same name. + if let Some(other_pkg) = other_pkg { + if other_pkg.name() == pkg.name() { + // Check what the old package had installed. + if let Some(installed) = tracker.installed_bins(*other_pkg) { + // If the old install has any names that no longer exist, + // add them to the list to remove. + for installed_name in installed { + if !all_self_names.contains(installed_name.as_str()) { + to_remove + .entry(*other_pkg) + .or_default() + .insert(installed_name.clone()); + } + } + } + } + } + } + + for (old_pkg, bins) in to_remove { + tracker.remove(old_pkg, &bins); + for bin in bins { + let full_path = dst.join(bin); + if full_path.exists() { + ws.config().shell().status( + "Removing", + format!( + "executable `{}` from previous version {}", + full_path.display(), + old_pkg + ), + )?; + paths::remove_file(&full_path) + .chain_err(|| format!("failed to remove {:?}", full_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 498caee3e62..f61a00d39a8 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -743,6 +743,14 @@ pub fn exe_names(pkg: &Package, filter: &ops::CompileFilter) -> BTreeSet .filter(|t| t.is_bin()) .map(|t| to_exe(t.name())) .collect(), + CompileFilter::Only { + all_targets: true, .. + } => pkg + .targets() + .iter() + .filter(|target| target.is_executable()) + .map(|target| to_exe(target.name())) + .collect(), CompileFilter::Only { ref bins, ref examples, diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index 5ebb3018829..ef909f1eb23 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -531,6 +531,7 @@ fn install_force_partial_overlap() { [FINISHED] release [optimized] target(s) in [..] [INSTALLING] [CWD]/home/.cargo/bin/foo-bin3[EXE] [REPLACING] [CWD]/home/.cargo/bin/foo-bin2[EXE] +[REMOVING] executable `[..]/bin/foo-bin1[EXE]` from previous version foo v0.0.1 [..] [INSTALLED] package `foo v0.2.0 ([..]/foo2)` (executable `foo-bin3[EXE]`) [REPLACED] package `foo v0.0.1 ([..]/foo)` with `foo v0.2.0 ([..]/foo2)` (executable `foo-bin2[EXE]`) [WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries @@ -541,8 +542,6 @@ fn install_force_partial_overlap() { cargo_process("install --list") .with_stdout( "\ -foo v0.0.1 ([..]): - foo-bin1[..] foo v0.2.0 ([..]): foo-bin2[..] foo-bin3[..] diff --git a/tests/testsuite/install_upgrade.rs b/tests/testsuite/install_upgrade.rs index ff4157f1a71..0ac5d3abfe9 100644 --- a/tests/testsuite/install_upgrade.rs +++ b/tests/testsuite/install_upgrade.rs @@ -65,6 +65,7 @@ fn installed_process(name: &str) -> Execs { /// Check that the given package name/version has the following bins listed in /// the trackers. Also verifies that both trackers are in sync and valid. +/// Pass in an empty `bins` list to assert that the package is *not* installed. fn validate_trackers(name: &str, version: &str, bins: &[&str]) { let v1 = load_crates1(); let v1_table = v1.get("v1").unwrap().as_table().unwrap(); @@ -88,7 +89,14 @@ fn validate_trackers(name: &str, version: &str, bins: &[&str]) { .map(|b| b.as_str().unwrap().to_string()) .collect(); if pkg_id.name().as_str() == name && pkg_id.version().to_string() == version { - assert_eq!(bins, v1_bins); + if bins.is_empty() { + panic!( + "Expected {} to not be installed, but found: {:?}", + name, v1_bins + ); + } else { + assert_eq!(bins, v1_bins); + } } let pkg_id_value = serde_json::to_value(&pkg_id).unwrap(); let pkg_id_str = pkg_id_value.as_str().unwrap(); @@ -784,3 +792,58 @@ Add --force to overwrite .with_status(101) .run(); } + +#[cargo_test] +fn deletes_orphaned() { + // When an executable is removed from a project, upgrading should remove it. + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + "#, + ) + .file("src/main.rs", "fn main() {}") + .file("src/bin/other.rs", "fn main() {}") + .file("examples/ex1.rs", "fn main() {}") + .build(); + p.cargo("install -Z install-upgrade --path . --bins --examples") + .masquerade_as_nightly_cargo() + .run(); + assert!(installed_exe("other").exists()); + + // Remove a binary, add a new one, and bump the version. + fs::remove_file(p.root().join("src/bin/other.rs")).unwrap(); + p.change_file("examples/ex2.rs", "fn main() {}"); + p.change_file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.2.0" + "#, + ); + p.cargo("install -Z install-upgrade --path . --bins --examples") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[INSTALLING] foo v0.2.0 [..] +[COMPILING] foo v0.2.0 [..] +[FINISHED] release [..] +[INSTALLING] [..]/.cargo/bin/ex2[EXE] +[REPLACING] [..]/.cargo/bin/ex1[EXE] +[REPLACING] [..]/.cargo/bin/foo[EXE] +[REMOVING] executable `[..]/.cargo/bin/other[EXE]` from previous version foo v0.1.0 [..] +[INSTALLED] package `foo v0.2.0 [..]` (executable `ex2[EXE]`) +[REPLACED] package `foo v0.1.0 [..]` with `foo v0.2.0 [..]` (executables `ex1[EXE]`, `foo[EXE]`) +[WARNING] be sure to add [..] +", + ) + .run(); + assert!(!installed_exe("other").exists()); + validate_trackers("foo", "0.2.0", &["foo", "ex1", "ex2"]); + // 0.1.0 should not have any entries. + validate_trackers("foo", "0.1.0", &[]); +}