fix(command): resolve relative PATH entries against cwd - #2350
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
b6e28ea to
990e718
Compare
|
@RSS1102 Can you add a snapshot test to cover this bug fix change? |
7abfc83 to
4e44ec1
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af66713e7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
fengmk2
added
test: e2e
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb1cd60261
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@fengmk2 pls review again when you have time |
fengmk2
added
the
preview-build
…lution' into rss1102/fix-relative-path-bin-resolution
| 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()) |
There was a problem hiding this comment.
One concern is that this changes a single which_in call into multiple calls, one for each PATH entry.
Is there any way to keep a single which_in call, or would that require fixing which itself to resolve relative PATH entries against the provided cwd?
Like:
let path = normalized_entries
.map(|entry| {
if is_plain_relative_path(&entry) {
cwd.join(entry)
} else {
entry
}
});
let path_env = std::env::join_paths(path)?;
which::which_in(bin_name, Some(path_env), cwd)There was a problem hiding this comment.
Yes, I agree that a single which_in call would be preferable.
However, normalizing the PATH locally still requires special handling for tilde entries and a fallback when join_paths fails, such as when a Unix cwd contains :. That would add more than ten lines while still retaining the current per-entry lookup as a fallback.
I’ve opened an upstream PR to address this in which itself:
I suggest keeping the current workaround with a TODO, then simplifying it to one which_in call after the upstream release. If you prefer to wait for that release, please let me know.
There was a problem hiding this comment.
LGTM. After the PR is merged, you can create a follow-up issue to track it.
2 participants
Summary
PATHentries against the command cwd before binary lookup~expansion semanticsProblem
which::which_insearches relative PATH entries against the process cwd rather than the cwd supplied for the command. The npm-distributed CLI can therefore fail to resolvenodewhen Vite+ git hooks prepend./node_modules/.bin, even though the binary exists.Validation
cargo test -p vp_command(17 passed)cargo test -p vp_global_cli relative_path_entry(1 passed)cargo clippy -p vp_command --all-targets --all-features -- -D warningscargo fmt --all -- --checkgit diff --checkvite-plus@0.2.7and a relative PATH entry, then verified the same command reachesvp checkwith the patched NAPI bindingFixes #2326