From 4c6a3d54ec7d12d915eaf51e89b1d1b8e0c3c183 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 22 Jan 2023 15:57:08 -0800 Subject: [PATCH 1/2] Don't add toolchain bin to PATH on Windows --- src/toolchain.rs | 4 ---- tests/mock/mock_bin_src.rs | 14 ++++++++++++- tests/suite/cli_rustup.rs | 43 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/src/toolchain.rs b/src/toolchain.rs index ff17dfb03c..0609004592 100644 --- a/src/toolchain.rs +++ b/src/toolchain.rs @@ -453,10 +453,6 @@ impl<'a> InstalledCommonToolchain<'a> { path_entries.push(cargo_home.join("bin")); } - if cfg!(target_os = "windows") { - path_entries.push(self.0.path.join("bin")); - } - env_var::prepend_path("PATH", path_entries, cmd); } } diff --git a/tests/mock/mock_bin_src.rs b/tests/mock/mock_bin_src.rs index 7acafb67e1..24add3c43e 100644 --- a/tests/mock/mock_bin_src.rs +++ b/tests/mock/mock_bin_src.rs @@ -61,6 +61,18 @@ fn main() { let rustc = &format!("rustc{}", EXE_SUFFIX); Command::new(rustc).arg("--version").status().unwrap(); } + Some("--recursive-cargo-subcommand") => { + Command::new("cargo-foo") + .arg("--recursive-cargo") + .status() + .unwrap(); + } + Some("--recursive-cargo") => { + Command::new("cargo") + .args(&["+nightly", "--version"]) + .status() + .unwrap(); + } Some("--echo-args") => { let mut out = io::stderr(); for arg in args { @@ -71,7 +83,7 @@ fn main() { let mut out = io::stderr(); writeln!(out, "{}", std::env::var("PATH").unwrap()).unwrap(); } - _ => panic!("bad mock proxy commandline"), + arg => panic!("bad mock proxy commandline: {:?}", arg), } } diff --git a/tests/suite/cli_rustup.rs b/tests/suite/cli_rustup.rs index 4a500ed61c..5f0af9c832 100644 --- a/tests/suite/cli_rustup.rs +++ b/tests/suite/cli_rustup.rs @@ -547,6 +547,49 @@ fn fallback_cargo_calls_correct_rustc() { }); } +// Checks that cargo can recursively invoke itself with rustup shorthand (via +// the proxy). +// +// This involves a series of chained commands: +// +// 1. Calls `cargo --recursive-cargo-subcommand` +// 2. The rustup `cargo` proxy launches, and launches the "mock" nightly cargo exe. +// 3. The nightly "mock" cargo sees --recursive-cargo-subcommand, and launches +// `cargo-foo --recursive-cargo` +// 4. `cargo-foo` sees `--recursive-cargo` and launches `cargo +nightly --version` +// 5. The rustup `cargo` proxy launches, and launches the "mock" nightly cargo exe. +// 6. The nightly "mock" cargo sees `--version` and prints the version. +// +// Previously, rustup would place the toolchain's `bin` directory in PATH for +// Windows due to some DLL issues. However, those aren't necessary anymore. +// If the toolchain `bin` directory is in PATH, then this test would fail in +// step 5 because the `cargo` executable would be the "mock" nightly cargo, +// and the first argument would be `+nightly` which would be an error. +#[test] +fn recursive_cargo() { + test(&|config| { + config.with_scenario(Scenario::ArchivesV2, &|config| { + config.expect_ok(&["rustup", "default", "nightly"]); + + // We need an intermediary to run cargo itself. + // The "mock" cargo can't do that because on Windows it will check + // for a `cargo.exe` in the current directory before checking PATH. + // + // The solution here is to copy from the "mock" `cargo.exe` into + // `~/.cargo/bin/cargo-foo`. This is just for convenience to avoid + // needing to build another executable just for this test. + let output = config.run("rustup", &["which", "cargo"], &[]); + let real_mock_cargo = output.stdout.trim(); + let cargo_bin_path = config.cargodir.join("bin"); + let cargo_subcommand = cargo_bin_path.join(format!("cargo-foo{}", EXE_SUFFIX)); + 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"); + }); + }); +} + #[test] fn show_home() { test(&|config| { From 6df98f6b8488f75d6afb8cb84f965c399d5da559 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 4 Feb 2023 07:50:30 -0800 Subject: [PATCH 2/2] 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. --- src/toolchain.rs | 19 +++++++++++++++++++ tests/mock/mock_bin_src.rs | 6 ++++-- tests/suite/cli_rustup.rs | 16 +++++++++++++++- 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/toolchain.rs b/src/toolchain.rs index 0609004592..99759ade1d 100644 --- a/src/toolchain.rs +++ b/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); } } diff --git a/tests/mock/mock_bin_src.rs b/tests/mock/mock_bin_src.rs index 24add3c43e..4d4ba25432 100644 --- a/tests/mock/mock_bin_src.rs +++ b/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(); diff --git a/tests/suite/cli_rustup.rs b/tests/suite/cli_rustup.rs index 5f0af9c832..54e4f7c20f 100644 --- a/tests/suite/cli_rustup.rs +++ b/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"); + } }); }); }