Append Windows bin directory to PATH by default

This also allows opting out of the new default using the `RUSTUP_WINDOWS_PATH_ADD_BIN`. Setting it controls if or where the bin directory is added to PATH:

0 => don't add to PATH
1 => prepend to PATH
unset, or anything else => append to PATH

Author: ChrisDenton

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<PathBuf>,
+    append: Option<PathBuf>,
     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::<VecDeque<_>>();
         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<PathBuf>, 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",
+            ],
+        );
+    }
 }

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