Auto merge of #1326 - alexcrichton:fix-errors, r=alexcrichton

Fix self update errors filling in missing proxies

The previous logic had some subtle bugs for a number of reasons, and hopefully
this iteration irons them out.

Closes #1316

Author: bors

Cargo.lock

@@ -42,6 +42,7 @@ markdown = "0.2"
 rand = "0.3.11"
 regex = "0.2"
 remove_dir_all = "0.2.0"
+same-file = "1.0"
 scopeguard = "0.3"
 serde = "1.0"
 serde_derive = "1.0"

Cargo.toml

@@ -1,5 +1,6 @@
 #![allow(dead_code)]
 
+use std::io;
 use std::path::PathBuf;
 
 use rustup;
@@ -15,6 +16,7 @@ error_chain! {
 
     foreign_links {
         Temp(temp::Error);
+        Io(io::Error);
     }
 
     errors {

src/rustup-cli/errors.rs

@@ -26,6 +26,7 @@ extern crate term;
 extern crate itertools;
 extern crate time;
 extern crate rand;
+extern crate same_file;
 extern crate scopeguard;
 extern crate tempdir;
 extern crate sha2;

src/rustup-cli/main.rs

@@ -34,6 +34,7 @@ use common::{self, Confirm};
 use errors::*;
 use rustup_dist::dist;
 use rustup_utils::utils;
+use same_file::Handle;
 use std::env;
 use std::env::consts::EXE_SUFFIX;
 use std::path::{Path, PathBuf, Component};
@@ -657,29 +658,63 @@ pub fn install_proxies() -> Result<()> {
     let ref bin_path = try!(utils::cargo_home()).join("bin");
     let ref rustup_path = bin_path.join(&format!("rustup{}", EXE_SUFFIX));
 
-    // Record the size of the known links, then when we get files which may or
-    // may not be links, we compare their size. Same size means probably a link.
-    let mut file_size = 0;
+    let rustup = Handle::from_path(rustup_path)?;
+    let mut prev_handles = Vec::new();
 
     // Try to hardlink all the Rust exes to the rustup exe. Some systems,
     // like Android, does not support hardlinks, so we fallback to symlinks.
+    //
+    // Note that this function may not be running in the context of a fresh
+    // self update but rather as part of a normal update to fill in missing
+    // proxies. In that case our process may actually have the `rustup.exe`
+    // file open, and on systems like Windows that means that you can't
+    // even remove other hard links to the same file. Basically if we have
+    // `rustup.exe` open and running and `cargo.exe` is a hard link to that
+    // file, we can't remove `cargo.exe`.
+    //
+    // To avoid unnecessary errors from being returned here we use the
+    // `same-file` crate and its `Handle` type to avoid clobbering hard links
+    // that are already valid. If a hard link already points to the
+    // `rustup.exe` file then we leave it alone and move to the next one.
     for tool in TOOLS {
         let ref tool_path = bin_path.join(&format!("{}{}", tool, EXE_SUFFIX));
-        if tool_path.exists() {
-            file_size = utils::file_size(tool_path)?;
+        if let Ok(handle) = Handle::from_path(tool_path) {
+            prev_handles.push(handle);
+            if rustup == *prev_handles.last().unwrap() {
+                continue
+            }
         }
         try!(utils::hard_or_symlink_file(rustup_path, tool_path));
     }
 
     for tool in DUP_TOOLS {
         let ref tool_path = bin_path.join(&format!("{}{}", tool, EXE_SUFFIX));
-        if tool_path.exists() && (file_size == 0 || utils::file_size(tool_path)? != file_size) {
-            warn!("tool `{}` is already installed, remove it from `{}`, then run `rustup update` \
-                   to have rustup manage this tool.",
-                  tool, bin_path.to_string_lossy());
-        } else {
-            try!(utils::hard_or_symlink_file(rustup_path, tool_path));
+        if let Ok(handle) = Handle::from_path(tool_path) {
+            // Like above, don't clobber anything that's already hardlinked to
+            // avoid extraneous errors from being returned.
+            if rustup == handle {
+                continue
+            }
+
+            // If this file exists and is *not* equivalent to all other
+            // preexisting tools we found, then we're going to assume that it
+            // was preinstalled and actually pointing to a totally different
+            // binary. This is intended for cases where historically users
+            // rand `cargo install rustfmt` and so they had custom `rustfmt`
+            // and `cargo-fmt` executables lying around, but we as rustup have
+            // since started managing these tools.
+            //
+            // If the file is managed by rustup it should be equivalent to some
+            // previous file, and if it's not equivalent to anything then it's
+            // pretty likely that it needs to be dealt with manually.
+            if prev_handles.iter().all(|h| *h != handle) {
+                warn!("tool `{}` is already installed, remove it from `{}`, then run `rustup update` \
+                       to have rustup manage this tool.",
+                      tool, bin_path.to_string_lossy());
+                continue
+            }
         }
+        try!(utils::hard_or_symlink_file(rustup_path, tool_path));
     }
 
     Ok(())

src/rustup-cli/self_update.rs

@@ -322,13 +322,19 @@ pub fn run(config: &Config, name: &str, args: &[&str], env: &[(&str, &str)]) ->
     for env in env {
         cmd.env(env.0, env.1);
     }
+
+    println!("running {:?}", cmd);
     let out = cmd.output().expect("failed to run test command");
 
-    SanitizedOutput {
+    let output = SanitizedOutput {
         ok: out.status.success(),
         stdout: String::from_utf8(out.stdout).unwrap(),
         stderr: String::from_utf8(out.stderr).unwrap(),
-    }
+    };
+    println!("status: {}", out.status);
+    println!("----- stdout\n{}", output.stdout);
+    println!("----- stderr\n{}", output.stderr);
+    return output
 }
 
 // Creates a mock dist server populated with some test data

src/rustup-mock/src/clitools.rs

@@ -1250,28 +1250,36 @@ fn rls_proxy_set_up_after_update() {
 
 #[test]
 fn update_does_not_overwrite_rustfmt() {
-    update_setup(&|config, _| {
+    update_setup(&|config, self_dist| {
         expect_ok(config, &["rustup-init", "-y"]);
+        let version = env!("CARGO_PKG_VERSION");
+        output_release_file(self_dist, "1", version);
+
+        // Since we just did a fresh install rustfmt will exist. Let's emulate
+        // it not existing in this test though by removing it just after our
+        // installation.
         let ref rustfmt_path = config.cargodir.join(format!("bin/rustfmt{}", EXE_SUFFIX));
+        assert!(rustfmt_path.exists());
+        fs::remove_file(rustfmt_path).unwrap();
         raw::write_file(rustfmt_path, "").unwrap();
         assert_eq!(utils::file_size(rustfmt_path).unwrap(), 0);
 
+        // Ok, now a self-update should complain about `rustfmt` not looking
+        // like rustup and the user should take some action.
         expect_stderr_ok(config, &["rustup", "self", "update"],
                          "`rustfmt` is already installed");
-        expect_ok(config, &["rustup", "self", "update"]);
         assert!(rustfmt_path.exists());
         assert_eq!(utils::file_size(rustfmt_path).unwrap(), 0);
 
-
-        // We run the test twice because the first time around none of the shims
-        // exist, and we want to check that out check for rustfmt works if there
-        // are shims or not.
-        let ref rustdoc_path = config.cargodir.join(format!("bin/rustdoc{}", EXE_SUFFIX));
-        assert!(rustdoc_path.exists());
-
-        expect_stderr_ok(config, &["rustup", "self", "update"],
-                         "`rustfmt` is already installed");
+        // Now simluate us removing the rustfmt executable and rerunning a self
+        // update, this should install the rustup shim. Note that we don't run
+        // `rustup` here but rather the rustup we've actually installed, this'll
+        // help reproduce bugs related to having that file being opened by the
+        // current process.
+        fs::remove_file(rustfmt_path).unwrap();
+        let installed_rustup = config.cargodir.join("bin/rustup");
+        expect_ok(config, &[installed_rustup.to_str().unwrap(), "self", "update"]);
         assert!(rustfmt_path.exists());
-        assert_eq!(utils::file_size(rustfmt_path).unwrap(), 0);
+        assert!(utils::file_size(rustfmt_path).unwrap() > 0);
     });
 }