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

Author: brson

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)))
 }
 

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<()> {

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");
     }

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<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};
@@ -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("");
@@ -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);
 
     // 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<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(),
             })
         }
     }

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<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)
     }
@@ -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> {

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| {