Skip to content

Commit

Permalink
Recursive tool invocations should invoke the proxy, not the tool dire…
Browse files Browse the repository at this point in the history
…ctly

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
  • Loading branch information
brson committed Nov 22, 2016
1 parent a6db7a3 commit a7cba0c
Show file tree
Hide file tree
Showing 6 changed files with 132 additions and 29 deletions.
6 changes: 3 additions & 3 deletions src/rustup-cli/proxy_mode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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)))
}

2 changes: 1 addition & 1 deletion src/rustup-cli/rustup_mode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<()> {
Expand Down
8 changes: 8 additions & 0 deletions src/rustup-mock/src/mock_bin_src.rs
Original file line number Diff line number Diff line change
@@ -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();
Expand All @@ -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");
}
Expand Down
28 changes: 13 additions & 15 deletions src/rustup/command.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -16,21 +14,19 @@ use telemetry::{Telemetry, TelemetryEvent};


pub fn run_command_for_dir<S: AsRef<OsStr>>(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<S: AsRef<OsStr>>(mut cmd: Command, args: &[S], cfg: &Cfg) -> Result<()> {
fn telemetry_rustc<S: AsRef<OsStr>>(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};
Expand All @@ -45,7 +41,7 @@ fn telemetry_rustc<S: AsRef<OsStr>>(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("");
Expand Down Expand Up @@ -130,14 +126,16 @@ fn telemetry_rustc<S: AsRef<OsStr>>(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<S: AsRef<OsStr>>(mut cmd: Command, args: &[S]) -> Result<()> {
cmd.args(&args[1..]);
fn run_command_for_dir_without_telemetry<S: AsRef<OsStr>>(
mut cmd: Command, arg0: &str, args: &[S]) -> Result<()>
{
cmd.args(&args);

This comment has been minimized.

Copy link
@MoSal

MoSal Feb 28, 2017

Contributor

This line causes a compile failure in latest nightly. s/&args/args fixes it.


// FIXME rust-lang/rust#32254. It's not clear to me
// when and why this is needed.
Expand All @@ -151,7 +149,7 @@ fn run_command_for_dir_without_telemetry<S: AsRef<OsStr>>(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(),
})
}
}
Expand Down
78 changes: 68 additions & 10 deletions src/rustup/toolchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)))
Expand Down Expand Up @@ -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);
Expand All @@ -293,7 +316,42 @@ impl<'a> Toolchain<'a> {
// to give custom toolchains access to cargo
pub fn create_fallback_command<T: AsRef<OsStr>>(&self, binary: T,
primary_toolchain: &Toolchain) -> Result<Command> {
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)
}
Expand Down Expand Up @@ -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<PathBuf> {
Expand Down
39 changes: 39 additions & 0 deletions tests/cli-rustup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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| {
Expand Down

0 comments on commit a7cba0c

Please sign in to comment.