Don't prepend CARGO_HOME/bin unnecessarily

The current logic forces nested invocations to execute `cargo` and
friends from `$CARGO_HOME/bin`. This makes rustup break in environments
where the appropriate rustup proxies to use happen to be installed
elsewhere (earlier on `$PATH`), and using the binaries in
`$CARGO_HOME/bin` would not work correctly.

It also ensures that Rustup won't change `$PATH` "just for the heck of
it", which _should_ help reduce unnecessary re-compilations when
downstream build logic notices that `$PATH` changes (since it will no
longer).

Helps with rust-lang#2848.

Fixes rust-lang/cargo#7431.

Author: Jon Gjengset

src/env_var.rs

@@ -1,3 +1,4 @@
+use std::collections::VecDeque;
 use std::env;
 use std::path::PathBuf;
 use std::process::Command;
@@ -21,15 +22,19 @@ fn append_path(name: &str, value: Vec<PathBuf>, cmd: &mut Command) {
     }
 }
 
-pub(crate) fn prepend_path(name: &str, value: Vec<PathBuf>, cmd: &mut Command) {
+pub(crate) fn prepend_path(name: &str, prepend: Vec<PathBuf>, cmd: &mut Command) {
     let old_value = process().var_os(name);
-    let mut parts: Vec<PathBuf>;
-    if let Some(ref v) = old_value {
-        parts = value;
-        parts.extend(env::split_paths(v).collect::<Vec<_>>());
+    let parts = if let Some(ref v) = old_value {
+        let mut tail = env::split_paths(v).collect::<VecDeque<_>>();
+        for path in prepend.into_iter().rev() {
+            if !tail.contains(&path) {
+                tail.push_front(path);
+            }
+        }
+        tail
     } else {
-        parts = value;
-    }
+        prepend.into()
+    };
 
     if let Ok(new_value) = env::join_paths(parts) {
         cmd.env(name, new_value);

tests/cli-misc.rs

@@ -298,6 +298,59 @@ fn rustup_run_searches_path() {
     });
 }
 
+#[test]
+fn rustup_doesnt_prepend_path_unnecessarily() {
+    setup(&|config| {
+        expect_ok(config, &["rustup", "default", "nightly"]);
+
+        let expect_stderr_ok_env_startswith =
+            |config: &Config, args: &[&str], env: &[(&str, &str)], expected: &str| {
+                let out = run(config, args[0], &args[1..], env);
+                if !out.ok || !out.stderr.starts_with(expected) {
+                    clitools::print_command(args, &out);
+                    println!("expected.ok: true");
+                    clitools::print_indented("expected.stderr.starts_with", expected);
+                    panic!();
+                }
+            };
+
+        // For all of these, CARGO_HOME/bin will be auto-prepended.
+        let cargo_home_bin = config.cargodir.join("bin");
+        expect_stderr_ok_env_startswith(
+            config,
+            &["cargo", "--echo-path"],
+            &[],
+            &format!("{}", cargo_home_bin.display()),
+        );
+        expect_stderr_ok_env_startswith(
+            config,
+            &["cargo", "--echo-path"],
+            &[("PATH", "")],
+            &format!("{}", cargo_home_bin.display()),
+        );
+
+        // Check that CARGO_HOME/bin is prepended to path.
+        expect_stderr_ok_env_startswith(
+            config,
+            &["cargo", "--echo-path"],
+            &[("PATH", &format!("{}", config.exedir.display()))],
+            &format!("{}:{}", cargo_home_bin.display(), config.exedir.display()),
+        );
+
+        // But if CARGO_HOME/bin is already on PATH, it will not be prepended again,
+        // so exedir will take precedence.
+        expect_stderr_ok_env_startswith(
+            config,
+            &["cargo", "--echo-path"],
+            &[(
+                "PATH",
+                &format!("{}:{}", config.exedir.display(), cargo_home_bin.display()),
+            )],
+            &format!("{}:{}", config.exedir.display(), cargo_home_bin.display()),
+        );
+    });
+}
+
 #[test]
 fn rustup_failed_path_search() {
     setup(&|config| {

tests/mock/clitools.rs

@@ -437,7 +437,7 @@ pub fn expect_component_not_executable(config: &Config, cmd: &str) {
     }
 }
 
-fn print_command(args: &[&str], out: &SanitizedOutput) {
+pub(crate) fn print_command(args: &[&str], out: &SanitizedOutput) {
     print!("\n>");
     for arg in args {
         if arg.contains(' ') {
@@ -452,7 +452,7 @@ fn print_command(args: &[&str], out: &SanitizedOutput) {
     print_indented("out.stderr", &out.stderr);
 }
 
-fn print_indented(heading: &str, text: &str) {
+pub(crate) fn print_indented(heading: &str, text: &str) {
     let mut lines = text.lines().count();
     // The standard library treats `a\n` and `a` as both being one line.
     // This is confusing when the test fails because of a missing newline.

tests/mock/mock_bin_src.rs

@@ -67,6 +67,10 @@ fn main() {
                 writeln!(out, "{}", arg.to_string_lossy()).unwrap();
             }
         }
+        Some("--echo-path") => {
+            let mut out = io::stderr();
+            writeln!(out, "{}", std::env::var("PATH").unwrap()).unwrap();
+        }
         _ => panic!("bad mock proxy commandline"),
     }
 }