From 9ea1f2af548992201cb2985874ae93937facfcf2 Mon Sep 17 00:00:00 2001 From: Gleb Kozyrev Date: Fri, 15 Apr 2016 14:20:45 +0300 Subject: [PATCH 1/2] Add `--force` flag to cargo install Close #2082 Adding `--force` (`-f`) instructs cargo to overwrite existing binaries (updating the metadata accordingly). This allows updating crates via `cargo install -f `. Installation happens in two stages now: binaries are copied into a temporary subdirectory of the destination first, then moved into destination. This should catch some errors earlier. In case of installation error cargo will remove new binaries but won't attempt to undo successful overwrites. --- src/bin/install.rs | 10 +- src/cargo/ops/cargo_install.rs | 184 ++++++++++++++++++++++++++------- tests/support/mod.rs | 1 + tests/test_cargo_install.rs | 145 +++++++++++++++++++++++++- 4 files changed, 296 insertions(+), 44 deletions(-) diff --git a/src/bin/install.rs b/src/bin/install.rs index bd2c3191bf3..3f8d50baebe 100644 --- a/src/bin/install.rs +++ b/src/bin/install.rs @@ -15,6 +15,7 @@ pub struct Options { flag_color: Option, flag_root: Option, flag_list: bool, + flag_force: bool, arg_crate: Option, flag_vers: Option, @@ -46,6 +47,7 @@ Build and install options: -h, --help Print this message -j N, --jobs N The number of jobs to run in parallel --features FEATURES Space-separated list of features to activate + -f, --force Force overwriting existing crates or binaries --no-default-features Do not build the `default` feature --debug Build in debug mode instead of release mode --bin NAME Only install the binary NAME @@ -55,7 +57,7 @@ Build and install options: -q, --quiet Less output printed to stdout --color WHEN Coloring: auto, always, never -This command manages Cargo's local set of install binary crates. Only packages +This command manages Cargo's local set of installed binary crates. Only packages which have [[bin]] targets can be installed, and all binaries are installed into the installation root's `bin` folder. The installation root is determined, in order of precedence, by `--root`, `$CARGO_INSTALL_ROOT`, the `install.root` @@ -75,6 +77,10 @@ crate has multiple binaries, the `--bin` argument can selectively install only one of them, and if you'd rather install examples the `--example` argument can be used as well. +By default cargo will refuse to overwrite existing binaries. The `--force` flag +enables overwriting existing binaries. Thus you can reinstall a crate with +`cargo install --force `. + As a special convenience, omitting the specification entirely will install the crate in the current directory. That is, `install` is equivalent to the more explicit `install --path .`. @@ -130,7 +136,7 @@ pub fn execute(options: Options, config: &Config) -> CliResult> { if options.flag_list { try!(ops::install_list(root, config)); } else { - try!(ops::install(root, krate, &source, vers, &compile_opts)); + try!(ops::install(root, krate, &source, vers, &compile_opts, options.flag_force)); } Ok(None) } diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index aab9c1e25af..237f3aba957 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -32,6 +32,12 @@ struct Transaction { bins: Vec, } +impl Transaction { + fn success(mut self) { + self.bins.clear(); + } +} + impl Drop for Transaction { fn drop(&mut self) { for bin in self.bins.iter() { @@ -44,7 +50,8 @@ pub fn install(root: Option<&str>, krate: Option<&str>, source_id: &SourceId, vers: Option<&str>, - opts: &ops::CompileOptions) -> CargoResult<()> { + opts: &ops::CompileOptions, + force: bool) -> CargoResult<()> { let config = opts.config; let root = try!(resolve_root(root, config)); let (pkg, source) = if source_id.is_git() { @@ -77,7 +84,7 @@ pub fn install(root: Option<&str>, let metadata = try!(metadata(config, &root)); let list = try!(read_crate_list(metadata.file())); let dst = metadata.parent().join("bin"); - try!(check_overwrites(&dst, &pkg, &opts.filter, &list)); + try!(check_overwrites(&dst, &pkg, &opts.filter, &list, force)); } let mut td_opt = None; @@ -102,24 +109,109 @@ pub fn install(root: Option<&str>, human(format!("failed to compile `{}`, intermediate artifacts can be \ found at `{}`", pkg, target_dir.display())) })); + let binaries: Vec<(&str, &Path)> = try!(compile.binaries.iter().map(|bin| { + let name = bin.file_name().unwrap(); + if let Some(s) = name.to_str() { + Ok((s, bin.as_ref())) + } else { + bail!("Binary `{:?}` name can't be serialized into string", name) + } + }).collect::>()); let metadata = try!(metadata(config, &root)); let mut list = try!(read_crate_list(metadata.file())); let dst = metadata.parent().join("bin"); - try!(check_overwrites(&dst, &pkg, &opts.filter, &list)); + let duplicates = try!(check_overwrites(&dst, &pkg, &opts.filter, &list, force)); - let mut t = Transaction { bins: Vec::new() }; try!(fs::create_dir_all(&dst)); - for bin in compile.binaries.iter() { - let dst = dst.join(bin.file_name().unwrap()); + + // Copy all binaries to a temporary directory under `dst` first, catching + // some failure modes (e.g. out of space) before touching the existing + // binaries. This directory will get cleaned up via RAII. + let staging_dir = try!(TempDir::new_in(&dst, "cargo-install")); + for &(bin, src) in binaries.iter() { + let dst = staging_dir.path().join(bin); + try!(fs::copy(src, &dst).chain_error(|| { + human(format!("failed to copy `{}` to `{}`", src.display(), + dst.display())) + })); + } + + let (to_replace, to_install): (Vec<&str>, Vec<&str>) = + binaries.iter().map(|&(bin, _)| bin) + .partition(|&bin| duplicates.contains_key(bin)); + + let mut installed = Transaction { bins: Vec::new() }; + + // Move the temporary copies into `dst` starting with new binaries. + for bin in to_install.iter() { + let src = staging_dir.path().join(bin); + let dst = dst.join(bin); try!(config.shell().status("Installing", dst.display())); - try!(fs::copy(&bin, &dst).chain_error(|| { - human(format!("failed to copy `{}` to `{}`", bin.display(), + try!(fs::rename(&src, &dst).chain_error(|| { + human(format!("failed to move `{}` to `{}`", src.display(), dst.display())) })); - t.bins.push(dst); + installed.bins.push(dst); } + // Repeat for binaries which replace existing ones but don't pop the error + // up until after updating metadata. + let mut replaced_names = Vec::new(); + let result = { + let mut try_install = || -> CargoResult<()> { + for &bin in to_replace.iter() { + let src = staging_dir.path().join(bin); + let dst = dst.join(bin); + try!(config.shell().status("Replacing", dst.display())); + try!(fs::rename(&src, &dst).chain_error(|| { + human(format!("failed to move `{}` to `{}`", src.display(), + dst.display())) + })); + replaced_names.push(bin); + } + Ok(()) + }; + try_install() + }; + + // Update records of replaced binaries. + for &bin in replaced_names.iter() { + if let Some(&Some(ref p)) = duplicates.get(bin) { + if let Some(set) = list.v1.get_mut(p) { + set.remove(bin); + } + } + list.v1.entry(pkg.package_id().clone()) + .or_insert_with(|| BTreeSet::new()) + .insert(bin.to_string()); + } + + // Remove empty metadata lines. + let pkgs = list.v1.iter() + .filter_map(|(p, set)| if set.is_empty() { Some(p.clone()) } else { None }) + .collect::>(); + for p in pkgs.iter() { + list.v1.remove(p); + } + + // If installation was successful record newly installed binaries. + if result.is_ok() { + list.v1.entry(pkg.package_id().clone()) + .or_insert_with(|| BTreeSet::new()) + .extend(to_install.iter().map(|s| s.to_string())); + } + + let write_result = write_crate_list(metadata.file(), list); + match write_result { + // Replacement error (if any) isn't actually caused by write error + // but this seems to be the only way to show both. + Err(err) => try!(result.chain_error(|| err)), + Ok(_) => try!(result), + } + + // Reaching here means all actions have succeeded. Clean up. + installed.success(); if !source_id.is_path() { // Don't bother grabbing a lock as we're going to blow it all away // anyway. @@ -127,15 +219,6 @@ pub fn install(root: Option<&str>, try!(fs::remove_dir_all(&target_dir)); } - list.v1.entry(pkg.package_id().clone()).or_insert_with(|| { - BTreeSet::new() - }).extend(t.bins.iter().map(|t| { - t.file_name().unwrap().to_string_lossy().into_owned() - })); - try!(write_crate_list(metadata.file(), list)); - - t.bins.truncate(0); - // Print a warning that if this directory isn't in PATH that they won't be // able to run these commands. let path = env::var_os("PATH").unwrap_or(OsString::new()); @@ -225,38 +308,61 @@ fn one(mut i: I, f: F) -> CargoResult> fn check_overwrites(dst: &Path, pkg: &Package, filter: &ops::CompileFilter, - prev: &CrateListingV1) -> CargoResult<()> { + prev: &CrateListingV1, + force: bool) -> CargoResult>> { + if let CompileFilter::Everything = *filter { + // If explicit --bin or --example flags were passed then those'll + // get checked during cargo_compile, we only care about the "build + // everything" case here + if pkg.targets().iter().filter(|t| t.is_bin()).next().is_none() { + 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 mut msg = String::new(); + for (ref bin, p) in duplicates.iter() { + msg.push_str(&format!("binary `{}` already exists in destination", bin)); + if let Some(p) = p.as_ref() { + msg.push_str(&format!(" as part of `{}`\n", p)); + } else { + msg.push_str("\n"); + } + } + msg.push_str("Add --force to overwrite"); + Err(human(msg)) +} + +fn find_duplicates(dst: &Path, + pkg: &Package, + filter: &ops::CompileFilter, + prev: &CrateListingV1) -> BTreeMap> { let check = |name| { let name = format!("{}{}", name, env::consts::EXE_SUFFIX); if fs::metadata(dst.join(&name)).is_err() { - return Ok(()) - } - let mut msg = format!("binary `{}` already exists in destination", name); - if let Some((p, _)) = prev.v1.iter().find(|&(_, v)| v.contains(&name)) { - msg.push_str(&format!(" as part of `{}`", p)); + None + } else if let Some((p, _)) = prev.v1.iter().find(|&(_, v)| v.contains(&name)) { + Some((name, Some(p.clone()))) + } else { + Some((name, None)) } - Err(human(msg)) }; match *filter { CompileFilter::Everything => { - // If explicit --bin or --example flags were passed then those'll - // get checked during cargo_compile, we only care about the "build - // everything" case here - if pkg.targets().iter().filter(|t| t.is_bin()).next().is_none() { - bail!("specified package has no binaries") - } - - for target in pkg.targets().iter().filter(|t| t.is_bin()) { - try!(check(target.name())); - } + pkg.targets().iter() + .filter(|t| t.is_bin()) + .filter_map(|t| check(t.name())) + .collect() } CompileFilter::Only { bins, examples, .. } => { - for bin in bins.iter().chain(examples) { - try!(check(bin)); - } + bins.iter().chain(examples) + .filter_map(|t| check(t)) + .collect() } } - Ok(()) } fn read_crate_list(mut file: &File) -> CargoResult { diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 9a55c1f53cd..bfaf48ca7b3 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -672,3 +672,4 @@ pub static UPLOADING: &'static str = " Uploading"; pub static VERIFYING: &'static str = " Verifying"; pub static ARCHIVING: &'static str = " Archiving"; pub static INSTALLING: &'static str = " Installing"; +pub static REPLACING: &'static str = " Replacing"; diff --git a/tests/test_cargo_install.rs b/tests/test_cargo_install.rs index cfad68443c8..f32ed786b55 100644 --- a/tests/test_cargo_install.rs +++ b/tests/test_cargo_install.rs @@ -8,7 +8,7 @@ use cargo::util::ProcessBuilder; use hamcrest::{assert_that, existing_file, is_not, Matcher, MatchResult}; use support::{project, execs}; -use support::{UPDATING, DOWNLOADING, COMPILING, INSTALLING, REMOVING, ERROR}; +use support::{UPDATING, DOWNLOADING, COMPILING, INSTALLING, REPLACING, REMOVING, ERROR}; use support::paths; use support::registry::Package; use support::git; @@ -199,6 +199,7 @@ test!(install_path { assert_that(cargo_process("install").arg("--path").arg(".").cwd(p.root()), execs().with_status(101).with_stderr(&format!("\ {error} binary `foo[..]` already exists in destination as part of `foo v0.1.0 [..]` +Add --force to overwrite ", error = ERROR))); }); @@ -389,18 +390,156 @@ test!(install_twice { version = "0.1.0" authors = [] "#) - .file("src/main.rs", "fn main() {}"); + .file("src/bin/foo-bin1.rs", "fn main() {}") + .file("src/bin/foo-bin2.rs", "fn main() {}"); p.build(); assert_that(cargo_process("install").arg("--path").arg(p.root()), execs().with_status(0)); assert_that(cargo_process("install").arg("--path").arg(p.root()), execs().with_status(101).with_stderr(&format!("\ -{error} binary `foo[..]` already exists in destination as part of `foo v0.1.0 ([..])` +{error} binary `foo-bin1[..]` already exists in destination as part of `foo v0.1.0 ([..])` +binary `foo-bin2[..]` already exists in destination as part of `foo v0.1.0 ([..])` +Add --force to overwrite ", error = ERROR))); }); +test!(install_force { + let p = project("foo") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + "#) + .file("src/main.rs", "fn main() {}"); + p.build(); + + assert_that(cargo_process("install").arg("--path").arg(p.root()), + execs().with_status(0)); + + let p = project("foo2") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.2.0" + authors = [] + "#) + .file("src/main.rs", "fn main() {}"); + p.build(); + + assert_that(cargo_process("install").arg("--force").arg("--path").arg(p.root()), + execs().with_status(0).with_stdout(&format!("\ +{compiling} foo v0.2.0 ([..]) +{replacing} {home}[..]bin[..]foo[..] +", + compiling = COMPILING, + replacing = REPLACING, + home = cargo_home().display()))); + + assert_that(cargo_process("install").arg("--list"), + execs().with_status(0).with_stdout("\ +foo v0.2.0 ([..]): + foo[..] +")); +}); + +test!(install_force_partial_overlap { + let p = project("foo") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + "#) + .file("src/bin/foo-bin1.rs", "fn main() {}") + .file("src/bin/foo-bin2.rs", "fn main() {}"); + p.build(); + + assert_that(cargo_process("install").arg("--path").arg(p.root()), + execs().with_status(0)); + + let p = project("foo2") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.2.0" + authors = [] + "#) + .file("src/bin/foo-bin2.rs", "fn main() {}") + .file("src/bin/foo-bin3.rs", "fn main() {}"); + p.build(); + + assert_that(cargo_process("install").arg("--force").arg("--path").arg(p.root()), + execs().with_status(0).with_stdout(&format!("\ +{compiling} foo v0.2.0 ([..]) +{installing} {home}[..]bin[..]foo-bin3[..] +{replacing} {home}[..]bin[..]foo-bin2[..] +", + compiling = COMPILING, + installing = INSTALLING, + replacing = REPLACING, + home = cargo_home().display()))); + + assert_that(cargo_process("install").arg("--list"), + execs().with_status(0).with_stdout("\ +foo v0.1.0 ([..]): + foo-bin1[..] +foo v0.2.0 ([..]): + foo-bin2[..] + foo-bin3[..] +")); +}); + +test!(install_force_bin { + let p = project("foo") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + "#) + .file("src/bin/foo-bin1.rs", "fn main() {}") + .file("src/bin/foo-bin2.rs", "fn main() {}"); + p.build(); + + assert_that(cargo_process("install").arg("--path").arg(p.root()), + execs().with_status(0)); + + let p = project("foo2") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.2.0" + authors = [] + "#) + .file("src/bin/foo-bin1.rs", "fn main() {}") + .file("src/bin/foo-bin2.rs", "fn main() {}"); + p.build(); + + assert_that(cargo_process("install").arg("--force") + .arg("--bin") + .arg("foo-bin2") + .arg("--path") + .arg(p.root()), + execs().with_status(0).with_stdout(&format!("\ +{compiling} foo v0.2.0 ([..]) +{replacing} {home}[..]bin[..]foo-bin2[..] +", + compiling = COMPILING, + replacing = REPLACING, + home = cargo_home().display()))); + + assert_that(cargo_process("install").arg("--list"), + execs().with_status(0).with_stdout("\ +foo v0.1.0 ([..]): + foo-bin1[..] +foo v0.2.0 ([..]): + foo-bin2[..] +")); +}); + test!(compile_failure { let p = project("foo") .file("Cargo.toml", r#" From 9f0fa249b5d307bd97c8c4957a98ad9f47116bf9 Mon Sep 17 00:00:00 2001 From: Gleb Kozyrev Date: Sun, 1 May 2016 02:46:41 +0300 Subject: [PATCH 2/2] cargo-install: move binaries from the build dir if possible Try moving the binaries (and fall back to copying) if the build directory is a temporary one (source isn't a local path). --- src/cargo/ops/cargo_install.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 237f3aba957..d3d894e2378 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -131,6 +131,12 @@ pub fn install(root: Option<&str>, let staging_dir = try!(TempDir::new_in(&dst, "cargo-install")); for &(bin, src) in binaries.iter() { let dst = staging_dir.path().join(bin); + // Try to move if `target_dir` is transient. + if !source_id.is_path() { + if fs::rename(src, &dst).is_ok() { + continue + } + } try!(fs::copy(src, &dst).chain_error(|| { human(format!("failed to copy `{}` to `{}`", src.display(), dst.display()))