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..d3d894e2378 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,115 @@ 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 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())) + })); + } + + 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 +225,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 +314,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#"