From 61926e6506c23f49c443f0e999be159e69b66740 Mon Sep 17 00:00:00 2001 From: Brian Anderson Date: Thu, 23 Feb 2017 12:32:30 -0800 Subject: [PATCH 1/4] Add the rls bin to installation --- src/rustup-cli/self_update.rs | 2 +- src/rustup-mock/src/clitools.rs | 21 ++++++++++++++++++++ tests/cli-misc.rs | 23 ++++++++++++++++++++++ tests/cli-self-upd.rs | 34 +++++++++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 1 deletion(-) diff --git a/src/rustup-cli/self_update.rs b/src/rustup-cli/self_update.rs index 041e0e6871f..12d4e3519d9 100644 --- a/src/rustup-cli/self_update.rs +++ b/src/rustup-cli/self_update.rs @@ -191,7 +191,7 @@ tools, but otherwise, install the C++ build tools before proceeding. "#; static TOOLS: &'static [&'static str] - = &["rustc", "rustdoc", "cargo", "rust-lldb", "rust-gdb"]; + = &["rustc", "rustdoc", "cargo", "rust-lldb", "rust-gdb", "rls"]; static UPDATE_ROOT: &'static str = "https://static.rust-lang.org/rustup"; diff --git a/src/rustup-mock/src/clitools.rs b/src/rustup-mock/src/clitools.rs index 6e6b5d53247..fc0f9b723aa 100644 --- a/src/rustup-mock/src/clitools.rs +++ b/src/rustup-mock/src/clitools.rs @@ -99,6 +99,7 @@ pub fn setup(s: Scenario, f: &Fn(&Config)) { let setup_path = config.exedir.join(format!("rustup-init{}", EXE_SUFFIX)); let rustc_path = config.exedir.join(format!("rustc{}", EXE_SUFFIX)); let cargo_path = config.exedir.join(format!("cargo{}", EXE_SUFFIX)); + let rls_path = config.exedir.join(format!("rls{}", EXE_SUFFIX)); // Don't copy an executable via `fs::copy` on Unix because that'll require // opening up the destination for writing. If one thread in our process then @@ -119,6 +120,7 @@ pub fn setup(s: Scenario, f: &Fn(&Config)) { fs::hard_link(rustup_path, setup_path).unwrap(); fs::hard_link(rustup_path, rustc_path).unwrap(); fs::hard_link(rustup_path, cargo_path).unwrap(); + fs::hard_link(rustup_path, rls_path).unwrap(); // Make sure the host triple matches the build triple. Otherwise testing a 32-bit build of // rustup on a 64-bit machine will fail, because the tests do not have the host detection @@ -365,6 +367,7 @@ fn build_mock_channel(s: Scenario, channel: &str, date: &str, let std = build_mock_std_installer(host_triple); let rustc = build_mock_rustc_installer(host_triple, version, version_hash); let cargo = build_mock_cargo_installer(version, version_hash); + let rls = build_mock_rls_installer(version, version_hash); let rust_docs = build_mock_rust_doc_installer(); let rust = build_combined_installer(&[&std, &rustc, &cargo, &rust_docs]); let cross_std1 = build_mock_cross_std_installer(CROSS_ARCH1, date); @@ -378,6 +381,7 @@ fn build_mock_channel(s: Scenario, channel: &str, date: &str, (cross_std2, CROSS_ARCH2.to_string())]), ("rustc", vec![(rustc, host_triple.clone())]), ("cargo", vec![(cargo, host_triple.clone())]), + ("rls", vec![(rls, host_triple.clone())]), ("rust-docs", vec![(rust_docs, host_triple.clone())]), ("rust-src", vec![(rust_src, "*".to_string())]), ("rust", vec![(rust, host_triple.clone())])]; @@ -386,6 +390,7 @@ fn build_mock_channel(s: Scenario, channel: &str, date: &str, let std = build_mock_std_installer(MULTI_ARCH1); let rustc = build_mock_rustc_installer(MULTI_ARCH1, version, version_hash); let cargo = build_mock_cargo_installer(version, version_hash); + let rls = build_mock_rls_installer(version, version_hash); let rust_docs = build_mock_rust_doc_installer(); let rust = build_combined_installer(&[&std, &rustc, &cargo, &rust_docs]); let cross_std1 = build_mock_cross_std_installer(CROSS_ARCH1, date); @@ -398,6 +403,7 @@ fn build_mock_channel(s: Scenario, channel: &str, date: &str, (cross_std2, CROSS_ARCH2.to_string())]), ("rustc", vec![(rustc, triple.clone())]), ("cargo", vec![(cargo, triple.clone())]), + ("rls", vec![(rls, host_triple.clone())]), ("rust-docs", vec![(rust_docs, triple.clone())]), ("rust-src", vec![(rust_src, "*".to_string())]), ("rust", vec![(rust, triple.clone())])]; @@ -445,6 +451,10 @@ fn build_mock_channel(s: Scenario, channel: &str, date: &str, name: "rust-docs".to_string(), target: target.to_string() }); + target_pkg.extensions.push(MockComponent { + name: "rls".to_string(), + target: target.to_string() + }); target_pkg.extensions.push(MockComponent { name: "rust-std".to_string(), target: CROSS_ARCH1.to_string(), @@ -544,6 +554,17 @@ fn build_mock_cargo_installer(version: &str, version_hash: &str) -> MockInstalle } } +fn build_mock_rls_installer(version: &str, version_hash: &str) -> MockInstallerBuilder { + let cargo = format!("bin/rls{}", EXE_SUFFIX); + MockInstallerBuilder { + components: vec![ + ("rls".to_string(), + vec![MockCommand::File(cargo.clone())], + vec![(cargo, mock_bin("rls", version, version_hash))]) + ] + } +} + fn build_mock_rust_doc_installer() -> MockInstallerBuilder { MockInstallerBuilder { components: vec![ diff --git a/tests/cli-misc.rs b/tests/cli-misc.rs index ed164965f47..00bbeeb302e 100644 --- a/tests/cli-misc.rs +++ b/tests/cli-misc.rs @@ -15,6 +15,7 @@ use time::Duration; use std::ops::Sub; use std::ops::Add; use std::time::Duration as StdDuration; +use std::env::consts::EXE_SUFFIX; macro_rules! for_host { ($s: expr) => (&format!($s, this_host_triple())) } @@ -405,3 +406,25 @@ fn telemetry_cleanup_removes_old_files() { assert!(count == 100); }); } + +#[test] +fn rls_exists_in_toolchain() { + setup(&|config| { + expect_ok(config, &["rustup", "default", "stable"]); + expect_ok(config, &["rustup", "component", "add", "rls"]); + assert!(config.exedir.join(format!("rls{}", EXE_SUFFIX)).exists()); + expect_ok(config, &["rls", "--version"]); + }); +} + +#[test] +fn rls_does_not_exist_in_toolchain() { + setup(&|config| { + // FIXME: If rls exists in the toolchain, this should suggest a command + // to run to install it + expect_ok(config, &["rustup", "default", "stable"]); + expect_err(config, &["rls", "--version"], + &format!("toolchain 'stable-{}' does not have the binary `rls{}`", + this_host_triple(), EXE_SUFFIX)); + }); +} diff --git a/tests/cli-self-upd.rs b/tests/cli-self-upd.rs index 1db5a55ac3d..4d0dbc2881f 100644 --- a/tests/cli-self-upd.rs +++ b/tests/cli-self-upd.rs @@ -1158,3 +1158,37 @@ fn uninstall_removes_legacy_home_symlink() { assert!(!multirust_dir.exists()); }); } + +#[test] +fn rls_proxy_set_up_after_install() { + setup(&|config| { + expect_ok(config, &["rustup-init", "-y"]); + expect_err(config, &["rls", "--version"], + &format!("toolchain 'stable-{}' does not have the binary `rls{}`", + this_host_triple(), EXE_SUFFIX)); + expect_ok(config, &["rustup", "component", "add", "rls"]); + expect_ok(config, &["rls", "--version"]); + }); +} + +#[test] +fn rls_proxy_set_up_after_upgrade() { + update_setup(&|config, _| { + expect_ok(config, &["rustup-init", "-y"]); + expect_ok(config, &["rustup", "component", "add", "rls"]); + // Delete the rls proxy to simulate an upgrade from an older rustup that + // didn't include it + let rls_bin = config.cargodir.join(format!("bin/rls{}", EXE_SUFFIX)); + fs::remove_file(rls_bin).expect("rls_bin"); + // Hm, delete this one in exedir too... + // FIXME Shouldn't be necessary + let rls_proxy = config.exedir.join(format!("rls{}", EXE_SUFFIX)); + fs::remove_file(rls_proxy).expect("rls_proxy"); + // RLS doesn't work now + let mut cmd = clitools::cmd(config, "rls", &["--version"]); + assert!(cmd.output().is_err()); + expect_ok(config, &["rustup", "self", "update"]); + // And now it does + expect_ok(config, &["rls", "--version"]); + }); +} From e937b108c7bacf35a08bf86e368db5021999b466 Mon Sep 17 00:00:00 2001 From: Brian Anderson Date: Fri, 24 Feb 2017 12:16:26 -0800 Subject: [PATCH 2/4] Fix the self-update tests to use the correct version of the bins I realized that, even in the installer tests, most of the binaries being run are out of the clitest 'exedir', not from CARGO_HOME/bin. So this patch fixes that by modifying the generic test runner to run bins off of the PATH, with PATH configured by default prefixed with 'exedir'. The cli-self-upd tests extend PATH with CARGO_HOME/bin. --- src/rustup-cli/main.rs | 15 +++++++ src/rustup-cli/self_update.rs | 4 +- src/rustup-mock/src/clitools.rs | 5 +-- src/rustup-utils/src/utils.rs | 7 ++++ tests/cli-self-upd.rs | 74 +++++++++++++++++++++++++++++---- 5 files changed, 91 insertions(+), 14 deletions(-) diff --git a/src/rustup-cli/main.rs b/src/rustup-cli/main.rs index bb34be8aaa4..2e82a2463b5 100644 --- a/src/rustup-cli/main.rs +++ b/src/rustup-cli/main.rs @@ -126,6 +126,7 @@ fn do_compatibility_hacks() { make_environment_compatible(); fix_windows_reg_key(); delete_multirust_bin(); + add_rls_proxy(); } // Convert any MULTIRUST_ env vars to RUSTUP_ and warn about them @@ -187,3 +188,17 @@ fn delete_multirust_bin() { } } } + +// RLS was introduced in an upgrade. Make sure the proxy exists. +fn add_rls_proxy() { + use rustup_utils::utils; + use std::env::consts::EXE_SUFFIX; + + if let Ok(home) = utils::cargo_home() { + let ref rustup_bin = home.join(format!("bin/rustup{}", EXE_SUFFIX)); + let ref rls_bin = home.join(format!("bin/rls{}", EXE_SUFFIX)); + if rustup_bin.exists() && !rls_bin.exists() { + let _ = utils::hard_or_symlink_file(rustup_bin, rls_bin); + } + } +} diff --git a/src/rustup-cli/self_update.rs b/src/rustup-cli/self_update.rs index 12d4e3519d9..4553bfbd4d3 100644 --- a/src/rustup-cli/self_update.rs +++ b/src/rustup-cli/self_update.rs @@ -594,9 +594,7 @@ fn install_bins() -> Result<()> { // 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 utils::hardlink_file(rustup_path, tool_path).is_err() { - try!(utils::symlink_file(rustup_path, tool_path)) - } + try!(utils::hard_or_symlink_file(rustup_path, tool_path)); } Ok(()) diff --git a/src/rustup-mock/src/clitools.rs b/src/rustup-mock/src/clitools.rs index fc0f9b723aa..e148f03066d 100644 --- a/src/rustup-mock/src/clitools.rs +++ b/src/rustup-mock/src/clitools.rs @@ -255,8 +255,7 @@ pub struct SanitizedOutput { } pub fn cmd(config: &Config, name: &str, args: &[&str]) -> Command { - let exe_path = config.exedir.join(format!("{}{}", name, EXE_SUFFIX)); - let mut cmd = Command::new(exe_path); + let mut cmd = Command::new(name); cmd.args(args); env(config, &mut cmd); cmd @@ -292,7 +291,7 @@ pub fn run(config: &Config, name: &str, args: &[&str], env: &[(&str, &str)]) -> for env in env { cmd.env(env.0, env.1); } - let out = cmd.output().unwrap(); + let out = cmd.output().expect("failed to run test command"); SanitizedOutput { ok: out.status.success(), diff --git a/src/rustup-utils/src/utils.rs b/src/rustup-utils/src/utils.rs index b505b726897..cefec8ef514 100644 --- a/src/rustup-utils/src/utils.rs +++ b/src/rustup-utils/src/utils.rs @@ -263,6 +263,13 @@ pub fn symlink_dir(src: &Path, dest: &Path, notify_handler: &Fn(Notification)) - }) } +pub fn hard_or_symlink_file(src: &Path, dest: &Path) -> Result<()> { + if hardlink_file(src, dest).is_err() { + symlink_file(src, dest)?; + } + Ok(()) +} + pub fn hardlink_file(src: &Path, dest: &Path) -> Result<()> { raw::hardlink(src, dest).chain_err(|| { ErrorKind::LinkingFile { diff --git a/tests/cli-self-upd.rs b/tests/cli-self-upd.rs index 4d0dbc2881f..a65d4485c61 100644 --- a/tests/cli-self-upd.rs +++ b/tests/cli-self-upd.rs @@ -47,6 +47,31 @@ pub fn setup(f: &Fn(&Config)) { } let _g = LOCK.lock(); + // For these tests we are running the bins out of ~/.cargo/bin, + // as during a real install, not the test 'exedir' that most + // of the test suite uses. We still need rustup-init though + // to begin the installation, so delete every bin except that. + for entry in fs::read_dir(&config.exedir).unwrap() { + let entry = entry.unwrap(); + // ffs Path/OsStr... + let is_rustup_init = entry.path() + .file_name().unwrap_or_default() + .to_string_lossy().contains("rustup-init"); + if is_rustup_init { + continue; + } + fs::remove_file(entry.path()).unwrap(); + } + + // Likewise set up PATH to include ~/.cargo/bin + let prev_path = env::var_os("PATH"); + let mut new_path = config.cargodir.join("bin").into_os_string(); + if let Some(ref p) = prev_path { + new_path.push(if cfg!(windows) { ";" } else { ":" }); + new_path.push(p); + } + env::set_var("PATH", new_path); + // An windows these tests mess with the user's PATH. Save // and restore them here to keep from trashing things. let saved_path = get_path(); @@ -1171,24 +1196,57 @@ fn rls_proxy_set_up_after_install() { }); } +// This is testing that, when the rls proxy doesn't exist, rustup proxies will +// create it lazily. This has to happen because rustup's self-updates aren't +// able to execute arbitrary code at update time. #[test] -fn rls_proxy_set_up_after_upgrade() { +fn rls_proxy_set_up_lazily_after_upgrade_via_proxy() { update_setup(&|config, _| { expect_ok(config, &["rustup-init", "-y"]); expect_ok(config, &["rustup", "component", "add", "rls"]); + // Delete the rls proxy to simulate an upgrade from an older rustup that // didn't include it - let rls_bin = config.cargodir.join(format!("bin/rls{}", EXE_SUFFIX)); + let ref rls_bin = config.cargodir.join(format!("bin/rls{}", EXE_SUFFIX)); fs::remove_file(rls_bin).expect("rls_bin"); - // Hm, delete this one in exedir too... - // FIXME Shouldn't be necessary - let rls_proxy = config.exedir.join(format!("rls{}", EXE_SUFFIX)); - fs::remove_file(rls_proxy).expect("rls_proxy"); + // RLS doesn't work now + assert!(!rls_bin.exists()); let mut cmd = clitools::cmd(config, "rls", &["--version"]); assert!(cmd.output().is_err()); - expect_ok(config, &["rustup", "self", "update"]); - // And now it does + + // Run a proxy to trigger a lazy upgrade + expect_ok(config, &["rustc", "--version"]); + + // Now RLS works + assert!(rls_bin.exists()); + expect_ok(config, &["rls", "--version"]); + }); +} + +// Same as above but instead of the proxies triggering the update, it's rustup +// directly +#[test] +fn rls_proxy_set_up_lazily_after_upgrade_via_rustup() { + update_setup(&|config, _| { + expect_ok(config, &["rustup-init", "-y"]); + expect_ok(config, &["rustup", "component", "add", "rls"]); + + // Delete the rls proxy to simulate an upgrade from an older rustup that + // didn't include it + let ref rls_bin = config.cargodir.join(format!("bin/rls{}", EXE_SUFFIX)); + fs::remove_file(rls_bin).expect("rls_bin"); + + // RLS doesn't work now + assert!(!rls_bin.exists()); + let mut cmd = clitools::cmd(config, "rls", &["--version"]); + assert!(cmd.output().is_err()); + + // Run rustup to trigger the update + expect_ok(config, &["rustup", "component", "add", "rls"]); + + // Now RLS works + assert!(rls_bin.exists()); expect_ok(config, &["rls", "--version"]); }); } From 19ec923fe476751e4d96c10f13af583ca6f77183 Mon Sep 17 00:00:00 2001 From: Brian Anderson Date: Thu, 23 Mar 2017 01:09:18 +0000 Subject: [PATCH 3/4] Fix test suite bug --- src/rustup-mock/src/clitools.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rustup-mock/src/clitools.rs b/src/rustup-mock/src/clitools.rs index e148f03066d..c5d34991dc8 100644 --- a/src/rustup-mock/src/clitools.rs +++ b/src/rustup-mock/src/clitools.rs @@ -402,7 +402,7 @@ fn build_mock_channel(s: Scenario, channel: &str, date: &str, (cross_std2, CROSS_ARCH2.to_string())]), ("rustc", vec![(rustc, triple.clone())]), ("cargo", vec![(cargo, triple.clone())]), - ("rls", vec![(rls, host_triple.clone())]), + ("rls", vec![(rls, triple.clone())]), ("rust-docs", vec![(rust_docs, triple.clone())]), ("rust-src", vec![(rust_src, "*".to_string())]), ("rust", vec![(rust, triple.clone())])]; From 1c9f146ee0ed877c65ef4451a6f2be40bc0661a0 Mon Sep 17 00:00:00 2001 From: Brian Anderson Date: Mon, 27 Mar 2017 02:04:34 +0000 Subject: [PATCH 4/4] Roll back test suite changes, roll back lazy rls upgrade The lazy upgrade isn't needed, per Diggsey. The test suite changes are too complex to deal with right now, and introduce the risk of the environment interfering with the tests (which is why the tests are all red on the bots but green locally). --- src/rustup-cli/main.rs | 15 ------- src/rustup-mock/src/clitools.rs | 3 +- tests/cli-self-upd.rs | 79 +++------------------------------ 3 files changed, 7 insertions(+), 90 deletions(-) diff --git a/src/rustup-cli/main.rs b/src/rustup-cli/main.rs index 2e82a2463b5..bb34be8aaa4 100644 --- a/src/rustup-cli/main.rs +++ b/src/rustup-cli/main.rs @@ -126,7 +126,6 @@ fn do_compatibility_hacks() { make_environment_compatible(); fix_windows_reg_key(); delete_multirust_bin(); - add_rls_proxy(); } // Convert any MULTIRUST_ env vars to RUSTUP_ and warn about them @@ -188,17 +187,3 @@ fn delete_multirust_bin() { } } } - -// RLS was introduced in an upgrade. Make sure the proxy exists. -fn add_rls_proxy() { - use rustup_utils::utils; - use std::env::consts::EXE_SUFFIX; - - if let Ok(home) = utils::cargo_home() { - let ref rustup_bin = home.join(format!("bin/rustup{}", EXE_SUFFIX)); - let ref rls_bin = home.join(format!("bin/rls{}", EXE_SUFFIX)); - if rustup_bin.exists() && !rls_bin.exists() { - let _ = utils::hard_or_symlink_file(rustup_bin, rls_bin); - } - } -} diff --git a/src/rustup-mock/src/clitools.rs b/src/rustup-mock/src/clitools.rs index c5d34991dc8..02a1cd60cf1 100644 --- a/src/rustup-mock/src/clitools.rs +++ b/src/rustup-mock/src/clitools.rs @@ -255,7 +255,8 @@ pub struct SanitizedOutput { } pub fn cmd(config: &Config, name: &str, args: &[&str]) -> Command { - let mut cmd = Command::new(name); + let exe_path = config.exedir.join(format!("{}{}", name, EXE_SUFFIX)); + let mut cmd = Command::new(exe_path); cmd.args(args); env(config, &mut cmd); cmd diff --git a/tests/cli-self-upd.rs b/tests/cli-self-upd.rs index a65d4485c61..af76ab6bcd6 100644 --- a/tests/cli-self-upd.rs +++ b/tests/cli-self-upd.rs @@ -47,31 +47,6 @@ pub fn setup(f: &Fn(&Config)) { } let _g = LOCK.lock(); - // For these tests we are running the bins out of ~/.cargo/bin, - // as during a real install, not the test 'exedir' that most - // of the test suite uses. We still need rustup-init though - // to begin the installation, so delete every bin except that. - for entry in fs::read_dir(&config.exedir).unwrap() { - let entry = entry.unwrap(); - // ffs Path/OsStr... - let is_rustup_init = entry.path() - .file_name().unwrap_or_default() - .to_string_lossy().contains("rustup-init"); - if is_rustup_init { - continue; - } - fs::remove_file(entry.path()).unwrap(); - } - - // Likewise set up PATH to include ~/.cargo/bin - let prev_path = env::var_os("PATH"); - let mut new_path = config.cargodir.join("bin").into_os_string(); - if let Some(ref p) = prev_path { - new_path.push(if cfg!(windows) { ";" } else { ":" }); - new_path.push(p); - } - env::set_var("PATH", new_path); - // An windows these tests mess with the user's PATH. Save // and restore them here to keep from trashing things. let saved_path = get_path(); @@ -1196,57 +1171,13 @@ fn rls_proxy_set_up_after_install() { }); } -// This is testing that, when the rls proxy doesn't exist, rustup proxies will -// create it lazily. This has to happen because rustup's self-updates aren't -// able to execute arbitrary code at update time. #[test] -fn rls_proxy_set_up_lazily_after_upgrade_via_proxy() { +fn rls_proxy_set_up_after_update() { update_setup(&|config, _| { + let ref rls_path = config.cargodir.join(format!("bin/rls{}", EXE_SUFFIX)); expect_ok(config, &["rustup-init", "-y"]); - expect_ok(config, &["rustup", "component", "add", "rls"]); - - // Delete the rls proxy to simulate an upgrade from an older rustup that - // didn't include it - let ref rls_bin = config.cargodir.join(format!("bin/rls{}", EXE_SUFFIX)); - fs::remove_file(rls_bin).expect("rls_bin"); - - // RLS doesn't work now - assert!(!rls_bin.exists()); - let mut cmd = clitools::cmd(config, "rls", &["--version"]); - assert!(cmd.output().is_err()); - - // Run a proxy to trigger a lazy upgrade - expect_ok(config, &["rustc", "--version"]); - - // Now RLS works - assert!(rls_bin.exists()); - expect_ok(config, &["rls", "--version"]); - }); -} - -// Same as above but instead of the proxies triggering the update, it's rustup -// directly -#[test] -fn rls_proxy_set_up_lazily_after_upgrade_via_rustup() { - update_setup(&|config, _| { - expect_ok(config, &["rustup-init", "-y"]); - expect_ok(config, &["rustup", "component", "add", "rls"]); - - // Delete the rls proxy to simulate an upgrade from an older rustup that - // didn't include it - let ref rls_bin = config.cargodir.join(format!("bin/rls{}", EXE_SUFFIX)); - fs::remove_file(rls_bin).expect("rls_bin"); - - // RLS doesn't work now - assert!(!rls_bin.exists()); - let mut cmd = clitools::cmd(config, "rls", &["--version"]); - assert!(cmd.output().is_err()); - - // Run rustup to trigger the update - expect_ok(config, &["rustup", "component", "add", "rls"]); - - // Now RLS works - assert!(rls_bin.exists()); - expect_ok(config, &["rls", "--version"]); + fs::remove_file(rls_path).unwrap(); + expect_ok(config, &["rustup", "self", "update"]); + assert!(rls_path.exists()); }); }