Auto merge of #2405 - gkoz:reinstall, r=alexcrichton

bors · bors · commit 401209f25866 · 2016-05-02T09:54:56.000-07:00
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 <crate>`. 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.
diff --git a/src/bin/install.rs b/src/bin/install.rs
@@ -15,6 +15,7 @@ pub struct Options {
     flag_color: Option<String>,
     flag_root: Option<String>,
     flag_list: bool,
+    flag_force: bool,
 
     arg_crate: Option<String>,
     flag_vers: Option<String>,
@@ -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 <crate>`.
+
 As a special convenience, omitting the <crate> 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<Option<()>> {
     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
@@ -32,6 +32,12 @@ struct Transaction {
     bins: Vec<PathBuf>,
 }
 
+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,40 +109,122 @@ 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::<CargoResult<_>>());
 
     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::<Vec<_>>();
+    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.
         let target_dir = target_dir.into_path_unlocked();
         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<I, F>(mut i: I, f: F) -> CargoResult<Option<I::Item>>
 fn check_overwrites(dst: &Path,
                     pkg: &Package,
                     filter: &ops::CompileFilter,
-                    prev: &CrateListingV1) -> CargoResult<()> {
+                    prev: &CrateListingV1,
+                    force: bool) -> CargoResult<BTreeMap<String, Option<PackageId>>> {
+    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<String, Option<PackageId>> {
     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<CrateListingV1> {
diff --git 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
