diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/package.json b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/package.json new file mode 100644 index 0000000000..f74441e9ab --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/package.json @@ -0,0 +1,6 @@ +{ + "name": "exec-relative-path-cwd", + "workspaces": [ + "packages/*" + ] +} diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/packages/app/package.json b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/packages/app/package.json new file mode 100644 index 0000000000..fd1f8f6386 --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/packages/app/package.json @@ -0,0 +1,3 @@ +{ + "name": "app" +} diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/setup.js b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/setup.js new file mode 100644 index 0000000000..331546320a --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/setup.js @@ -0,0 +1,14 @@ +const fs = require('fs'); + +function writeFakeNode(directory, message) { + fs.mkdirSync(directory, { recursive: true }); + fs.writeFileSync( + `${directory}/fake-node`, + `#!/usr/bin/env node\nconsole.log(${JSON.stringify(message)});\n`, + { mode: 0o755 }, + ); + fs.writeFileSync(`${directory}/fake-node.cmd`, '@node "%~dp0\\fake-node" %*\n'); +} + +writeFakeNode('packages/app/tools', 'resolved from package cwd'); +writeFakeNode('packages/shared-tools', 'resolved from parent relative PATH'); diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/snapshots.toml b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/snapshots.toml new file mode 100644 index 0000000000..58d9222323 --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/snapshots.toml @@ -0,0 +1,11 @@ +[[case]] +name = "command_exec_relative_path_cwd" +vp = "local" +skip-platforms = ["windows"] +comment = "A relative PATH entry must resolve against the selected package cwd, not the vp process cwd." +steps = [ + { argv = ["node", "setup.js"], snapshot = false, continue-on-failure = true }, + { argv = ["vp", "exec", "--filter", "app", "--", "fake-node"], envs = [["PATH", "./tools:${PATH}"]], comment = "relative PATH entry resolves from the selected package", continue-on-failure = true }, + { argv = ["vp", "exec", "--filter", "app", "--", "fake-node"], envs = [["PATH", "tools:${PATH}"]], comment = "plain relative PATH entry resolves from the selected package", continue-on-failure = true }, + { argv = ["vp", "exec", "--filter", "app", "--", "fake-node"], envs = [["PATH", "../shared-tools:${PATH}"]], comment = "parent relative PATH entry resolves from the selected package", continue-on-failure = true }, +] diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/snapshots/command_exec_relative_path_cwd.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/snapshots/command_exec_relative_path_cwd.md new file mode 100644 index 0000000000..cfb4bb529d --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_exec_relative_path_cwd/snapshots/command_exec_relative_path_cwd.md @@ -0,0 +1,30 @@ +# command_exec_relative_path_cwd + +A relative PATH entry must resolve against the selected package cwd, not the vp process cwd. + +## `node setup.js` + + +## `PATH=./tools:${PATH} vp exec --filter app -- fake-node` + +relative PATH entry resolves from the selected package + +``` +resolved from package cwd +``` + +## `PATH=tools:${PATH} vp exec --filter app -- fake-node` + +plain relative PATH entry resolves from the selected package + +``` +resolved from package cwd +``` + +## `PATH=../shared-tools:${PATH} vp exec --filter app -- fake-node` + +parent relative PATH entry resolves from the selected package + +``` +resolved from parent relative PATH +``` diff --git a/crates/vp_command/src/lib.rs b/crates/vp_command/src/lib.rs index d388688756..76b8ada3f1 100644 --- a/crates/vp_command/src/lib.rs +++ b/crates/vp_command/src/lib.rs @@ -10,6 +10,7 @@ use std::os::fd::{BorrowedFd, RawFd}; use std::{ collections::HashMap, ffi::{OsStr, OsString}, + path::Path, process::{ExitStatus, Stdio}, }; @@ -21,6 +22,44 @@ use vt_path::{AbsolutePath, AbsolutePathBuf, RelativePathBuf}; mod ps1_shim; +fn normalize_path_env( + path_env: &OsStr, + cwd: &AbsolutePath, +) -> Result { + std::env::join_paths(std::env::split_paths(path_env).map(|path| { + if path.starts_with("~") || !is_plain_relative_path(&path) { + path + } else { + cwd.as_path().join(path) + } + })) +} + +/// Return whether a PATH entry is an ordinary relative path that should be resolved against the +/// command cwd. This includes `tools`, `./tools`, and `../tools`. +/// +/// Windows drive-relative (`C:tools`) and root-relative (`\tools`) paths have distinct native +/// semantics. They are intentionally left unchanged, as are absolute drive and UNC paths. +fn is_plain_relative_path(path: &Path) -> bool { + #[cfg(windows)] + { + !path.has_root() + && path.components().next().is_some_and(|component| { + matches!( + component, + std::path::Component::CurDir + | std::path::Component::ParentDir + | std::path::Component::Normal(_) + ) + }) + } + + #[cfg(not(windows))] + { + !path.is_absolute() + } +} + /// Result of running a command with fspy tracking. #[derive(Debug)] pub struct FspyCommandResult { @@ -39,6 +78,7 @@ pub fn resolve_bin( path_env: Option<&OsStr>, cwd: impl AsRef, ) -> Result { + let cwd = cwd.as_ref(); let current_path; let path_env = if let Some(p) = path_env { p @@ -46,8 +86,14 @@ pub fn resolve_bin( current_path = std::env::var_os("PATH").unwrap_or_default(); ¤t_path }; - let path = which::which_in(bin_name, Some(path_env), cwd.as_ref()) + // `which` resolves relative PATH entries against the process cwd instead of the supplied + // command cwd. Commands are spawned with `cwd`, so resolve the entries the same way first; + // leave `~` entries for `which` to expand against the user's home directory. + let path_env = normalize_path_env(path_env, cwd) .map_err(|_| Error::CannotFindBinaryPath(bin_name.into()))?; + let path = which::which_in(bin_name, Some(&path_env), cwd) + .map_err(|_| Error::CannotFindBinaryPath(bin_name.into()))?; + let path = if is_plain_relative_path(&path) { cwd.as_path().join(path) } else { path }; AbsolutePathBuf::new(path).ok_or_else(|| Error::CannotFindBinaryPath(bin_name.into())) } @@ -399,6 +445,182 @@ mod tests { tempdir().expect("Failed to create temp directory") } + #[cfg(unix)] + fn create_executable(path: &std::path::Path) { + use std::{fs, os::unix::fs::PermissionsExt}; + + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(path, "#!/bin/sh\nexit 0\n").unwrap(); + let mut permissions = fs::metadata(path).unwrap().permissions(); + permissions.set_mode(0o755); + fs::set_permissions(path, permissions).unwrap(); + } + + #[cfg(unix)] + #[test] + fn test_resolve_bin_with_relative_path_entry() { + use std::path::PathBuf; + + let temp_dir = create_temp_dir(); + let cwd_path = temp_dir.path().canonicalize().unwrap(); + let cwd = AbsolutePathBuf::new(cwd_path.clone()).unwrap(); + let bin_dir = cwd_path.join("node_modules/.bin"); + let bin_path = bin_dir.join("fake-node"); + let fallback_bin_dir = cwd_path.join("fallback-bin"); + let fallback_bin_path = fallback_bin_dir.join("fake-node"); + + create_executable(&bin_path); + create_executable(&fallback_bin_path); + + let path_env = + std::env::join_paths([PathBuf::from("./node_modules/.bin"), fallback_bin_dir]).unwrap(); + let resolved = resolve_bin("fake-node", Some(&path_env), &cwd).unwrap(); + + assert_eq!(resolved.into_path_buf(), bin_path); + } + + #[cfg(unix)] + #[test] + fn test_resolve_bin_continues_after_missing_relative_path_entry() { + use std::path::PathBuf; + + let temp_dir = create_temp_dir(); + let cwd_path = temp_dir.path().canonicalize().unwrap(); + let cwd = AbsolutePathBuf::new(cwd_path.clone()).unwrap(); + let fallback_bin_dir = cwd_path.join("fallback-bin"); + let fallback_bin_path = fallback_bin_dir.join("fake-node"); + + create_executable(&fallback_bin_path); + + let path_env = + std::env::join_paths([PathBuf::from("./missing-bin"), fallback_bin_dir]).unwrap(); + let resolved = resolve_bin("fake-node", Some(&path_env), &cwd).unwrap(); + + assert_eq!(resolved.into_path_buf(), fallback_bin_path); + } + + #[cfg(unix)] + #[test] + fn test_resolve_bin_with_empty_path_entry() { + use std::path::PathBuf; + + let temp_dir = create_temp_dir(); + let cwd_path = temp_dir.path().canonicalize().unwrap(); + let cwd = AbsolutePathBuf::new(cwd_path.clone()).unwrap(); + let bin_path = cwd_path.join("fake-node"); + + create_executable(&bin_path); + + let path_env = std::env::join_paths([PathBuf::new()]).unwrap(); + let resolved = resolve_bin("fake-node", Some(&path_env), &cwd).unwrap(); + + assert_eq!(resolved.into_path_buf(), bin_path); + } + + #[cfg(unix)] + #[test] + fn test_normalize_path_env_preserves_tilde_entry() { + use std::path::PathBuf; + + let temp_dir = create_temp_dir(); + let cwd_path = temp_dir.path().canonicalize().unwrap(); + let cwd = AbsolutePathBuf::new(cwd_path).unwrap(); + let path_env = std::env::join_paths([PathBuf::from("~/bin")]).unwrap(); + + let normalized = normalize_path_env(&path_env, &cwd).unwrap(); + + assert_eq!( + std::env::split_paths(&normalized).collect::>(), + [PathBuf::from("~/bin")] + ); + } + + #[test] + fn test_normalize_path_env_resolves_plain_relative_entry_against_cwd() { + use std::path::PathBuf; + + let temp_dir = create_temp_dir(); + let cwd_path = temp_dir.path().canonicalize().unwrap(); + let cwd = AbsolutePathBuf::new(cwd_path.clone()).unwrap(); + let path_env = std::env::join_paths([PathBuf::from("node_modules/.bin")]).unwrap(); + + let normalized = normalize_path_env(&path_env, &cwd).unwrap(); + + assert_eq!( + std::env::split_paths(&normalized).collect::>(), + [cwd_path.join("node_modules/.bin")] + ); + } + + #[test] + fn test_normalize_path_env_resolves_common_relative_entries_and_keeps_absolute_entries() { + use std::path::PathBuf; + + let temp_dir = create_temp_dir(); + let cwd_path = temp_dir.path().canonicalize().unwrap(); + let cwd = AbsolutePathBuf::new(cwd_path.clone()).unwrap(); + let absolute_entry = cwd_path.join("absolute-bin"); + let path_env = std::env::join_paths([ + PathBuf::from("node_modules/.bin"), + PathBuf::from("./tools"), + PathBuf::from("../shared-bin"), + absolute_entry.clone(), + ]) + .unwrap(); + + let normalized = normalize_path_env(&path_env, &cwd).unwrap(); + + assert_eq!( + std::env::split_paths(&normalized).collect::>(), + [ + cwd_path.join("node_modules/.bin"), + cwd_path.join("./tools"), + cwd_path.join("../shared-bin"), + absolute_entry, + ] + ); + } + + #[cfg(windows)] + #[test] + fn test_normalize_path_env_preserves_windows_absolute_entries() { + use std::path::PathBuf; + + let temp_dir = create_temp_dir(); + let cwd_path = temp_dir.path().canonicalize().unwrap(); + let cwd = AbsolutePathBuf::new(cwd_path).unwrap(); + + for entry in [r"C:\tools\bin", r"\\server\share\bin"] { + let path_env = std::env::join_paths([PathBuf::from(entry)]).unwrap(); + let normalized = normalize_path_env(&path_env, &cwd).unwrap(); + + assert_eq!( + std::env::split_paths(&normalized).collect::>(), + [PathBuf::from(entry)] + ); + } + } + + #[cfg(windows)] + #[test] + fn test_normalize_path_env_does_not_rewrite_windows_special_relative_entries() { + use std::path::PathBuf; + + let temp_dir = create_temp_dir(); + let cwd_path = temp_dir.path().canonicalize().unwrap(); + let cwd = AbsolutePathBuf::new(cwd_path).unwrap(); + + for entry in [r"C:tools\bin", r"\tools\bin"] { + let path_env = std::env::join_paths([PathBuf::from(entry)]).unwrap(); + let normalized = normalize_path_env(&path_env, &cwd).unwrap(); + + assert_eq!( + std::env::split_paths(&normalized).collect::>(), + [PathBuf::from(entry)] + ); + } + } + mod run_command_tests { use super::*;