Add RUSTUP_WINDOWS_PATH_ADD_BIN

Due to the uncertainty around whether or not this change will
cause problems, this introduces an opt-in for removing the PATH
change on Windows.

Author: ehuss

src/toolchain.rs

@@ -453,6 +453,25 @@ impl<'a> InstalledCommonToolchain<'a> {
             path_entries.push(cargo_home.join("bin"));
         }
 
+        if cfg!(target_os = "windows") {
+            // Historically rustup has included the bin directory in PATH to
+            // work around some bugs (see
+            // https://github.com/rust-lang/rustup/pull/3178 for more
+            // information). This shouldn't be needed anymore, and it causes
+            // problems because calling tools recursively (like `cargo
+            // +nightly metadata` from within a cargo subcommand). The
+            // recursive call won't work because it is not executing the
+            // proxy, so the `+` toolchain override doesn't work.
+            //
+            // This is opt-in to allow us to get more real-world testing.
+            if process()
+                .var_os("RUSTUP_WINDOWS_PATH_ADD_BIN")
+                .map_or(true, |s| s == "1")
+            {
+                path_entries.push(self.0.path.join("bin"));
+            }
+        }
+
         env_var::prepend_path("PATH", path_entries, cmd);
     }
 }

tests/mock/mock_bin_src.rs

@@ -62,16 +62,18 @@ fn main() {
             Command::new(rustc).arg("--version").status().unwrap();
         }
         Some("--recursive-cargo-subcommand") => {
-            Command::new("cargo-foo")
+            let status = Command::new("cargo-foo")
                 .arg("--recursive-cargo")
                 .status()
                 .unwrap();
+            assert!(status.success());
         }
         Some("--recursive-cargo") => {
-            Command::new("cargo")
+            let status = Command::new("cargo")
                 .args(&["+nightly", "--version"])
                 .status()
                 .unwrap();
+            assert!(status.success());
         }
         Some("--echo-args") => {
             let mut out = io::stderr();

tests/suite/cli_rustup.rs

@@ -585,7 +585,21 @@ fn recursive_cargo() {
             fs::create_dir_all(&cargo_bin_path).unwrap();
             fs::copy(&real_mock_cargo, &cargo_subcommand).unwrap();
 
-            config.expect_stdout_ok(&["cargo", "--recursive-cargo-subcommand"], "hash-nightly-2");
+            // Verify the default behavior, which is currently broken on Windows.
+            let args = &["cargo", "--recursive-cargo-subcommand"];
+            if cfg!(windows) {
+                config.expect_err(
+                    &["cargo", "--recursive-cargo-subcommand"],
+                    "bad mock proxy commandline",
+                );
+            }
+
+            // Try the opt-in, which should fix it.
+            let out = config.run(args[0], &args[1..], &[("RUSTUP_WINDOWS_PATH_ADD_BIN", "0")]);
+            if !out.ok || !out.stdout.contains("hash-nightly-2") {
+                clitools::print_command(args, &out);
+                panic!("expected hash-nightly-2 in output");
+            }
         });
     });
 }