Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: which should use cwd if given a relative filepath #9761

Merged
merged 5 commits into from
Mar 31, 2024

Conversation

paperclover
Copy link
Member

What does this PR do?

This corrects a logic mistake in my previous PR to fix Bun.which

Doing which node_modules/.bin/next should result in the relative path join of cwd and the path, which happens specifically because there are slashes in the path. A path without slashes is treated as a command name, and that is looked up in PATH.

You can verify this new logic is correct by looking at the which command's output.

@@ -1642,7 +1642,7 @@ pub const Subprocess = struct {

if (argv0 == null) {
var path_buf: [bun.MAX_PATH_BYTES]u8 = undefined;
const resolved = Which.which(&path_buf, PATH, arg0.slice()) orelse {
const resolved = Which.which(&path_buf, PATH, cwd, arg0.slice()) orelse {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this cwd be the one on line 1709?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was from reverting #9691. i'll look if this is correct code.

src/which.zig Outdated
@@ -133,7 +140,20 @@ fn whichWin(buf: *bun.WPathBuffer, path: []const u8, bin: []const u8) ?[:0]const
return searchBin(buf, bin_utf16.len, check_windows_extensions);
}

if (path.len == 0) return null;
// check if bin is in cwd
if (bun.strings.containsChar(bin, '/')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should be backslashes because this is the windows version. Or maybe we should accept both?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be both

buf,
&path_buf,
cwd,
bun.strings.withoutPrefixComptime(bin, "./"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was from reverting #9691. i'll look if this is correct code.

@@ -145,3 +165,14 @@ fn whichWin(buf: *bun.WPathBuffer, path: []const u8, bin: []const u8) ?[:0]const

return null;
}

test "which" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm excited to get zig tests working

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was from reverting #9691. i don't think we'll get the zigtests working

Copy link
Member

@dylan-conway dylan-conway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple comments

Copy link
Contributor

github-actions bot commented Mar 31, 2024

Copy link
Contributor

github-actions bot commented Mar 31, 2024

Copy link
Contributor

github-actions bot commented Mar 31, 2024

@Jarred-Sumner
Copy link
Collaborator

which test is failing on windows

Copy link
Contributor

@Jarred-Sumner Jarred-Sumner merged commit f027525 into main Mar 31, 2024
26 of 32 checks passed
@Jarred-Sumner Jarred-Sumner deleted the dave/cwd-fixes branch March 31, 2024 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants