From 9fc36e1fbc9bf5c1cd5786bdc1c61bd36ce21e1d Mon Sep 17 00:00:00 2001 From: Jeff Dickey <216188+jdx@users.noreply.github.com> Date: Tue, 5 Dec 2023 16:18:14 -0600 Subject: [PATCH] shims: insert rtx PATH entries just before the shim directory Before, rtx would put the PATH entries at the front of PATH but that is too aggressive in some scenarios Fixes #1061 --- e2e/assert.sh | 9 ++++ e2e/test_shims | 7 ++- src/main.rs | 1 + src/path_env.rs | 122 +++++++++++++++++++++++++++++++++++++++++++++ src/toolset/mod.rs | 21 ++++---- 5 files changed, 148 insertions(+), 12 deletions(-) create mode 100644 src/path_env.rs diff --git a/e2e/assert.sh b/e2e/assert.sh index 246fb2571..1b3dfa93c 100644 --- a/e2e/assert.sh +++ b/e2e/assert.sh @@ -33,3 +33,12 @@ assert_fail() { exit 1 fi } + +assert_matches() { + local actual + actual="$(bash -c "$1")" + if [[ ! "$actual" =~ $2 ]]; then + echo "Expected '$2' to match '$actual'" + exit 1 + fi +} diff --git a/e2e/test_shims b/e2e/test_shims index 56a74b0ac..ba0a9919c 100755 --- a/e2e/test_shims +++ b/e2e/test_shims @@ -3,6 +3,11 @@ set -euo pipefail # shellcheck source-path=SCRIPTDIR source "$(dirname "$0")/assert.sh" +export PATH="/XSTARTX:$RTX_DATA_DIR/shims:/XENDX$PATH" rtx i node rtx reshim -assert "$RTX_DATA_DIR/shims/node -v" "v20.0.0" +assert "node -v" "v20.0.0" + +# should still have this prefix since rtx should only modify PATH +# starting at the shim directory +assert_matches "node -p 'process.env.PATH'" "^/XSTARTX:" diff --git a/src/main.rs b/src/main.rs index 858f77896..338c942da 100644 --- a/src/main.rs +++ b/src/main.rs @@ -46,6 +46,7 @@ mod install_context; mod lock_file; mod logger; mod migrate; +mod path_env; mod plugins; mod rand; mod runtime_symlinks; diff --git a/src/path_env.rs b/src/path_env.rs new file mode 100644 index 000000000..3d9edb9ca --- /dev/null +++ b/src/path_env.rs @@ -0,0 +1,122 @@ +use crate::dirs; +use itertools::Itertools; +use std::env::join_paths; +use std::ffi::OsString; +use std::fmt; +use std::fmt::{Display, Formatter}; +use std::path::PathBuf; + +pub struct PathEnv { + pre: Vec, + rtx: Vec, + post: Vec, + seen_shims: bool, +} + +impl PathEnv { + pub fn new() -> Self { + Self { + pre: Vec::new(), + rtx: Vec::new(), + post: Vec::new(), + seen_shims: false, + } + } + + pub fn add(&mut self, path: PathBuf) { + self.rtx.push(path); + } + + pub fn to_vec(&self) -> Vec { + let mut paths = self.pre.iter().chain(self.rtx.iter()).collect_vec(); + if self.seen_shims { + paths.push(&dirs::SHIMS); + } + paths.into_iter().chain(self.post.iter()).cloned().collect() + } + + pub fn join(&self) -> OsString { + join_paths(self.to_vec()).unwrap() + } +} + +impl Display for PathEnv { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.join().to_string_lossy()) + } +} + +impl FromIterator for PathEnv { + fn from_iter>(paths: T) -> Self { + let mut path_env = Self::new(); + + for path in paths { + if path_env.seen_shims { + path_env.post.push(path); + } else if path == *dirs::SHIMS { + path_env.seen_shims = true; + } else { + path_env.pre.push(path); + } + } + if !path_env.seen_shims { + path_env.post = path_env.pre; + path_env.pre = Vec::new(); + } + + path_env + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_path_env() { + let mut path_env = PathEnv::from_iter( + [ + "/before-1", + "/before-2", + "/before-3", + dirs::SHIMS.to_str().unwrap(), + "/after-1", + "/after-2", + "/after-3", + ] + .map(PathBuf::from), + ); + path_env.add("/1".into()); + path_env.add("/2".into()); + path_env.add("/3".into()); + assert_eq!( + path_env.to_string(), + format!( + "/before-1:/before-2:/before-3:/1:/2:/3:{}:/after-1:/after-2:/after-3", + dirs::SHIMS.to_str().unwrap() + ) + ); + } + + #[test] + fn test_path_env_no_rtx() { + let mut path_env = PathEnv::from_iter( + [ + "/before-1", + "/before-2", + "/before-3", + "/after-1", + "/after-2", + "/after-3", + ] + .map(PathBuf::from), + ); + path_env.add("/1".into()); + path_env.add("/2".into()); + path_env.add("/3".into()); + assert_eq!( + path_env.to_string(), + format!("/1:/2:/3:/before-1:/before-2:/before-3:/after-1:/after-2:/after-3") + ); + } +} diff --git a/src/toolset/mod.rs b/src/toolset/mod.rs index ee4bd84c0..5047e1f56 100644 --- a/src/toolset/mod.rs +++ b/src/toolset/mod.rs @@ -1,5 +1,4 @@ use std::collections::{BTreeMap, BTreeSet, HashMap}; -use std::env::join_paths; use std::fmt::{Display, Formatter}; use std::path::PathBuf; use std::sync::{Arc, Mutex}; @@ -20,6 +19,7 @@ pub use tool_version_request::ToolVersionRequest; use crate::config::Config; use crate::env; use crate::install_context::InstallContext; +use crate::path_env::PathEnv; use crate::plugins::PluginName; use crate::runtime_symlinks; use crate::shims; @@ -261,12 +261,18 @@ impl Toolset { .collect() } pub fn env_with_path(&self, config: &Config) -> BTreeMap { + let mut path_env = PathEnv::from_iter(env::PATH.clone()); + for p in config.path_dirs.clone() { + path_env.add(p); + } + for p in self.list_paths(config) { + path_env.add(p); + } let mut env = self.env(config); - let mut path_env = self.path_env(config); if let Some(path) = env.get("PATH") { - path_env = format!("{}:{}", path, path_env); + path_env.add(PathBuf::from(path)); } - env.insert("PATH".to_string(), path_env); + env.insert("PATH".to_string(), path_env.to_string()); env } pub fn env(&self, config: &Config) -> BTreeMap { @@ -298,13 +304,6 @@ impl Toolset { entries.extend(config.env.clone()); entries } - pub fn path_env(&self, config: &Config) -> String { - let installs = self.list_paths(config); - join_paths([config.path_dirs.clone(), installs, env::PATH.clone()].concat()) - .unwrap() - .to_string_lossy() - .into() - } pub fn list_paths(&self, config: &Config) -> Vec { self.list_current_installed_versions(config) .into_par_iter()