Skip to content

Commit

Permalink
Auto merge of #7246 - ehuss:install-remove-orphan, r=alexcrichton
Browse files Browse the repository at this point in the history
`cargo install`: Remove orphaned executables.

When a new version of a package is installed that no longer contains an executable from a previous version, this change causes those orphaned executables to also be removed.

I can place this new behavior behind the `install-upgrade` feature gate if anyone is uncomfortable with changing the behavior now.

cc #6797
  • Loading branch information
bors committed Aug 19, 2019
2 parents e01a8f3 + 5a59b80 commit f42275c
Show file tree
Hide file tree
Showing 5 changed files with 152 additions and 17 deletions.
27 changes: 15 additions & 12 deletions src/cargo/ops/cargo_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
66 changes: 64 additions & 2 deletions src/cargo/ops/cargo_install.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand All @@ -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};
Expand Down Expand Up @@ -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?,
Expand Down Expand Up @@ -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<String, Option<PackageId>>,
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<PackageId, BTreeSet<String>> = 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(())
}
8 changes: 8 additions & 0 deletions src/cargo/ops/common_for_install_and_uninstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,14 @@ pub fn exe_names(pkg: &Package, filter: &ops::CompileFilter) -> BTreeSet<String>
.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,
Expand Down
3 changes: 1 addition & 2 deletions tests/testsuite/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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[..]
Expand Down
65 changes: 64 additions & 1 deletion tests/testsuite/install_upgrade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -782,3 +790,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", &[]);
}

0 comments on commit f42275c

Please sign in to comment.