Merge pull request #2978 from jonhoo/dont-add-if-added

Don't prepend CARGO_HOME/bin unnecessarily

Author: kinnison

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

@@ -3,8 +3,8 @@
 
 pub mod mock;
 
-use std::env::consts::EXE_SUFFIX;
 use std::str;
+use std::{env::consts::EXE_SUFFIX, path::Path};
 
 use rustup::for_host;
 use rustup::test::this_host_triple;
@@ -298,6 +298,92 @@ 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_first_then =
+            |config: &Config,
+             args: &[&str],
+             env: &[(&str, &str)],
+             first: &Path,
+             second: Option<&Path>| {
+                let out = run(config, args[0], &args[1..], env);
+                let first_then_second = |list: &str| -> bool {
+                    let mut saw_first = false;
+                    let mut saw_second = false;
+                    for path in std::env::split_paths(list) {
+                        if path == first {
+                            if saw_second {
+                                return false;
+                            }
+                            saw_first = true;
+                        }
+                        if Some(&*path) == second {
+                            if !saw_first {
+                                return false;
+                            }
+                            saw_second = true;
+                        }
+                    }
+                    true
+                };
+                if !out.ok || !first_then_second(&out.stderr) {
+                    clitools::print_command(args, &out);
+                    println!("expected.ok: true");
+                    clitools::print_indented(
+                        "expected.stderr.first_then",
+                        &format!("{} comes before {:?}", first.display(), second),
+                    );
+                    panic!();
+                }
+            };
+
+        // For all of these, CARGO_HOME/bin will be auto-prepended.
+        let cargo_home_bin = config.cargodir.join("bin");
+        expect_stderr_ok_env_first_then(
+            config,
+            &["cargo", "--echo-path"],
+            &[],
+            &cargo_home_bin,
+            None,
+        );
+        expect_stderr_ok_env_first_then(
+            config,
+            &["cargo", "--echo-path"],
+            &[("PATH", "")],
+            &cargo_home_bin,
+            None,
+        );
+
+        // Check that CARGO_HOME/bin is prepended to path.
+        expect_stderr_ok_env_first_then(
+            config,
+            &["cargo", "--echo-path"],
+            &[("PATH", &format!("{}", config.exedir.display()))],
+            &cargo_home_bin,
+            Some(&config.exedir),
+        );
+
+        // But if CARGO_HOME/bin is already on PATH, it will not be prepended again,
+        // so exedir will take precedence.
+        expect_stderr_ok_env_first_then(
+            config,
+            &["cargo", "--echo-path"],
+            &[(
+                "PATH",
+                std::env::join_paths([&config.exedir, &cargo_home_bin])
+                    .unwrap()
+                    .to_str()
+                    .unwrap(),
+            )],
+            &config.exedir,
+            Some(&cargo_home_bin),
+        );
+    });
+}
+
 #[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"),
     }
 }