Auto merge of #1294 - rust-lang-nursery:rustfmt, r=alexcrichton

Add rustfmt to the tools list

r? @alexcrichton

We should probably add something for cargo-fmt too once rust-lang/rust#46031 lands, but I'm not sure if we can just add it to the list or not, seeing as it is not its own component.

Author: bors

src/rustup-cli/self_update.rs

@@ -184,14 +184,19 @@ Studio 2013 and during install select the "C++ tools":
 
 _Install the C++ build tools before proceeding_.
 
-If you will be targetting the GNU ABI or otherwise know what you are
+If you will be targeting the GNU ABI or otherwise know what you are
 doing then it is fine to continue installation without the build
 tools, but otherwise, install the C++ build tools before proceeding.
 "#;
 
 static TOOLS: &'static [&'static str]
     = &["rustc", "rustdoc", "cargo", "rust-lldb", "rust-gdb", "rls"];
 
+// Tools which are commonly installed by Cargo as well as rustup. We take a bit
+// more care with these to ensure we don't overwrite the user's previous
+// installation.
+static DUP_TOOLS: &'static [&'static str] = &["rustfmt", "cargo-fmt"];
+
 static UPDATE_ROOT: &'static str
     = "https://static.rust-lang.org/rustup";
 
@@ -646,13 +651,31 @@ fn install_bins() -> Result<()> {
     try!(utils::copy_file(this_exe_path, rustup_path));
     try!(utils::make_executable(rustup_path));
 
+    // 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;
+
     // Try to hardlink all the Rust exes to the rustup exe. Some systems,
     // like Android, does not support hardlinks, so we fallback to symlinks.
     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)?;
+        }
         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));
+        }
+    }
+
     Ok(())
 }
 
@@ -752,7 +775,7 @@ pub fn uninstall(no_prompt: bool) -> Result<()> {
 
     // Then everything in bin except rustup and tools. These can't be unlinked
     // until this process exits (on windows).
-    let tools = TOOLS.iter().map(|t| format!("{}{}", t, EXE_SUFFIX));
+    let tools = TOOLS.iter().chain(DUP_TOOLS.iter()).map(|t| format!("{}{}", t, EXE_SUFFIX));
     let tools: Vec<_> = tools.chain(vec![format!("rustup{}", EXE_SUFFIX)]).collect();
     for dirent in try!(fs::read_dir(&cargo_home.join("bin")).chain_err(|| read_dir_err)) {
         let dirent = try!(dirent.chain_err(|| read_dir_err));

src/rustup-utils/src/utils.rs

@@ -383,6 +383,16 @@ pub fn set_permissions(path: &Path, perms: fs::Permissions) -> Result<()> {
     })
 }
 
+pub fn file_size(path: &Path) -> Result<u64> {
+    let metadata = fs::metadata(path).chain_err(|| {
+        ErrorKind::ReadingFile {
+            name: "metadata for",
+            path: PathBuf::from(path),
+        }
+    })?;
+    Ok(metadata.len())
+}
+
 pub fn make_executable(path: &Path) -> Result<()> {
     #[cfg(windows)]
     fn inner(_: &Path) -> Result<()> {

src/rustup-win-installer/src/lib.rs

@@ -16,7 +16,7 @@ pub const LOGMSG_STANDARD: i32 = 2;
 
 // TODO: share this with self_update.rs
 static TOOLS: &'static [&'static str]
-    = &["rustc", "rustdoc", "cargo", "rust-lldb", "rust-gdb", "rls"];
+    = &["rustc", "rustdoc", "cargo", "rust-lldb", "rust-gdb", "rls", "rustfmt", "cargo-fmt"];
 
 #[no_mangle]
 /// This is be run as a `deferred` action after `InstallFiles` on install and upgrade

tests/cli-self-upd.rs

@@ -27,16 +27,13 @@ use std::process::Command;
 use remove_dir_all::remove_dir_all;
 use rustup_mock::clitools::{self, Config, Scenario,
                                expect_ok, expect_ok_ex,
-                               expect_stdout_ok,
+                               expect_stdout_ok, expect_stderr_ok,
                                expect_err, expect_err_ex,
                                this_host_triple};
 use rustup_mock::dist::{calc_hash};
 use rustup_mock::{get_path, restore_path};
 use rustup_utils::{utils, raw};
 
-#[cfg(windows)]
-use rustup_mock::clitools::expect_stderr_ok;
-
 macro_rules! for_host { ($s: expr) => (&format!($s, this_host_triple())) }
 
 const TEST_VERSION: &'static str = "1.1.1";
@@ -49,7 +46,7 @@ pub fn setup(f: &Fn(&Config)) {
         }
         let _g = LOCK.lock();
 
-        // An windows these tests mess with the user's PATH. Save
+        // On windows these tests mess with the user's PATH. Save
         // and restore them here to keep from trashing things.
         let saved_path = get_path();
         let _g = scopeguard::guard(saved_path, |p| restore_path(p));
@@ -1249,3 +1246,32 @@ fn rls_proxy_set_up_after_update() {
         assert!(rls_path.exists());
     });
 }
+
+
+#[test]
+fn update_does_not_overwrite_rustfmt() {
+    update_setup(&|config, _| {
+        expect_ok(config, &["rustup-init", "-y"]);
+        let ref rustfmt_path = config.cargodir.join(format!("bin/rustfmt{}", EXE_SUFFIX));
+        raw::write_file(rustfmt_path, "").unwrap();
+        assert_eq!(utils::file_size(rustfmt_path).unwrap(), 0);
+
+        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");
+        assert!(rustfmt_path.exists());
+        assert_eq!(utils::file_size(rustfmt_path).unwrap(), 0);
+    });
+}