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: large bunx changes, mostly for better windows support #9143

Merged
merged 36 commits into from
Mar 7, 2024

Conversation

paperclover
Copy link
Member

@paperclover paperclover commented Feb 28, 2024

stuff like this works now:
image

Copy link
Contributor

github-actions bot commented Feb 28, 2024

@paperdave 1 files with test failures on bun-darwin-aarch64:

View test output

#56c032e52272c68fdc41d112843a5ef29daf7ef5

Copy link
Contributor

github-actions bot commented Feb 28, 2024

Copy link
Contributor

github-actions bot commented Feb 28, 2024

@paperdave 1 files with test failures on linux-x64-baseline:

View test output

#56c032e52272c68fdc41d112843a5ef29daf7ef5

Copy link
Contributor

github-actions bot commented Feb 28, 2024

Copy link
Contributor

github-actions bot commented Feb 28, 2024

❌🪟 @paperdave, there are 13 test regressions on Windows x86_64

  • test\cli\install\bun-upgrade.test.ts
  • test\cli\run\transpiler-cache.test.ts
  • test\js\bun\dns\resolve-dns.test.ts
  • test\js\bun\http\fetch-file-upload.test.ts
  • test\js\bun\http\bun-server.test.ts
  • test\js\bun\shell\shelloutput.test.ts
  • test\js\bun\shell\throw.test.ts
  • test\js\node\dns\node-dns.test.js
  • test\js\node\process\process-args.test.js
  • test\js\web\fetch\body-stream-excess.test.ts
  • test\js\web\fetch\body-stream.test.ts
  • test\js\web\websocket\websocket.test.js
  • test\js\web\workers\worker.test.ts

Full Test Output

@paperclover
Copy link
Member Author

another fix done
image

@paperclover
Copy link
Member Author

image i am removing this because unreachable here does not do any optimization and unlike in zig we cannot do noreturn.

@paperclover paperclover changed the title fix(windows): bunx works without node installed fix: large bunx changes, mostly for better windows support Mar 5, 2024
@paperclover paperclover marked this pull request as ready for review March 6, 2024 22:32
@paperclover paperclover marked this pull request as draft March 7, 2024 00:01
@paperclover
Copy link
Member Author

Many test failures to fix first

@paperclover paperclover marked this pull request as ready for review March 7, 2024 02:59
@@ -202,6 +207,21 @@ pub const Shebang = struct {
return try Shebang.init(line, false);
}

pub fn coerceNodeToBun(self: Shebang, buf: *[32766]u8) Shebang {
Copy link
Collaborator

Choose a reason for hiding this comment

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

✂️

@Jarred-Sumner
Copy link
Collaborator

Assuming the test failures are gone, looks great.

@Jarred-Sumner Jarred-Sumner merged commit 3b13f7f into main Mar 7, 2024
25 of 29 checks passed
@Jarred-Sumner Jarred-Sumner deleted the dave/win/x branch March 7, 2024 22:20
_ = nt.NtClose(process.hProcess);
_ = nt.NtClose(process.hThread);
_ = nt.NtClose(process.hProcess);
_ = nt.NtClose(process.hThread);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@paperdave was it intentional we close the thread handle only after the process has exited instead of immediately? since this handle is not used, couldn't we close it immediately?

inline for (.{ 0, 1 }) |attempt_number| iteration: {
if (dbg)
debug("lpCommandLine: {}\n", .{fmt16(std.mem.span(spawn_command_line))});
const did_process_spawn = k32.CreateProcessW(
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should get/set the codepage before & after?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants