From 4fd8135f54af3430bb66e630e89b5a8e663353ed Mon Sep 17 00:00:00 2001 From: Benjamin Fahl Date: Fri, 24 Jul 2026 23:58:01 +0200 Subject: [PATCH 1/3] fix(shell): stop stacking duplicate PATH entries on every activation Every activation emitted "export PATH=:$PATH", prepending its bin directory to the live PATH without removing the entry the previous activation had put there. Each switch therefore left its predecessor behind, and in bash the cd hook runs from PROMPT_COMMAND -- after every command, not just after a cd -- so a shell sitting in a directory with a .php-version file grew PATH by one entry per prompt. PATH manipulation moves out of the emitted shell code: the wrapper runs pvm as a direct child, so the Rust process already sees the PATH its output will be eval'd into. fs::path_without_versions() drops every $PVM_DIR/versions entry and Shell::path bakes the resulting value in literally, which also lets deactivate() stop shelling out to tr/grep/paste. Two related guards: Bash::use_on_cd returns early unless $PWD actually changed, and wrapper_fn only prepends $PVM_DIR/bin when it is not already on PATH, so re-sourcing the rc file in a nested shell stops duplicating that entry too. pvm env now prints the default-version activation before the wrapper, because the literal snapshot cannot contain the $PVM_DIR/bin entry the wrapper adds one line later. Verified in bash, zsh and fish: six consecutive switches leave four PATH entries, and pvm use system restores the original PATH. --- CLAUDE.md | 4 +- src/commands/env.rs | 12 ++- src/commands/install.rs | 2 +- src/commands/use_cmd.rs | 4 +- src/fs.rs | 16 ++++ src/shell.rs | 173 +++++++++++++++++++++++++++++----------- tests/cli.rs | 59 +++++++++++++- 7 files changed, 215 insertions(+), 55 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 53a75cd..c0425a9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -52,7 +52,9 @@ The hard part: a child process cannot mutate its parent shell's environment. PVM 3. The Rust binary writes `export PATH=...; export PVM_MULTISHELL_PATH=...` to that tmpfile (with an exclusive file lock — see `fs::write_env_file_locked`). 4. The wrapper `eval`s the tmpfile and removes it. -The `Shell` trait (`Bash`, `Zsh`, `Fish`) emits the syntax for each shell; `detect_shell()` reads `$SHELL`. Bash and Zsh share the `posix_*` free functions; only `use_on_cd` differs. The `cd` hook reads `.php-version` and calls `pvm use --silent` automatically. `deactivate()` (used by `pvm use system`) writes an env-file snippet that clears `PVM_MULTISHELL_PATH` and filters `$PVM_DIR/versions` entries out of PATH. +The `Shell` trait (`Bash`, `Zsh`, `Fish`) emits the syntax for each shell; `detect_shell()` reads `$SHELL`. Bash and Zsh share the `posix_*` free functions; only `use_on_cd` differs. The `cd` hook reads `.php-version` and calls `pvm use --silent` automatically. `deactivate()` (used by `pvm use system`) writes an env-file snippet that clears `PVM_MULTISHELL_PATH` and restores the filtered PATH. + +PATH is never manipulated in shell code: `Shell::path` and `Shell::deactivate` take an already-filtered entry list from `fs::path_without_versions()` and bake the whole value in literally. Because the wrapper runs `pvm` as a direct child, the Rust process sees exactly the PATH its output will be eval'd into, so it can drop stale `$PVM_DIR/versions` entries itself. Emitting `PATH=:$PATH` instead would stack a duplicate on every activation — and the bash `cd` hook runs from `PROMPT_COMMAND`, i.e. after *every* command, so that grows without bound (hence the `__pvm_last_pwd` guard in `Bash::use_on_cd` and the `case`/`contains` guard around the `$PVM_DIR/bin` prepend in `wrapper_fn`). The activation tail (patch-update offer, `.php-version` prompts, env-file write) lives in `use_cmd::activate(version, ActivateOpts)`; both `pvm use` and the interactive `pvm ls` picker call it. `ActivateOpts.offer_save` distinguishes picker flows (offer to write `.php-version`) from explicit-argument flows (offer to update an existing differing file). diff --git a/src/commands/env.rs b/src/commands/env.rs index bad1104..00eb7c0 100644 --- a/src/commands/env.rs +++ b/src/commands/env.rs @@ -28,11 +28,12 @@ impl Env { }; println!("{}", s.set_env_var(PVM_DIR_VAR, &pvm_dir.to_string_lossy())); - println!("{}", s.wrapper_fn()); - println!("{}", s.use_on_cd()); // Activate the persisted default version ('pvm default') so every new - // shell starts on it instead of the system PHP. + // shell starts on it instead of the system PHP. This has to precede + // wrapper_fn: the PATH it emits is a literal snapshot of this process's + // PATH, which cannot contain the $PVM_DIR/bin entry the wrapper adds + // one line later. if let Some(version) = crate::fs::get_default_version()? { if crate::fs::is_version_installed(&version)? { let bin_dir = crate::fs::get_version_bin_dir(&version)?; @@ -40,7 +41,7 @@ impl Env { "{}", s.set_env_var(MULTISHELL_PATH_VAR, &bin_dir.to_string_lossy()) ); - println!("{}", s.path(&bin_dir)); + println!("{}", s.path(&bin_dir, &crate::fs::path_without_versions()?)); } else { // stderr so the eval'd stdout stays clean. eprintln!( @@ -50,6 +51,9 @@ impl Env { } } + println!("{}", s.wrapper_fn()); + println!("{}", s.use_on_cd()); + Ok(()) } } diff --git a/src/commands/install.rs b/src/commands/install.rs index aef3c37..6912d08 100644 --- a/src/commands/install.rs +++ b/src/commands/install.rs @@ -147,7 +147,7 @@ pub async fn execute_install_with( let bin_dir = crate::fs::get_version_bin_dir(&v)?; let s = crate::shell::detect_shell(); let export_str1 = s.set_env_var(MULTISHELL_PATH_VAR, &bin_dir.to_string_lossy()); - let export_str2 = s.path(&bin_dir); + let export_str2 = s.path(&bin_dir, &crate::fs::path_without_versions()?); let env_file = crate::fs::get_env_update_path()?; crate::fs::write_env_file_locked(&env_file, &format!("{}\n{}", export_str1, export_str2))?; diff --git a/src/commands/use_cmd.rs b/src/commands/use_cmd.rs index e132d0c..c22156a 100644 --- a/src/commands/use_cmd.rs +++ b/src/commands/use_cmd.rs @@ -204,7 +204,7 @@ fn save_question(version: &str, file_version: &Option, offer_save: bool) fn switch_to_system(quiet: bool) -> Result<()> { let s = shell::detect_shell(); let env_file = fs::get_env_update_path()?; - fs::write_env_file_locked(&env_file, &s.deactivate(&fs::get_versions_dir()?))?; + fs::write_env_file_locked(&env_file, &s.deactivate(&fs::path_without_versions()?))?; if !quiet { eprintln!("{} Switched to system PHP", "✓".green()); } @@ -298,7 +298,7 @@ pub(crate) async fn activate(mut version: String, opts: ActivateOpts) -> Result< // These evaluate in the user's shell hook via wrapper let export_str1 = s.set_env_var(MULTISHELL_PATH_VAR, &bin_dir.to_string_lossy()); - let export_str2 = s.path(&bin_dir); + let export_str2 = s.path(&bin_dir, &fs::path_without_versions()?); let env_file = fs::get_env_update_path()?; fs::write_env_file_locked(&env_file, &format!("{}\n{}", export_str1, export_str2))?; diff --git a/src/fs.rs b/src/fs.rs index 7ca7408..6e88801 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -87,6 +87,22 @@ pub fn get_current_version() -> String { "system".to_string() } +/// The inherited PATH with every `$PVM_DIR/versions` entry dropped, as a list +/// of plain strings. +/// +/// The wrapper runs `pvm` as a direct child of the shell, so this process sees +/// exactly the PATH the emitted snippet will be eval'd into. Filtering here — +/// instead of prepending in the shell — is what keeps repeated activations +/// (every `cd` into a `.php-version` directory) from stacking duplicates. +pub fn path_without_versions() -> Result> { + let versions_dir = get_versions_dir()?; + let path = std::env::var_os("PATH").unwrap_or_default(); + Ok(std::env::split_paths(&path) + .filter(|p| !p.starts_with(&versions_dir)) + .map(|p| p.to_string_lossy().into_owned()) + .collect()) +} + pub fn get_env_update_path() -> Result { if let Ok(env_path) = std::env::var("PVM_ENV_UPDATE_PATH") { return Ok(PathBuf::from(env_path)); diff --git a/src/shell.rs b/src/shell.rs index 5a05eaa..089616d 100644 --- a/src/shell.rs +++ b/src/shell.rs @@ -2,13 +2,19 @@ use crate::constants::{ENV_UPDATE_FILE, PVM_DIR_VAR}; use std::path::Path; pub trait Shell { - fn path(&self, path: &Path) -> String; + /// Emit a PATH assignment putting `bin_dir` in front of `rest`. + /// + /// `rest` is the caller's already-filtered PATH (see + /// `fs::path_without_versions`), not `$PATH`: the full value is baked in + /// literally so an activation replaces the previous pvm entry instead of + /// stacking a duplicate on top of it. + fn path(&self, bin_dir: &Path, rest: &[String]) -> String; fn set_env_var(&self, name: &str, value: &str) -> String; fn use_on_cd(&self) -> String; fn wrapper_fn(&self) -> String; /// Emit commands that drop every pvm-managed entry from PATH and clear /// the multishell marker, returning the shell to the system PHP. - fn deactivate(&self, versions_dir: &Path) -> String; + fn deactivate(&self, rest: &[String]) -> String; } /// Quote a string for POSIX shells (bash/zsh) by wrapping it in single quotes @@ -42,13 +48,23 @@ fn fish_single_quote(value: &str) -> String { out } -fn posix_path(path: &Path) -> String { +fn posix_path(bin_dir: &Path, rest: &[String]) -> String { format!( - "export PATH={}:\"$PATH\"", - posix_single_quote(&path.display().to_string()) + "export PATH={}", + posix_single_quote(&join_path(bin_dir, rest)) ) } +/// Join `bin_dir` and `rest` into a `:`-separated PATH value. Empty entries are +/// dropped: a stray `::` or trailing `:` means "current directory" to the shell. +fn join_path(bin_dir: &Path, rest: &[String]) -> String { + std::iter::once(bin_dir.display().to_string()) + .chain(rest.iter().cloned()) + .filter(|p| !p.is_empty()) + .collect::>() + .join(":") +} + fn posix_set_env_var(name: &str, value: &str) -> String { format!("export {}={}", name, posix_single_quote(value)) } @@ -56,15 +72,18 @@ fn posix_set_env_var(name: &str, value: &str) -> String { fn posix_wrapper_fn() -> String { format!( " -export PATH=\"${{{}}}/bin:$PATH\" +case \":$PATH:\" in + *\":${{{d}}}/bin:\"*) ;; + *) export PATH=\"${{{d}}}/bin:$PATH\" ;; +esac pvm() {{ local command=$1 if [[ \"$command\" == \"env\" ]]; then command pvm \"$@\" else - if [[ -n \"${{{}}}\" && -d \"${{{}}}\" ]]; then - local env_file=\"${{{}}}/{}_$$_${{RANDOM}}${{RANDOM}}_$(date +%s)\" + if [[ -n \"${{{d}}}\" && -d \"${{{d}}}\" ]]; then + local env_file=\"${{{d}}}/{f}_$$_${{RANDOM}}${{RANDOM}}_$(date +%s)\" [[ -f \"$env_file\" ]] && command rm -f \"$env_file\" 2>/dev/null PVM_ENV_UPDATE_PATH=\"$env_file\" command pvm \"$@\" local exit_code=$? @@ -79,22 +98,23 @@ pvm() {{ fi }} ", - PVM_DIR_VAR, PVM_DIR_VAR, PVM_DIR_VAR, PVM_DIR_VAR, ENV_UPDATE_FILE + d = PVM_DIR_VAR, + f = ENV_UPDATE_FILE ) } -fn posix_deactivate(versions_dir: &Path) -> String { +fn posix_deactivate(rest: &[String]) -> String { format!( - "export PVM_MULTISHELL_PATH=''\nexport PATH=\"$(printf '%s' \"$PATH\" | tr ':' '\\n' | grep -vF {} | paste -sd : -)\"", - posix_single_quote(&versions_dir.display().to_string()) + "export PVM_MULTISHELL_PATH=''\nexport PATH={}", + posix_single_quote(&rest.join(":")) ) } pub struct Bash; impl Shell for Bash { - fn path(&self, path: &Path) -> String { - posix_path(path) + fn path(&self, bin_dir: &Path, rest: &[String]) -> String { + posix_path(bin_dir, rest) } fn set_env_var(&self, name: &str, value: &str) -> String { @@ -102,8 +122,13 @@ impl Shell for Bash { } fn use_on_cd(&self) -> String { + // Bash has no chpwd hook, so this runs from PROMPT_COMMAND — i.e. after + // every command, not just after a cd. Bail out unless the directory + // actually changed, or every prompt would fork a pvm process. " _pvm_cd_hook() { + [[ \"$PWD\" == \"${__pvm_last_pwd-}\" ]] && return + __pvm_last_pwd=\"$PWD\" if [[ -f .php-version ]]; then pvm use --silent \"$(cat .php-version)\" || true fi @@ -121,16 +146,16 @@ fi posix_wrapper_fn() } - fn deactivate(&self, versions_dir: &Path) -> String { - posix_deactivate(versions_dir) + fn deactivate(&self, rest: &[String]) -> String { + posix_deactivate(rest) } } pub struct Zsh; impl Shell for Zsh { - fn path(&self, path: &Path) -> String { - posix_path(path) + fn path(&self, bin_dir: &Path, rest: &[String]) -> String { + posix_path(bin_dir, rest) } fn set_env_var(&self, name: &str, value: &str) -> String { @@ -154,18 +179,29 @@ add-zsh-hook chpwd _pvm_cd_hook posix_wrapper_fn() } - fn deactivate(&self, versions_dir: &Path) -> String { - posix_deactivate(versions_dir) + fn deactivate(&self, rest: &[String]) -> String { + posix_deactivate(rest) } } pub struct Fish; +/// Quote each entry separately: PATH is a list in fish, not a `:`-joined string. +fn fish_path_words(entries: impl Iterator) -> String { + entries + .filter(|p| !p.is_empty()) + .map(|p| fish_single_quote(&p)) + .collect::>() + .join(" ") +} + impl Shell for Fish { - fn path(&self, path: &Path) -> String { + fn path(&self, bin_dir: &Path, rest: &[String]) -> String { format!( - "set -gx PATH {} $PATH", - fish_single_quote(&path.display().to_string()) + "set -gx PATH {}", + fish_path_words( + std::iter::once(bin_dir.display().to_string()).chain(rest.iter().cloned()) + ) ) } @@ -184,25 +220,27 @@ end .to_string() } - fn deactivate(&self, versions_dir: &Path) -> String { + fn deactivate(&self, rest: &[String]) -> String { format!( - "set -gx PVM_MULTISHELL_PATH ''\nset -gx PATH (string match -v -- {} $PATH)", - fish_single_quote(&format!("{}*", versions_dir.display())) + "set -gx PVM_MULTISHELL_PATH ''\nset -gx PATH {}", + fish_path_words(rest.iter().cloned()) ) } fn wrapper_fn(&self) -> String { format!( " -set -gx PATH \"${}/bin\" $PATH +if not contains \"${d}/bin\" $PATH + set -gx PATH \"${d}/bin\" $PATH +end function pvm set command $argv[1] if test \"$command\" = \"env\" command pvm $argv else - if test -n \"${}\"; and test -d \"${}\" - set env_file \"${}/{}_$fish_pid\"_(random)(random)_(date +%s) + if test -n \"${d}\"; and test -d \"${d}\" + set env_file \"${d}/{f}_$fish_pid\"_(random)(random)_(date +%s) if test -f \"$env_file\" command rm -f \"$env_file\" &>/dev/null end @@ -219,7 +257,8 @@ function pvm end end ", - PVM_DIR_VAR, PVM_DIR_VAR, PVM_DIR_VAR, PVM_DIR_VAR, ENV_UPDATE_FILE + d = PVM_DIR_VAR, + f = ENV_UPDATE_FILE ) } } @@ -239,16 +278,41 @@ pub fn detect_shell() -> Box { mod tests { use super::*; + fn rest() -> Vec { + vec!["/usr/bin".to_string(), "/bin".to_string()] + } + #[test] fn test_bash_path_generation() { let bash = Bash; let path = std::path::Path::new("/home/user/.local/share/pvm/versions/8.3.1/bin"); assert_eq!( - bash.path(path), - "export PATH='/home/user/.local/share/pvm/versions/8.3.1/bin':\"$PATH\"" + bash.path(path, &rest()), + "export PATH='/home/user/.local/share/pvm/versions/8.3.1/bin:/usr/bin:/bin'" ); } + #[test] + fn test_posix_path_drops_empty_entries() { + // A trailing/duplicated ':' in PATH means "current directory" — never + // let one survive into the emitted assignment. + let bash = Bash; + let out = bash.path( + std::path::Path::new("/pvm/versions/8.3.1/bin"), + &["".to_string(), "/usr/bin".to_string(), "".to_string()], + ); + assert_eq!(out, "export PATH='/pvm/versions/8.3.1/bin:/usr/bin'"); + } + + #[test] + fn test_path_does_not_reference_shell_path_var() { + // The whole point of baking the value in: re-activating must replace the + // previous pvm entry, not prepend on top of the live $PATH again. + let bash = Bash; + let out = bash.path(std::path::Path::new("/pvm/versions/8.3.1/bin"), &rest()); + assert!(!out.contains("$PATH"), "got: {}", out); + } + #[test] fn test_bash_set_env() { let bash = Bash; @@ -272,8 +336,8 @@ mod tests { let zsh = Zsh; let path = std::path::Path::new("/home/user/.local/share/pvm/versions/8.3.1/bin"); assert_eq!( - zsh.path(path), - "export PATH='/home/user/.local/share/pvm/versions/8.3.1/bin':\"$PATH\"" + zsh.path(path, &rest()), + "export PATH='/home/user/.local/share/pvm/versions/8.3.1/bin:/usr/bin:/bin'" ); } @@ -282,8 +346,8 @@ mod tests { let fish = Fish; let path = std::path::Path::new("/home/user/.local/share/pvm/versions/8.3.1/bin"); assert_eq!( - fish.path(path), - "set -gx PATH '/home/user/.local/share/pvm/versions/8.3.1/bin' $PATH" + fish.path(path, &rest()), + "set -gx PATH '/home/user/.local/share/pvm/versions/8.3.1/bin' '/usr/bin' '/bin'" ); } @@ -294,19 +358,38 @@ mod tests { } #[test] - fn test_bash_deactivate_filters_versions_dir() { + fn test_bash_deactivate_restores_filtered_path() { let bash = Bash; - let out = bash.deactivate(std::path::Path::new("/data/pvm/versions")); - assert!(out.contains("export PVM_MULTISHELL_PATH=''")); - assert!(out.contains("grep -vF '/data/pvm/versions'")); - assert!(out.contains("paste -sd : -")); + let out = bash.deactivate(&rest()); + assert_eq!( + out, + "export PVM_MULTISHELL_PATH=''\nexport PATH='/usr/bin:/bin'" + ); } #[test] - fn test_fish_deactivate_filters_versions_dir() { + fn test_fish_deactivate_restores_filtered_path() { let fish = Fish; - let out = fish.deactivate(std::path::Path::new("/data/pvm/versions")); - assert!(out.contains("set -gx PVM_MULTISHELL_PATH ''")); - assert!(out.contains("string match -v -- '/data/pvm/versions*' $PATH")); + let out = fish.deactivate(&rest()); + assert_eq!( + out, + "set -gx PVM_MULTISHELL_PATH ''\nset -gx PATH '/usr/bin' '/bin'" + ); + } + + #[test] + fn test_bash_cd_hook_skips_unchanged_pwd() { + // PROMPT_COMMAND fires after every command; without this guard each + // prompt in a .php-version directory would fork a pvm process. + let out = Bash.use_on_cd(); + assert!(out.contains("[[ \"$PWD\" == \"${__pvm_last_pwd-}\" ]] && return")); + } + + #[test] + fn test_wrapper_fn_guards_against_duplicate_bin_entry() { + // Re-sourcing the rc file (nested shells, `exec bash`) must not stack + // another $PVM_DIR/bin entry onto PATH. + assert!(Bash.wrapper_fn().contains("case \":$PATH:\" in")); + assert!(Fish.wrapper_fn().contains("if not contains")); } } diff --git a/tests/cli.rs b/tests/cli.rs index 9705d32..671a150 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -270,11 +270,13 @@ fn test_prune_non_tty_requires_yes() { fn test_use_system_writes_deactivation_env_file() { let temp_dir = tempfile::tempdir().unwrap(); let env_file = temp_dir.path().join("custom_env_update"); + let active_bin = temp_dir.path().join("versions/8.9.6/bin"); let mut cmd = assert_cmd::cargo::cargo_bin_cmd!("pvm"); cmd.env("PVM_DIR", temp_dir.path()); cmd.env("PVM_ENV_UPDATE_PATH", &env_file); cmd.env("SHELL", "/bin/bash"); + cmd.env("PATH", format!("{}:/usr/bin:/bin", active_bin.display())); cmd.arg("use").arg("system"); cmd.assert() .success() @@ -282,8 +284,61 @@ fn test_use_system_writes_deactivation_env_file() { let content = std::fs::read_to_string(env_file).unwrap(); assert!(content.contains("export PVM_MULTISHELL_PATH=''")); - assert!(content.contains("grep -vF")); - assert!(content.contains("versions")); + assert!( + content.contains("export PATH='/usr/bin:/bin'"), + "{}", + content + ); + assert!(!content.contains("versions"), "{}", content); +} + +/// Regression: every activation used to prepend its bin dir to the live $PATH, +/// so the cd-hook stacked a fresh duplicate on every switch (in bash: on every +/// prompt) until PATH was hundreds of entries long. +#[test] +fn test_use_does_not_stack_duplicate_path_entries() { + let temp_dir = tempfile::tempdir().unwrap(); + seed_installed_version(temp_dir.path(), "8.9.6", &["cli"]); + seed_installed_version(temp_dir.path(), "8.8.1", &["cli"]); + let bin_896 = temp_dir.path().join("versions/8.9.6/bin"); + let bin_881 = temp_dir.path().join("versions/8.8.1/bin"); + let env_file = temp_dir.path().join("custom_env_update"); + + // Simulate a shell that already has 8.9.6 active twice over plus a stale + // 8.8.1 entry, i.e. the state the old code kept growing. + let dirty_path = format!( + "{}:{}:{}:/usr/bin:/bin", + bin_896.display(), + bin_896.display(), + bin_881.display() + ); + + let mut cmd = assert_cmd::cargo::cargo_bin_cmd!("pvm"); + cmd.env("PVM_DIR", temp_dir.path()); + cmd.env("PVM_ENV_UPDATE_PATH", &env_file); + cmd.env("SHELL", "/bin/bash"); + cmd.env("PATH", &dirty_path); + cmd.env_remove("PVM_MULTISHELL_PATH"); + cmd.arg("use").arg("--silent").arg("8.9.6"); + cmd.assert().success(); + + let content = std::fs::read_to_string(&env_file).unwrap(); + let path_line = content + .lines() + .find(|l| l.starts_with("export PATH=")) + .expect("no PATH line"); + assert_eq!( + path_line.matches(&*bin_896.to_string_lossy()).count(), + 1, + "active version listed more than once: {}", + path_line + ); + assert!( + !path_line.contains(&*bin_881.to_string_lossy()), + "stale version survived: {}", + path_line + ); + assert!(path_line.contains("/usr/bin:/bin"), "{}", path_line); } #[test] From 61b16e1a9378c768bdf8560e3f5ba18762d8822f Mon Sep 17 00:00:00 2001 From: Benjamin Fahl Date: Fri, 24 Jul 2026 23:58:01 +0200 Subject: [PATCH 2/3] fix(ci): build and publish the linux-aarch64 release asset get_target_triple() resolves linux-aarch64 and install.sh maps uname -m to pvm-linux-aarch64.tar.gz, but the build matrix only produced linux-x86_64 and the two macOS targets. On ARM Linux both the install script and pvm self-update fetched an asset that was never uploaded and failed with a 404. Builds natively on the ubuntu-24.04-arm runner, so no cross toolchain is needed. --- .github/workflows/release.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index ad1397c..e1280fe 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -127,6 +127,12 @@ jobs: target: x86_64-unknown-linux-gnu artifact_name: pvm asset_name: pvm-linux-x86_64 + # install.sh and self-update both resolve this asset on ARM Linux + # (get_target_triple returns linux-aarch64); without it they 404. + - os: ubuntu-24.04-arm + target: aarch64-unknown-linux-gnu + artifact_name: pvm + asset_name: pvm-linux-aarch64 - os: macos-latest target: aarch64-apple-darwin artifact_name: pvm From 8c374611f2988afbf642cb955ee51748c159d3db Mon Sep 17 00:00:00 2001 From: Benjamin Fahl Date: Sat, 25 Jul 2026 00:07:07 +0200 Subject: [PATCH 3/3] refactor(shell): fold the PATH renderers into one entry helper posix and fish each had their own "drop empty entries, render them" pass, and posix_deactivate had neither -- it joined the list raw, so an inherited "::" survived deactivation while activation stripped it. One path_entries() feeding a per-shell assignment renderer removes the duplication and the asymmetry, and taking &str instead of an owned iterator drops a String allocation per entry on the fish paths. Test PATHs move to synthetic /opt/tool-* entries so no assertion depends on how the host or CI runner lays out its directories, and the two snippet guards are asserted by relative position (guard before the thing it guards) rather than by a verbatim copy of the source line, which broke on any reformatting. Both were confirmed to fail with their guard removed. --- src/commands/env.rs | 6 +- src/fs.rs | 9 +-- src/shell.rs | 136 +++++++++++++++++++++++++++----------------- tests/cli.rs | 16 ++++-- 4 files changed, 98 insertions(+), 69 deletions(-) diff --git a/src/commands/env.rs b/src/commands/env.rs index 00eb7c0..2ab4fef 100644 --- a/src/commands/env.rs +++ b/src/commands/env.rs @@ -30,10 +30,8 @@ impl Env { println!("{}", s.set_env_var(PVM_DIR_VAR, &pvm_dir.to_string_lossy())); // Activate the persisted default version ('pvm default') so every new - // shell starts on it instead of the system PHP. This has to precede - // wrapper_fn: the PATH it emits is a literal snapshot of this process's - // PATH, which cannot contain the $PVM_DIR/bin entry the wrapper adds - // one line later. + // shell starts on it instead of the system PHP. Must precede wrapper_fn: + // its PATH is a snapshot taken before the wrapper adds $PVM_DIR/bin. if let Some(version) = crate::fs::get_default_version()? { if crate::fs::is_version_installed(&version)? { let bin_dir = crate::fs::get_version_bin_dir(&version)?; diff --git a/src/fs.rs b/src/fs.rs index 6e88801..a7dfc01 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -87,13 +87,8 @@ pub fn get_current_version() -> String { "system".to_string() } -/// The inherited PATH with every `$PVM_DIR/versions` entry dropped, as a list -/// of plain strings. -/// -/// The wrapper runs `pvm` as a direct child of the shell, so this process sees -/// exactly the PATH the emitted snippet will be eval'd into. Filtering here — -/// instead of prepending in the shell — is what keeps repeated activations -/// (every `cd` into a `.php-version` directory) from stacking duplicates. +/// The inherited PATH with every `$PVM_DIR/versions` entry dropped — the base +/// an activation prepends to, so repeated switches cannot stack duplicates. pub fn path_without_versions() -> Result> { let versions_dir = get_versions_dir()?; let path = std::env::var_os("PATH").unwrap_or_default(); diff --git a/src/shell.rs b/src/shell.rs index 089616d..3347a91 100644 --- a/src/shell.rs +++ b/src/shell.rs @@ -2,12 +2,8 @@ use crate::constants::{ENV_UPDATE_FILE, PVM_DIR_VAR}; use std::path::Path; pub trait Shell { - /// Emit a PATH assignment putting `bin_dir` in front of `rest`. - /// - /// `rest` is the caller's already-filtered PATH (see - /// `fs::path_without_versions`), not `$PATH`: the full value is baked in - /// literally so an activation replaces the previous pvm entry instead of - /// stacking a duplicate on top of it. + /// Emit a PATH assignment of `bin_dir` followed by `rest`, an already + /// pvm-free entry list from `fs::path_without_versions` (never `$PATH`). fn path(&self, bin_dir: &Path, rest: &[String]) -> String; fn set_env_var(&self, name: &str, value: &str) -> String; fn use_on_cd(&self) -> String; @@ -48,21 +44,28 @@ fn fish_single_quote(value: &str) -> String { out } -fn posix_path(bin_dir: &Path, rest: &[String]) -> String { - format!( - "export PATH={}", - posix_single_quote(&join_path(bin_dir, rest)) - ) +/// `head` (if any) followed by `rest`, empty entries dropped: a stray `::` or a +/// trailing `:` in the inherited PATH means "current directory" to the shell. +fn path_entries<'a>(head: Option<&'a str>, rest: &'a [String]) -> impl Iterator { + head.into_iter() + .chain(rest.iter().map(String::as_str)) + .filter(|p| !p.is_empty()) } -/// Join `bin_dir` and `rest` into a `:`-separated PATH value. Empty entries are -/// dropped: a stray `::` or trailing `:` means "current directory" to the shell. -fn join_path(bin_dir: &Path, rest: &[String]) -> String { - std::iter::once(bin_dir.display().to_string()) - .chain(rest.iter().cloned()) - .filter(|p| !p.is_empty()) - .collect::>() - .join(":") +fn posix_path_assignment<'a>(entries: impl Iterator) -> String { + let joined = entries.collect::>().join(":"); + format!("export PATH={}", posix_single_quote(&joined)) +} + +/// Quote each entry separately: PATH is a list in fish, not a `:`-joined string. +fn fish_path_assignment<'a>(entries: impl Iterator) -> String { + let words: Vec = entries.map(fish_single_quote).collect(); + format!("set -gx PATH {}", words.join(" ")) +} + +fn posix_path(bin_dir: &Path, rest: &[String]) -> String { + let bin = bin_dir.to_string_lossy(); + posix_path_assignment(path_entries(Some(bin.as_ref()), rest)) } fn posix_set_env_var(name: &str, value: &str) -> String { @@ -105,8 +108,8 @@ pvm() {{ fn posix_deactivate(rest: &[String]) -> String { format!( - "export PVM_MULTISHELL_PATH=''\nexport PATH={}", - posix_single_quote(&rest.join(":")) + "export PVM_MULTISHELL_PATH=''\n{}", + posix_path_assignment(path_entries(None, rest)) ) } @@ -186,23 +189,10 @@ add-zsh-hook chpwd _pvm_cd_hook pub struct Fish; -/// Quote each entry separately: PATH is a list in fish, not a `:`-joined string. -fn fish_path_words(entries: impl Iterator) -> String { - entries - .filter(|p| !p.is_empty()) - .map(|p| fish_single_quote(&p)) - .collect::>() - .join(" ") -} - impl Shell for Fish { fn path(&self, bin_dir: &Path, rest: &[String]) -> String { - format!( - "set -gx PATH {}", - fish_path_words( - std::iter::once(bin_dir.display().to_string()).chain(rest.iter().cloned()) - ) - ) + let bin = bin_dir.to_string_lossy(); + fish_path_assignment(path_entries(Some(bin.as_ref()), rest)) } fn set_env_var(&self, name: &str, value: &str) -> String { @@ -222,8 +212,8 @@ end fn deactivate(&self, rest: &[String]) -> String { format!( - "set -gx PVM_MULTISHELL_PATH ''\nset -gx PATH {}", - fish_path_words(rest.iter().cloned()) + "set -gx PVM_MULTISHELL_PATH ''\n{}", + fish_path_assignment(path_entries(None, rest)) ) } @@ -278,8 +268,10 @@ pub fn detect_shell() -> Box { mod tests { use super::*; + /// Stand-in for the non-pvm part of PATH. Deliberately synthetic: nothing + /// here may depend on how the host or CI runner lays out its directories. fn rest() -> Vec { - vec!["/usr/bin".to_string(), "/bin".to_string()] + vec!["/opt/tool-a/bin".to_string(), "/opt/tool-b/bin".to_string()] } #[test] @@ -288,7 +280,7 @@ mod tests { let path = std::path::Path::new("/home/user/.local/share/pvm/versions/8.3.1/bin"); assert_eq!( bash.path(path, &rest()), - "export PATH='/home/user/.local/share/pvm/versions/8.3.1/bin:/usr/bin:/bin'" + "export PATH='/home/user/.local/share/pvm/versions/8.3.1/bin:/opt/tool-a/bin:/opt/tool-b/bin'" ); } @@ -299,9 +291,24 @@ mod tests { let bash = Bash; let out = bash.path( std::path::Path::new("/pvm/versions/8.3.1/bin"), - &["".to_string(), "/usr/bin".to_string(), "".to_string()], + &[ + "".to_string(), + "/opt/tool-a/bin".to_string(), + "".to_string(), + ], + ); + assert_eq!(out, "export PATH='/pvm/versions/8.3.1/bin:/opt/tool-a/bin'"); + } + + #[test] + fn test_deactivate_drops_empty_entries() { + // Same guarantee on the way out: 'pvm use system' must not hand back a + // PATH whose empty entry silently means the current directory. + let out = Bash.deactivate(&["/opt/tool-a/bin".to_string(), "".to_string()]); + assert_eq!( + out, + "export PVM_MULTISHELL_PATH=''\nexport PATH='/opt/tool-a/bin'" ); - assert_eq!(out, "export PATH='/pvm/versions/8.3.1/bin:/usr/bin'"); } #[test] @@ -337,7 +344,7 @@ mod tests { let path = std::path::Path::new("/home/user/.local/share/pvm/versions/8.3.1/bin"); assert_eq!( zsh.path(path, &rest()), - "export PATH='/home/user/.local/share/pvm/versions/8.3.1/bin:/usr/bin:/bin'" + "export PATH='/home/user/.local/share/pvm/versions/8.3.1/bin:/opt/tool-a/bin:/opt/tool-b/bin'" ); } @@ -347,7 +354,7 @@ mod tests { let path = std::path::Path::new("/home/user/.local/share/pvm/versions/8.3.1/bin"); assert_eq!( fish.path(path, &rest()), - "set -gx PATH '/home/user/.local/share/pvm/versions/8.3.1/bin' '/usr/bin' '/bin'" + "set -gx PATH '/home/user/.local/share/pvm/versions/8.3.1/bin' '/opt/tool-a/bin' '/opt/tool-b/bin'" ); } @@ -363,7 +370,7 @@ mod tests { let out = bash.deactivate(&rest()); assert_eq!( out, - "export PVM_MULTISHELL_PATH=''\nexport PATH='/usr/bin:/bin'" + "export PVM_MULTISHELL_PATH=''\nexport PATH='/opt/tool-a/bin:/opt/tool-b/bin'" ); } @@ -373,23 +380,46 @@ mod tests { let out = fish.deactivate(&rest()); assert_eq!( out, - "set -gx PVM_MULTISHELL_PATH ''\nset -gx PATH '/usr/bin' '/bin'" + "set -gx PVM_MULTISHELL_PATH ''\nset -gx PATH '/opt/tool-a/bin' '/opt/tool-b/bin'" ); } + /// Index of `needle`, or a failure naming the snippet that lacked it. + fn offset_of(haystack: &str, needle: &str) -> usize { + haystack + .find(needle) + .unwrap_or_else(|| panic!("{:?} missing from:\n{}", needle, haystack)) + } + #[test] fn test_bash_cd_hook_skips_unchanged_pwd() { - // PROMPT_COMMAND fires after every command; without this guard each - // prompt in a .php-version directory would fork a pvm process. - let out = Bash.use_on_cd(); - assert!(out.contains("[[ \"$PWD\" == \"${__pvm_last_pwd-}\" ]] && return")); + // PROMPT_COMMAND fires after every command, not just after a cd, so the + // hook has to compare $PWD and bail before it ever reads .php-version — + // otherwise every prompt forks a pvm process. + let hook = Bash.use_on_cd(); + assert!( + offset_of(&hook, "__pvm_last_pwd") < offset_of(&hook, ".php-version"), + "cd hook reads .php-version before the $PWD guard:\n{}", + hook + ); } #[test] fn test_wrapper_fn_guards_against_duplicate_bin_entry() { // Re-sourcing the rc file (nested shells, `exec bash`) must not stack - // another $PVM_DIR/bin entry onto PATH. - assert!(Bash.wrapper_fn().contains("case \":$PATH:\" in")); - assert!(Fish.wrapper_fn().contains("if not contains")); + // another $PVM_DIR/bin entry onto PATH, so the prepend has to sit behind + // a membership test rather than run unconditionally. + let bash = Bash.wrapper_fn(); + assert!( + offset_of(&bash, "case \":$PATH:\"") < offset_of(&bash, "/bin:$PATH"), + "bash prepends $PVM_DIR/bin unguarded:\n{}", + bash + ); + let fish = Fish.wrapper_fn(); + assert!( + offset_of(&fish, "if not contains") < offset_of(&fish, "set -gx PATH"), + "fish prepends $PVM_DIR/bin unguarded:\n{}", + fish + ); } } diff --git a/tests/cli.rs b/tests/cli.rs index 671a150..3f7a9a9 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1,5 +1,10 @@ use predicates::prelude::*; +/// The non-pvm part of a fabricated PATH. Deliberately synthetic so PATH +/// assertions never depend on how the host or CI runner is laid out; pvm itself +/// never resolves a binary through PATH, so these need not exist. +const OTHER_PATH: &str = "/opt/tool-a/bin:/opt/tool-b/bin"; + /// Seed the remote version cache so commands that hit the network resolve /// entirely offline. Mirrors the cache filename scheme in network.rs. fn seed_remote_cache(pvm_dir: &std::path::Path, versions: &[(&str, &[&str])]) { @@ -276,7 +281,7 @@ fn test_use_system_writes_deactivation_env_file() { cmd.env("PVM_DIR", temp_dir.path()); cmd.env("PVM_ENV_UPDATE_PATH", &env_file); cmd.env("SHELL", "/bin/bash"); - cmd.env("PATH", format!("{}:/usr/bin:/bin", active_bin.display())); + cmd.env("PATH", format!("{}:{}", active_bin.display(), OTHER_PATH)); cmd.arg("use").arg("system"); cmd.assert() .success() @@ -285,7 +290,7 @@ fn test_use_system_writes_deactivation_env_file() { let content = std::fs::read_to_string(env_file).unwrap(); assert!(content.contains("export PVM_MULTISHELL_PATH=''")); assert!( - content.contains("export PATH='/usr/bin:/bin'"), + content.contains(&format!("export PATH='{}'", OTHER_PATH)), "{}", content ); @@ -307,10 +312,11 @@ fn test_use_does_not_stack_duplicate_path_entries() { // Simulate a shell that already has 8.9.6 active twice over plus a stale // 8.8.1 entry, i.e. the state the old code kept growing. let dirty_path = format!( - "{}:{}:{}:/usr/bin:/bin", + "{}:{}:{}:{}", bin_896.display(), bin_896.display(), - bin_881.display() + bin_881.display(), + OTHER_PATH ); let mut cmd = assert_cmd::cargo::cargo_bin_cmd!("pvm"); @@ -338,7 +344,7 @@ fn test_use_does_not_stack_duplicate_path_entries() { "stale version survived: {}", path_line ); - assert!(path_line.contains("/usr/bin:/bin"), "{}", path_line); + assert!(path_line.contains(OTHER_PATH), "{}", path_line); } #[test]