diff --git a/src/env_var.rs b/src/env_var.rs index 6b9863ad15..ed2eb44602 100644 --- a/src/env_var.rs +++ b/src/env_var.rs @@ -7,14 +7,15 @@ use crate::process::Process; pub const RUST_RECURSION_COUNT_MAX: u32 = 20; -pub(crate) fn prepend_path( +pub(crate) fn insert_path( name: &str, prepend: Vec, + append: Option, cmd: &mut Command, process: &Process, ) { let old_value = process.var_os(name); - let parts = if let Some(ref v) = old_value { + let mut parts = if let Some(v) = &old_value { let mut tail = env::split_paths(v).collect::>(); for path in prepend.into_iter().rev() { if !tail.contains(&path) { @@ -26,6 +27,12 @@ pub(crate) fn prepend_path( prepend.into() }; + if let Some(path) = append { + if !parts.contains(&path) { + parts.push_back(path); + } + } + if let Ok(new_value) = env::join_paths(parts) { cmd.env(name, new_value); } @@ -77,7 +84,7 @@ mod tests { let path_z = PathBuf::from(z); path_entries.push(path_z); - prepend_path("PATH", path_entries, &mut cmd, &tp.process); + insert_path("PATH", path_entries, None, &mut cmd, &tp.process); let envs: Vec<_> = cmd.get_envs().collect(); assert_eq!( @@ -99,4 +106,82 @@ mod tests { ),] ); } + + #[test] + fn append_unique_path() { + let mut vars = HashMap::new(); + vars.env( + "PATH", + env::join_paths(["/home/a/.cargo/bin", "/home/b/.cargo/bin"].iter()).unwrap(), + ); + let tp = TestProcess::with_vars(vars); + #[cfg(windows)] + let _path_guard = RegistryGuard::new(&USER_PATH).unwrap(); + + #[track_caller] + fn check(tp: &TestProcess, path_entries: Vec, append: &str, expected: &[&str]) { + let mut cmd = Command::new("test"); + + insert_path( + "PATH", + path_entries, + Some(append.into()), + &mut cmd, + &tp.process, + ); + let envs: Vec<_> = cmd.get_envs().collect(); + + assert_eq!( + envs, + &[( + OsStr::new("PATH"), + Some(env::join_paths(expected.iter()).unwrap().as_os_str()) + ),] + ); + } + + check( + &tp, + Vec::new(), + "/home/z/.cargo/bin", + &[ + "/home/a/.cargo/bin", + "/home/b/.cargo/bin", + "/home/z/.cargo/bin", + ], + ); + check( + &tp, + Vec::new(), + "/home/a/.cargo/bin", + &["/home/a/.cargo/bin", "/home/b/.cargo/bin"], + ); + check( + &tp, + Vec::new(), + "/home/b/.cargo/bin", + &["/home/a/.cargo/bin", "/home/b/.cargo/bin"], + ); + check( + &tp, + Vec::from(["/home/c/.cargo/bin".into()]), + "/home/c/.cargo/bin", + &[ + "/home/c/.cargo/bin", + "/home/a/.cargo/bin", + "/home/b/.cargo/bin", + ], + ); + check( + &tp, + Vec::from(["/home/c/.cargo/bin".into()]), + "/home/z/.cargo/bin", + &[ + "/home/c/.cargo/bin", + "/home/a/.cargo/bin", + "/home/b/.cargo/bin", + "/home/z/.cargo/bin", + ], + ); + } } diff --git a/src/toolchain.rs b/src/toolchain.rs index e65d9f443d..fadce15f42 100644 --- a/src/toolchain.rs +++ b/src/toolchain.rs @@ -217,7 +217,7 @@ impl<'a> Toolchain<'a> { new_path.push(PathBuf::from("/usr/lib")); } - env_var::prepend_path(sysenv::LOADER_PATH, new_path, cmd, self.cfg.process); + env_var::insert_path(sysenv::LOADER_PATH, new_path, None, cmd, self.cfg.process); // 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 @@ -228,30 +228,50 @@ impl<'a> Toolchain<'a> { path_entries.push(cargo_home.join("bin")); } - // Historically rustup 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 + // On Windows, we append the "bin" directory to PATH by default. + // Windows loads DLLs from PATH and the "bin" directory contains DLLs + // that proc macros and other tools not in the sysroot use. + // It's appended rather than prepended so that the exe files in "bin" + // do not take precedence over anything else in PATH. + // + // Historically rustup prepended the bin directory in PATH but doing so 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. + // See: https://github.com/rust-lang/rustup/pull/3178 // - // The RUSTUP_WINDOWS_PATH_ADD_BIN env var was added to opt-in to - // testing the fix. The default is now off, but this is left here - // just in case there are problems. Consider removing in the - // future if it doesn't seem necessary. - #[cfg(target_os = "windows")] - if self - .cfg - .process - .var_os("RUSTUP_WINDOWS_PATH_ADD_BIN") - .is_some_and(|s| s == "1") - { - path_entries.push(self.path.join("bin")); - } + // This behaviour was then changed to not add the bin directory at all. + // But this caused another set of problems due to the sysroot DLLs + // not being found by the loader, e.g. for proc macros. + // See: https://github.com/rust-lang/rustup/issues/3825 + // + // Which is how we arrived at the current default described above. + // + // The `RUSTUP_WINDOWS_PATH_ADD_BIN` environment variable allows + // users to opt-in to one of the old behaviours in case the new + // default causes any new issues. + // + // FIXME: The `RUSTUP_WINDOWS_PATH_ADD_BIN` environment variable can + // be removed once we're confident that the default behaviour works. + let append = if cfg!(target_os = "windows") { + let add_bin = self.cfg.process.var("RUSTUP_WINDOWS_PATH_ADD_BIN"); + match add_bin.as_deref().unwrap_or("append") { + // Don't add to PATH at all + "0" => None, + // Prepend to PATH + "1" => { + path_entries.push(self.path.join("bin")); + None + } + // Append to PATH (the default) + _ => Some(self.path.join("bin")), + } + } else { + None + }; - env_var::prepend_path("PATH", path_entries, cmd, self.cfg.process); + env_var::insert_path("PATH", path_entries, append, cmd, self.cfg.process); } /// Infallible function that describes the version of rustc in an installed distribution