From 2c4828efa52dc9d7f271409ad29641680a76e1e4 Mon Sep 17 00:00:00 2001 From: chansuke Date: Sat, 18 Sep 2021 14:14:23 +0900 Subject: [PATCH 1/4] Fix duplicated PATH entries - Unit test append_path and prepend_path - Order is preserved, second and above instances of a given entry are dropped. Co-Authored-By: Robert Collins Signed-off-by: Robert Collins --- src/env_var.rs | 181 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 178 insertions(+), 3 deletions(-) diff --git a/src/env_var.rs b/src/env_var.rs index ab48f5bc88..6328381421 100644 --- a/src/env_var.rs +++ b/src/env_var.rs @@ -1,5 +1,6 @@ use std::collections::VecDeque; use std::env; +use std::ffi::OsStr; use std::path::PathBuf; use std::process::Command; @@ -7,13 +8,34 @@ use crate::process; pub const RUST_RECURSION_COUNT_MAX: u32 = 20; +/// This can be removed when https://github.com/rust-lang/rust/issues/ 44434 is +/// stablised. +pub(crate) trait ProcessEnvs { + fn env(&mut self, key: K, val: V) -> &mut Self + where + Self: Sized, + K: AsRef, + V: AsRef; +} + +impl ProcessEnvs for Command { + fn env(&mut self, key: K, val: V) -> &mut Self + where + Self: Sized, + K: AsRef, + V: AsRef, + { + self.env(key, val) + } +} + #[allow(unused)] -fn append_path(name: &str, value: Vec, cmd: &mut Command) { +fn append_path(name: &str, value: Vec, cmd: &mut E) { let old_value = process().var_os(name); let mut parts: Vec; if let Some(ref v) = old_value { - parts = env::split_paths(v).collect(); - parts.extend(value); + let old_paths: Vec = env::split_paths(v).collect::>(); + parts = concat_uniq_paths(old_paths, value); } else { parts = value; } @@ -50,3 +72,156 @@ pub(crate) fn inc(name: &str, cmd: &mut Command) { cmd.env(name, (old_value + 1).to_string()); } + +fn concat_uniq_paths(fst_paths: Vec, snd_paths: Vec) -> Vec { + let deduped_fst_paths = dedupe_with_preserved_order(fst_paths); + let deduped_snd_paths = dedupe_with_preserved_order(snd_paths); + + let vec_fst_paths: Vec<_> = deduped_fst_paths.into_iter().collect(); + + let mut unified_paths; + unified_paths = vec_fst_paths.clone(); + unified_paths.extend( + deduped_snd_paths + .into_iter() + .filter(|v| !vec_fst_paths.contains(v)) + .collect::>(), + ); + + unified_paths +} + +fn dedupe_with_preserved_order(paths: Vec) -> Vec { + let mut uniq_paths: Vec = Vec::new(); + let mut seen_paths: HashSet = HashSet::new(); + + for path in &paths { + if !seen_paths.contains(path) { + seen_paths.insert(path.to_path_buf()); + uniq_paths.push(path.to_path_buf()); + } + } + + uniq_paths +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::currentprocess; + use crate::test::{with_saved_path, Env}; + + use super::ProcessEnvs; + use std::collections::HashMap; + use std::ffi::{OsStr, OsString}; + + #[derive(Default)] + struct TestCommand { + envs: HashMap>, + } + + impl ProcessEnvs for TestCommand { + fn env(&mut self, key: K, val: V) -> &mut Self + where + Self: Sized, + K: AsRef, + V: AsRef, + { + self.envs + .insert(key.as_ref().to_owned(), Some(val.as_ref().to_owned())); + self + } + } + + #[test] + fn deduplicate_and_concat_paths() { + let mut old_paths = vec![]; + + let z = OsString::from("/home/z/.cargo/bin"); + let path_z = PathBuf::from(z); + old_paths.push(path_z); + + let a = OsString::from("/home/a/.cargo/bin"); + let path_a = PathBuf::from(a); + old_paths.push(path_a); + + let _a = OsString::from("/home/a/.cargo/bin"); + let _path_a = PathBuf::from(_a); + old_paths.push(_path_a); + + let mut new_paths = vec![]; + + let n = OsString::from("/home/n/.cargo/bin"); + let path_n = PathBuf::from(n); + new_paths.push(path_n); + + let g = OsString::from("/home/g/.cargo/bin"); + let path_g = PathBuf::from(g); + new_paths.push(path_g); + + let _g = OsString::from("/home/g/.cargo/bin"); + let _path_g = PathBuf::from(_g); + new_paths.push(_path_g); + + let _z = OsString::from("/home/z/.cargo/bin"); + let path_z = PathBuf::from(_z); + old_paths.push(path_z); + + let mut unified_paths: Vec = vec![]; + let zpath = OsString::from("/home/z/.cargo/bin"); + let _zpath = PathBuf::from(zpath); + unified_paths.push(_zpath); + let apath = OsString::from("/home/a/.cargo/bin"); + let _apath = PathBuf::from(apath); + unified_paths.push(_apath); + let npath = OsString::from("/home/n/.cargo/bin"); + let _npath = PathBuf::from(npath); + unified_paths.push(_npath); + let gpath = OsString::from("/home/g/.cargo/bin"); + let _gpath = PathBuf::from(gpath); + unified_paths.push(_gpath); + + assert_eq!(concat_uniq_paths(old_paths, new_paths), unified_paths); + } + + #[test] + fn prepend_unique_path() { + let mut vars = HashMap::new(); + vars.env("PATH", "/home/a/.cargo/bin:/home/b/.cargo/bin"); + let tp = Box::new(currentprocess::TestProcess { + vars, + ..Default::default() + }); + with_saved_path(&|| { + currentprocess::with(tp.clone(), || { + let mut path_entries = vec![]; + let mut cmd = TestCommand::default(); + + let a = OsString::from("/home/a/.cargo/bin"); + let path_a = PathBuf::from(a); + path_entries.push(path_a); + + let _a = OsString::from("/home/a/.cargo/bin"); + let _path_a = PathBuf::from(_a); + path_entries.push(_path_a); + + let z = OsString::from("/home/z/.cargo/bin"); + let path_z = PathBuf::from(z); + path_entries.push(path_z); + + prepend_path("PATH", path_entries, &mut cmd); + let envs: Vec<_> = cmd.envs.iter().collect(); + + assert_eq!( + envs, + &[( + &OsString::from("PATH"), + &Some(OsString::from( + "/home/a/.cargo/bin:/home/z/.cargo/bin:/home/b/.cargo/bin" + )) + ),] + ); + }); + }); + } +} From 067ea0fe5274514ee6182d896001e74d9c209aa0 Mon Sep 17 00:00:00 2001 From: Dustin Martin Date: Sun, 6 Feb 2022 22:35:49 -0800 Subject: [PATCH 2/4] Fix prepend_unique_path test on Windows --- src/env_var.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/env_var.rs b/src/env_var.rs index 6328381421..1404afc756 100644 --- a/src/env_var.rs +++ b/src/env_var.rs @@ -187,7 +187,10 @@ mod tests { #[test] fn prepend_unique_path() { let mut vars = HashMap::new(); - vars.env("PATH", "/home/a/.cargo/bin:/home/b/.cargo/bin"); + vars.env( + "PATH", + env::join_paths(vec!["/home/a/.cargo/bin", "/home/b/.cargo/bin"].iter()).unwrap(), + ); let tp = Box::new(currentprocess::TestProcess { vars, ..Default::default() @@ -216,9 +219,17 @@ mod tests { envs, &[( &OsString::from("PATH"), - &Some(OsString::from( - "/home/a/.cargo/bin:/home/z/.cargo/bin:/home/b/.cargo/bin" - )) + &Some( + env::join_paths( + vec![ + "/home/a/.cargo/bin", + "/home/z/.cargo/bin", + "/home/b/.cargo/bin" + ] + .iter() + ) + .unwrap() + ) ),] ); }); From fc5cc4bab671d8b1caa686265672f7d2eb859aee Mon Sep 17 00:00:00 2001 From: chansuke Date: Tue, 8 Feb 2022 02:53:34 +0900 Subject: [PATCH 3/4] Remove space from comment --- src/env_var.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/env_var.rs b/src/env_var.rs index 1404afc756..b880aadb5a 100644 --- a/src/env_var.rs +++ b/src/env_var.rs @@ -8,7 +8,7 @@ use crate::process; pub const RUST_RECURSION_COUNT_MAX: u32 = 20; -/// This can be removed when https://github.com/rust-lang/rust/issues/ 44434 is +/// This can be removed when https://github.com/rust-lang/rust/issues/44434 is /// stablised. pub(crate) trait ProcessEnvs { fn env(&mut self, key: K, val: V) -> &mut Self From c97e02a9052de26e13f6b0e60dd71567e76208f4 Mon Sep 17 00:00:00 2001 From: chansuke Date: Sat, 18 Sep 2021 14:14:23 +0900 Subject: [PATCH 4/4] Add type parameter to prepend_path --- src/env_var.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/env_var.rs b/src/env_var.rs index b880aadb5a..ccf1b5ca9f 100644 --- a/src/env_var.rs +++ b/src/env_var.rs @@ -1,4 +1,4 @@ -use std::collections::VecDeque; +use std::collections::{HashSet, VecDeque}; use std::env; use std::ffi::OsStr; use std::path::PathBuf; @@ -44,7 +44,7 @@ fn append_path(name: &str, value: Vec, cmd: &mut E) { } } -pub(crate) fn prepend_path(name: &str, prepend: Vec, cmd: &mut Command) { +pub(crate) fn prepend_path(name: &str, prepend: Vec, cmd: &mut E) { let old_value = process().var_os(name); let parts = if let Some(ref v) = old_value { let mut tail = env::split_paths(v).collect::>(); @@ -222,8 +222,8 @@ mod tests { &Some( env::join_paths( vec![ - "/home/a/.cargo/bin", "/home/z/.cargo/bin", + "/home/a/.cargo/bin", "/home/b/.cargo/bin" ] .iter()