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

implement node:util.getSystemErrorName() #12837

Merged
merged 9 commits into from
Jul 27, 2024
Merged

Conversation

nektro
Copy link
Member

@nektro nektro commented Jul 26, 2024

split off from #11492 to merge independently

includes new test coverage that should all pass

Copy link
Contributor

github-actions bot commented Jul 26, 2024

@nektro, your commit has failing tests :(

🐧🖥 1 failing tests Linux x64

  • test/js/node/worker_threads/worker_destruction.test.ts 1 failing

🪟💻 2 failing tests Windows x64 baseline

  • test/cli/install/registry/bun-install-registry.test.ts 1 failing
  • test/js/node/child_process/child_process.test.ts 1 failing

🪟💻 2 failing tests Windows x64

  • test/cli/install/registry/bun-install-registry.test.ts 1 failing
  • test/js/node/child_process/child_process.test.ts 1 failing

View logs

@Jarred-Sumner
Copy link
Collaborator

Serializing IRL discussion:

  • The error methods should be done in C++ to ensure we don't run into situations where the JSString is collected before the Error instance is finished being created. Also because the current implementation would involve several Latin1 -> UTF-8 -> Latin1 string conversions which are unnecessary. Also because the usage of ZigString.slice() is incorrect, it would produce invalid data when the String is a UTF-16 string
  • The system errno map should follow the convention we have in darwin_c.zig, windows_c.zig, and linux_c.zig as those already have mostly the same data.

@nektro nektro requested a review from Jarred-Sumner July 27, 2024 00:54
Copy link
Contributor

@nektro, clang-tidy had something to share with you about your code:

CMake Warning at CMakeLists.txt:254 (message):
  Expected LLVM 18 as the C++ compiler, build may fail or break at runtime.


  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0

  0  366M    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
 11  366M   11 41.2M    0     0  28.5M      0  0:00:12  0:00:01  0:00:11 48.8M
 23  366M   23 85.2M    0     0  34.9M      0  0:00:10  0:00:02  0:00:08 46.2M
 29  366M   29  110M    0     0  36.3M      0  0:00:10  0:00:03  0:00:07 45.3M
curl: (92) HTTP/2 stream 1 was not closed cleanly: PROTOCOL_ERROR (err 1)
CMake Error at CMakeLists.txt:560 (message):
  Prebuilt WebKit package bun-webkit-macos-arm64-debug failed to install

Commit: ef74003

@Jarred-Sumner Jarred-Sumner merged commit a0ebb05 into main Jul 27, 2024
49 of 55 checks passed
@Jarred-Sumner Jarred-Sumner deleted the nektro-patch-14066 branch July 27, 2024 07:20
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.

2 participants