From a7cba0c74b5dddc6e76ca9e719d1e0cfa09396db Mon Sep 17 00:00:00 2001 From: Brian Anderson Date: Tue, 15 Nov 2016 03:44:09 +0000 Subject: [PATCH] Recursive tool invocations should invoke the proxy, not the tool directly The way the proxy was setting up the PATH variable contained two bugs: first, that it allowed the toolchain path to precede the value of CARGO_HOME/bin; but second that it didn't add CARGO_HOME/bin to the path at all. The result was that when e.g. cargo execs rustc, it was directly execing the toolchain rustc. Now it execs the proxy, assuming that CARGO_HOME/bin is set up correctly, and guaranteeing not to run the toolchain tool directly. Fixes #809 --- src/rustup-cli/proxy_mode.rs | 6 +-- src/rustup-cli/rustup_mode.rs | 2 +- src/rustup-mock/src/mock_bin_src.rs | 8 +++ src/rustup/command.rs | 28 +++++------ src/rustup/toolchain.rs | 78 +++++++++++++++++++++++++---- tests/cli-rustup.rs | 39 +++++++++++++++ 6 files changed, 132 insertions(+), 29 deletions(-) diff --git a/src/rustup-cli/proxy_mode.rs b/src/rustup-cli/proxy_mode.rs index b93acab78b..a25f30c6a9 100644 --- a/src/rustup-cli/proxy_mode.rs +++ b/src/rustup-cli/proxy_mode.rs @@ -34,9 +34,9 @@ pub fn main() -> Result<()> { // Build command args now while we know whether or not to skip arg 1. let cmd_args: Vec<_> = if toolchain.is_none() { - env::args_os().collect() + env::args_os().skip(1).collect() } else { - env::args_os().take(1).chain(env::args_os().skip(2)).collect() + env::args_os().skip(2).collect() }; let cfg = try!(set_globals(false)); @@ -51,6 +51,6 @@ fn direct_proxy(cfg: &Cfg, arg0: &str, toolchain: Option<&str>, args: &[OsString None => try!(cfg.create_command_for_dir(&try!(utils::current_dir()), arg0)), Some(tc) => try!(cfg.create_command_for_toolchain(tc, arg0)), }; - Ok(try!(run_command_for_dir(cmd, &args, &cfg))) + Ok(try!(run_command_for_dir(cmd, arg0, args, &cfg))) } diff --git a/src/rustup-cli/rustup_mode.rs b/src/rustup-cli/rustup_mode.rs index 5cb98c4562..e3a2baf25b 100644 --- a/src/rustup-cli/rustup_mode.rs +++ b/src/rustup-cli/rustup_mode.rs @@ -487,7 +487,7 @@ fn run(cfg: &Cfg, m: &ArgMatches) -> Result<()> { let args: Vec<_> = args.collect(); let cmd = try!(cfg.create_command_for_toolchain(toolchain, args[0])); - Ok(try!(command::run_command_for_dir(cmd, &args, &cfg))) + Ok(try!(command::run_command_for_dir(cmd, args[0], &args[1..], &cfg))) } fn which(cfg: &Cfg, m: &ArgMatches) -> Result<()> { diff --git a/src/rustup-mock/src/mock_bin_src.rs b/src/rustup-mock/src/mock_bin_src.rs index d4728f5485..3f090e705c 100644 --- a/src/rustup-mock/src/mock_bin_src.rs +++ b/src/rustup-mock/src/mock_bin_src.rs @@ -1,4 +1,6 @@ +use std::process::Command; use std::io::{self, BufWriter, Write}; +use std::env::consts::EXE_SUFFIX; fn main() { let args: Vec<_> = ::std::env::args().collect(); @@ -13,6 +15,12 @@ fn main() { for _ in 0 .. 10000 { buf.write_all(b"error: a value named `fail` has already been defined in this module [E0428]\n").unwrap(); } + } else if args.get(1) == Some(&"--call-rustc".to_string()) { + // Used by the fallback_cargo_calls_correct_rustc test. Tests that + // the environment has been set up right such that invoking rustc + // will actually invoke the wrapper + let rustc = &format!("rustc{}", EXE_SUFFIX); + Command::new(rustc).arg("--version").status().unwrap(); } else { panic!("bad mock proxy commandline"); } diff --git a/src/rustup/command.rs b/src/rustup/command.rs index 52119a2687..72e7d75bca 100644 --- a/src/rustup/command.rs +++ b/src/rustup/command.rs @@ -1,8 +1,6 @@ -use std::env; use std::ffi::OsStr; use std::fs::File; use std::io::{self, Write, BufRead, BufReader, Seek, SeekFrom}; -use std::path::PathBuf; use std::process::{self, Command, Stdio}; use std::time::Instant; use regex::Regex; @@ -16,21 +14,19 @@ use telemetry::{Telemetry, TelemetryEvent}; pub fn run_command_for_dir>(cmd: Command, + arg0: &str, args: &[S], cfg: &Cfg) -> Result<()> { - let arg0 = env::args().next().map(PathBuf::from); - let arg0 = arg0.as_ref() - .and_then(|a| a.file_name()) - .and_then(|a| a.to_str()); - let arg0 = try!(arg0.ok_or(ErrorKind::NoExeName)); if (arg0 == "rustc" || arg0 == "rustc.exe") && try!(cfg.telemetry_enabled()) { - return telemetry_rustc(cmd, args, cfg); + return telemetry_rustc(cmd, arg0, args, cfg); } - run_command_for_dir_without_telemetry(cmd, args) + run_command_for_dir_without_telemetry(cmd, arg0, args) } -fn telemetry_rustc>(mut cmd: Command, args: &[S], cfg: &Cfg) -> Result<()> { +fn telemetry_rustc>(mut cmd: Command, + arg0: &str, + args: &[S], cfg: &Cfg) -> Result<()> { #[cfg(unix)] fn file_as_stdio(file: &File) -> Stdio { use std::os::unix::io::{AsRawFd, FromRawFd}; @@ -45,7 +41,7 @@ fn telemetry_rustc>(mut cmd: Command, args: &[S], cfg: &Cfg) -> let now = Instant::now(); - cmd.args(&args[1..]); + cmd.args(args); let has_color_args = args.iter().any(|e| { let e = e.as_ref().to_str().unwrap_or(""); @@ -130,14 +126,16 @@ fn telemetry_rustc>(mut cmd: Command, args: &[S], cfg: &Cfg) -> }); Err(e).chain_err(|| rustup_utils::ErrorKind::RunningCommand { - name: args[0].as_ref().to_owned(), + name: OsStr::new(arg0).to_owned(), }) }, } } -fn run_command_for_dir_without_telemetry>(mut cmd: Command, args: &[S]) -> Result<()> { - cmd.args(&args[1..]); +fn run_command_for_dir_without_telemetry>( + mut cmd: Command, arg0: &str, args: &[S]) -> Result<()> +{ + cmd.args(&args); // FIXME rust-lang/rust#32254. It's not clear to me // when and why this is needed. @@ -151,7 +149,7 @@ fn run_command_for_dir_without_telemetry>(mut cmd: Command, args } Err(e) => { Err(e).chain_err(|| rustup_utils::ErrorKind::RunningCommand { - name: args[0].as_ref().to_owned(), + name: OsStr::new(arg0).to_owned(), }) } } diff --git a/src/rustup/toolchain.rs b/src/rustup/toolchain.rs index 9d351a54e6..2eb2059267 100644 --- a/src/rustup/toolchain.rs +++ b/src/rustup/toolchain.rs @@ -13,6 +13,8 @@ use install::{self, InstallMethod}; use telemetry; use telemetry::{Telemetry, TelemetryEvent}; +use std::env::consts::EXE_SUFFIX; +use std::ffi::OsString; use std::process::Command; use std::path::{Path, PathBuf}; use std::ffi::OsStr; @@ -67,7 +69,16 @@ impl<'a> Toolchain<'a> { &self.path } pub fn exists(&self) -> bool { - utils::is_directory(&self.path) + // HACK: linked toolchains are symlinks, and, contrary to what std docs + // lead me to believe `fs::metadata`, used by `is_directory` does not + // seem to follow symlinks on windows. + let is_symlink = if cfg!(windows) { + use std::fs; + fs::symlink_metadata(&self.path).map(|m| m.file_type().is_symlink()).unwrap_or(false) + } else { + false + }; + utils::is_directory(&self.path) || is_symlink } pub fn verify(&self) -> Result<()> { Ok(try!(utils::assert_is_directory(&self.path))) @@ -277,12 +288,24 @@ impl<'a> Toolchain<'a> { return Err(ErrorKind::ToolchainNotInstalled(self.name.to_owned()).into()); } - // Assume this binary exists within the current toolchain - let bin_path = self.path.join("bin").join(binary.as_ref()); + // Create the path to this binary within the current toolchain sysroot + let binary = if let Some(binary_str) = binary.as_ref().to_str() { + if binary_str.ends_with(EXE_SUFFIX) { + binary.as_ref().to_owned() + } else { + OsString::from(format!("{}{}", binary_str, EXE_SUFFIX)) + } + } else { + // Very weird case. Non-unicode command. + binary.as_ref().to_owned() + }; + + let bin_path = self.path.join("bin").join(&binary); let mut cmd = Command::new(if utils::is_file(&bin_path) { &bin_path } else { - // If not, let the OS try to resolve it globally for us + // If the bin doesn't actually exist in the sysroot, let the OS try + // to resolve it globally for us Path::new(&binary) }); self.set_env(&mut cmd); @@ -293,7 +316,42 @@ impl<'a> Toolchain<'a> { // to give custom toolchains access to cargo pub fn create_fallback_command>(&self, binary: T, primary_toolchain: &Toolchain) -> Result { - let mut cmd = try!(self.create_command(binary)); + // With the hacks below this only works for cargo atm + assert!(binary.as_ref() == "cargo" || binary.as_ref() == "cargo.exe"); + + if !self.exists() { + return Err(ErrorKind::ToolchainNotInstalled(self.name.to_owned()).into()); + } + if !primary_toolchain.exists() { + return Err(ErrorKind::ToolchainNotInstalled(self.name.to_owned()).into()); + } + + let src_file = self.path.join("bin").join(format!("cargo{}", EXE_SUFFIX)); + + // MAJOR HACKS: Copy cargo.exe to its own directory on windows before + // running it. This is so that the fallback cargo, when it in turn runs + // rustc.exe, will run the rustc.exe out of the PATH environment + // variable, _not_ the rustc.exe sitting in the same directory as the + // fallback. See the `fallback_cargo_calls_correct_rustc` testcase and + // PR 812. + let exe_path = if cfg!(windows) { + use std::fs; + let fallback_dir = self.cfg.multirust_dir.join("fallback"); + try!(fs::create_dir_all(&fallback_dir) + .chain_err(|| "unable to create dir to hold fallback exe")); + let fallback_file = fallback_dir.join("cargo.exe"); + if fallback_file.exists() { + try!(fs::remove_file(&fallback_file) + .chain_err(|| "unable to unlink old fallback exe")); + } + try!(fs::hard_link(&src_file, &fallback_file) + .chain_err(|| "unable to hard link fallback exe")); + fallback_file + } else { + src_file + }; + let mut cmd = Command::new(exe_path); + self.set_env(&mut cmd); cmd.env("RUSTUP_TOOLCHAIN", &primary_toolchain.name); Ok(cmd) } @@ -328,13 +386,13 @@ impl<'a> Toolchain<'a> { } env_var::prepend_path(sysenv::LOADER_PATH, &new_path, cmd); - // Prepend first cargo_home, then toolchain/bin to the PATH - let mut path_to_prepend = PathBuf::from(""); + // Prepend CARGO_HOME/bin to the PATH variable so that we're sure to run + // cargo/rustc via the proxy bins. There is no fallback case for if the + // proxy bins don't exist. We'll just be running whatever happens to + // be on the PATH. if let Ok(cargo_home) = utils::cargo_home() { - path_to_prepend.push(cargo_home.join("bin")); + env_var::prepend_path("PATH", &cargo_home.join("bin"), cmd); } - path_to_prepend.push(self.path.join("bin")); - env_var::prepend_path("PATH", path_to_prepend.as_path(), cmd); } pub fn doc_path(&self, relative: &str) -> Result { diff --git a/tests/cli-rustup.rs b/tests/cli-rustup.rs index f6b3a9de05..69c73dedcc 100644 --- a/tests/cli-rustup.rs +++ b/tests/cli-rustup.rs @@ -5,6 +5,8 @@ extern crate rustup_utils; extern crate rustup_mock; extern crate tempdir; +use std::fs; +use std::env::consts::EXE_SUFFIX; use rustup_mock::clitools::{self, Config, Scenario, expect_ok, expect_ok_ex, expect_stdout_ok, @@ -269,6 +271,43 @@ fn link() { }); } +// Issue #809. When we call the fallback cargo, when it in turn invokes +// "rustc", that rustc should actually be the rustup proxy, not the toolchain rustc. +// That way the proxy can pick the correct toolchain. +#[test] +fn fallback_cargo_calls_correct_rustc() { + setup(&|config| { + // Hm, this is the _only_ test that assumes that toolchain proxies + // exist in CARGO_HOME. Adding that proxy here. + let ref rustup_path = config.exedir.join(format!("rustup{}", EXE_SUFFIX)); + let ref cargo_bin_path = config.cargodir.join("bin"); + fs::create_dir_all(cargo_bin_path).unwrap(); + let ref rustc_path = cargo_bin_path.join(format!("rustc{}", EXE_SUFFIX)); + fs::hard_link(rustup_path, rustc_path).unwrap(); + + // Install a custom toolchain and a nightly toolchain for the cargo fallback + let path = config.customdir.join("custom-1"); + let path = path.to_string_lossy(); + expect_ok(config, &["rustup", "toolchain", "link", "custom", + &path]); + expect_ok(config, &["rustup", "default", "custom"]); + expect_ok(config, &["rustup", "update", "nightly"]); + expect_stdout_ok(config, &["rustc", "--version"], + "hash-c-1"); + expect_stdout_ok(config, &["cargo", "--version"], + "hash-n-2"); + + assert!(rustc_path.exists()); + + // Here --call-rustc tells the mock cargo bin to exec `rustc --version`. + // We should be ultimately calling the custom rustc, according to the + // RUSTUP_TOOLCHAIN variable set by the original "cargo" proxy, and + // interpreted by the nested "rustc" proxy. + expect_stdout_ok(config, &["cargo", "--call-rustc"], + "hash-c-1"); + }); +} + #[test] fn show_toolchain_none() { setup(&|config| {