Skip to content

Commit

Permalink
Fix self update errors filling in missing proxies
Browse files Browse the repository at this point in the history
The previous logic had some subtle bugs for a number of reasons, and hopefully
this iteration irons them out.

Closes rust-lang#1316
  • Loading branch information
alexcrichton committed Jan 4, 2018
1 parent 17aa5b5 commit 98acdbb
Show file tree
Hide file tree
Showing 9 changed files with 119 additions and 28 deletions.
32 changes: 32 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 2 additions & 0 deletions src/rustup-cli/errors.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#![allow(dead_code)]

use std::io;
use std::path::PathBuf;

use rustup;
Expand All @@ -15,6 +16,7 @@ error_chain! {

foreign_links {
Temp(temp::Error);
Io(io::Error);
}

errors {
Expand Down
1 change: 1 addition & 0 deletions src/rustup-cli/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
63 changes: 51 additions & 12 deletions src/rustup-cli/self_update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -657,29 +658,67 @@ 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));
println!("remove {:?}", utils::remove_file("wut", tool_path));
assert!(!tool_path.exists());
try!(utils::hardlink_file(rustup_path, tool_path).chain_error(|| {
"wut"
}));
}

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(())
Expand Down
10 changes: 8 additions & 2 deletions src/rustup-mock/src/clitools.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/rustup-utils/src/raw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ pub fn append_file(dest: &Path, line: &str) -> io::Result<()> {
Ok(())
}

pub fn tee_file<W: io::Write>(path: &Path, mut w: &mut W) -> io::Result<()> {
pub fn tee_file<W: io::Write>(path: &Path, w: &mut W) -> io::Result<()> {
let mut file = try!(fs::OpenOptions::new()
.read(true)
.open(path));
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions tests/cli-inst-interactive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
32 changes: 20 additions & 12 deletions tests/cli-self-upd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}

0 comments on commit 98acdbb

Please sign in to comment.