diff --git a/Cargo.lock b/Cargo.lock index d675f59d8d9..33e8ef7d7c8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -810,6 +810,7 @@ dependencies = [ "rustup-dist 1.8.0", "rustup-mock 1.8.0", "rustup-utils 1.8.0", + "same-file 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)", "scopeguard 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.23 (registry+https://github.com/rust-lang/crates.io-index)", "serde_derive 1.0.23 (registry+https://github.com/rust-lang/crates.io-index)", @@ -917,6 +918,14 @@ dependencies = [ "winapi 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "same-file" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "winapi 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "schannel" version = "0.1.9" @@ -1394,11 +1403,30 @@ name = "winapi" version = "0.2.8" source = "registry+https://github.com/rust-lang/crates.io-index" +[[package]] +name = "winapi" +version = "0.3.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "winapi-i686-pc-windows-gnu 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)", + "winapi-x86_64-pc-windows-gnu 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "winapi-build" version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" +[[package]] +name = "winapi-i686-pc-windows-gnu" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" + +[[package]] +name = "winapi-x86_64-pc-windows-gnu" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" + [[package]] name = "winreg" version = "0.4.0" @@ -1534,6 +1562,7 @@ dependencies = [ "checksum rustc-serialize 0.3.24 (registry+https://github.com/rust-lang/crates.io-index)" = "dcf128d1287d2ea9d80910b5f1120d0b8eede3fbf1abe91c40d39ea7d51e6fda" "checksum safemem 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "e27a8b19b835f7aea908818e871f5cc3a5a186550c30773be987e155e8163d8f" "checksum same-file 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)" = "d931a44fdaa43b8637009e7632a02adc4f2b2e0733c08caa4cf00e8da4a117a7" +"checksum same-file 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "f3257af0472da4b8b8902102a57bafffd9991f0f43772a8af6153d597e6e4ae2" "checksum schannel 0.1.9 (registry+https://github.com/rust-lang/crates.io-index)" = "4330c2e874379fbd28fa67ba43239dbe8c7fb00662ceb1078bd37474f08bf5ce" "checksum scoped-tls 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "f417c22df063e9450888a7561788e9bd46d3bb3c1466435b4eccb903807f147d" "checksum scopeguard 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)" = "94258f53601af11e6a49f722422f6e3425c52b06245a5cf9bc09908b174f5e27" @@ -1592,7 +1621,10 @@ dependencies = [ "checksum wait-timeout 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)" = "b9f3bf741a801531993db6478b95682117471f76916f5e690dd8d45395b09349" "checksum walkdir 1.0.7 (registry+https://github.com/rust-lang/crates.io-index)" = "bb08f9e670fab86099470b97cd2b252d6527f0b3cc1401acdb595ffc9dd288ff" "checksum winapi 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)" = "167dc9d6949a9b857f3451275e911c3f44255842c1f7a76f33c55103a909087a" +"checksum winapi 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)" = "b09fb3b6f248ea4cd42c9a65113a847d612e17505d6ebd1f7357ad68a8bf8693" "checksum winapi-build 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "2d315eee3b34aca4797b2da6b13ed88266e6d612562a0c46390af8299fc699bc" +"checksum winapi-i686-pc-windows-gnu 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)" = "ec6667f60c23eca65c561e63a13d81b44234c2e38a6b6c959025ee907ec614cc" +"checksum winapi-x86_64-pc-windows-gnu 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)" = "98f12c52b2630cd05d2c3ffd8e008f7f48252c042b4871c72aed9dc733b96668" "checksum winreg 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)" = "cbf285379f20d7f26abd990d9a566be9d31ab7a9d335299baaa1f0604f5f96af" "checksum ws2_32-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)" = "d59cefebd0c892fa2dd6de581e937301d8552cb44489cdff035c6187cb63fa5e" "checksum xattr 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)" = "5f04de8a1346489a2f9e9bd8526b73d135ec554227b17568456e86aa35b6f3fc" diff --git a/Cargo.toml b/Cargo.toml index f1fb8728d0f..5e3431511f1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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" diff --git a/src/rustup-cli/errors.rs b/src/rustup-cli/errors.rs index 6d7a4e95928..f36486e5738 100644 --- a/src/rustup-cli/errors.rs +++ b/src/rustup-cli/errors.rs @@ -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 { diff --git a/src/rustup-cli/main.rs b/src/rustup-cli/main.rs index e0ebd709116..0140d086309 100644 --- a/src/rustup-cli/main.rs +++ b/src/rustup-cli/main.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; diff --git a/src/rustup-cli/self_update.rs b/src/rustup-cli/self_update.rs index 35f7904930c..42b24ef7bde 100644 --- a/src/rustup-cli/self_update.rs +++ b/src/rustup-cli/self_update.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,70 @@ 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 tool_handles = Vec::new(); + let mut link_afterwards = 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)?; + let tool_path = bin_path.join(&format!("{}{}", tool, EXE_SUFFIX)); + if let Ok(handle) = Handle::from_path(&tool_path) { + tool_handles.push(handle); + if rustup == *tool_handles.last().unwrap() { + continue + } } - try!(utils::hard_or_symlink_file(rustup_path, tool_path)); + link_afterwards.push(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 tool_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)); + } + + drop(tool_handles); + for path in link_afterwards { + try!(utils::hard_or_symlink_file(rustup_path, &path)); } Ok(()) diff --git a/src/rustup-mock/src/clitools.rs b/src/rustup-mock/src/clitools.rs index 4dcb70e9c19..a7254b25350 100644 --- a/src/rustup-mock/src/clitools.rs +++ b/src/rustup-mock/src/clitools.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 diff --git a/src/rustup-utils/src/raw.rs b/src/rustup-utils/src/raw.rs index 00ed137f6b3..a4165e130aa 100644 --- a/src/rustup-utils/src/raw.rs +++ b/src/rustup-utils/src/raw.rs @@ -130,7 +130,7 @@ pub fn append_file(dest: &Path, line: &str) -> io::Result<()> { Ok(()) } -pub fn tee_file(path: &Path, mut w: &mut W) -> io::Result<()> { +pub fn tee_file(path: &Path, w: &mut W) -> io::Result<()> { let mut file = try!(fs::OpenOptions::new() .read(true) .open(path)); @@ -213,7 +213,7 @@ fn symlink_junction_inner(target: &Path, junction: &Path) -> io::Result<()> { ptr::null_mut()); let mut data = [0u8; MAXIMUM_REPARSE_DATA_BUFFER_SIZE]; - let mut db = data.as_mut_ptr() + let db = data.as_mut_ptr() as *mut REPARSE_MOUNTPOINT_DATA_BUFFER; let buf = &mut (*db).ReparseTarget as *mut _; let mut i = 0; diff --git a/tests/cli-inst-interactive.rs b/tests/cli-inst-interactive.rs index de1a132fc31..a2379fe5b89 100644 --- a/tests/cli-inst-interactive.rs +++ b/tests/cli-inst-interactive.rs @@ -96,6 +96,8 @@ fn blank_lines_around_stderr_log_output_update() { setup(&|config| { run_input(config, &["rustup-init"], "\n\n"); let out = run_input(config, &["rustup-init"], "\n\n"); + println!("-- stdout --\n {}", out.stdout); + println!("-- stderr --\n {}", out.stderr); assert!(out.stdout.contains(r" 3) Cancel installation diff --git a/tests/cli-self-upd.rs b/tests/cli-self-upd.rs index c8bc9c4b45e..d2bf24f37b3 100644 --- a/tests/cli-self-upd.rs +++ b/tests/cli-self-upd.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); }); }