node
node copied to clipboard
nodejs
test,doc: enable running embedtest for Windows
Currently the embedtest does not run on Windows.
One of the main reasons is that the Windows command line does not accept UTF-8 characters required by the test.
In this PR we enable embedtest to run on Windows:
- Use
node::FixupMainfromtools/executable_wrapper.hto fixargcandargvvalues. TheNODE_MAINmacro is defined aswmainfor Windows targets that receives parameters as UTF-16. Then, thenode::FixupMainconverts them to UTF-8. - Make sure that we read and write snapshot files as binary files. Windows uses text mode by default.
- Add calling
embedtesttovcbuild.batas a part ofcctestcall. It is currently implemented the same way in theMakefile. -
Makefileis changed to addtoolsinclude directory for theembedtestproject. It is need to include theexecutable_wrapper.h. - Fix small typo in
embedding.mdfile.
We already have
tools/executable_wrapper.hand AFAICT it serves the same purpose. Not sure why this wasn't done for the embedtest although that wrapper was introduced later when we do need to create executable that also works on Windows (js2c). Can we just use that for the embedtest?
Great! It could work.
For the PR #43542 I will need to make tools/executable_wrapper.h compatible with C.
It was the reason why the new APIs were done to match C calling conventions.
Since tools/executable_wrapper.h is header-only it must be trivial to do as part of PR #43542.
IIUC to share code with https://github.com/nodejs/node/pull/43542 ultimately we need to expose a helper like node::FixupMain to node.h. That sounds like a good idea, because the UTF8 convention is not currently surfaced to node.h, and we can also use it to remove the repeated code in src/node_main.cc. Not sure about how useful/appropriate it is for the C API, I'll defer that to the n-api team.
It seems that we can avoid converting the node::FixupMain to C.
#43542 adds two new executables to the embedtest directory.
The simplest approach would be to add testmain.cc that is reused across all three executables.
It will be a C++ file and it will use the node::FixupMain. It then will call TestMain function for each test executable.
Anyway, the change will be implemented in the #43542.
This PR will be greatly simplified by using the node::FixupMain.
As for the src/node_main.cc, I see that it has a copy of tools/executable_wrapper.h code.
I guess we include tools/executable_wrapper.h to src/node_main.ccto reuse it.
I can create a separate PR for it.
It seems that macOS benchmark is failing, but the failure is not related to changes in this PR. I guess something else makes the test to break.
I think that is one of the known flaky tests, restarted the github action on macOS
I guess we include tools/executable_wrapper.h to src/node_main.cc to reuse it.
I think the reason why we don't do that is, we generally try to keep src/node_main.cc rely only on node.h and not any internal headers (I am not sure if that's just my impression at this point, or is that something really enforced). But tackling that in a different PR SGTM.
It seem that CI shows that the new embedding testing is failing on Windows and it is related to this PR. I am going to investigate/fix it. I wonder if the child.stderr is changed last week.
10:18:57 > echo running 'Release\node.exe test\embedding\test-embedding.js'
10:18:57 running 'Release\node.exe test\embedding\test-embedding.js'
10:18:57
10:18:57 > "Release\node.exe" test\embedding\test-embedding.js
10:18:57 [process 0]: --- stderr ---
10:18:57 c:\workspace\node-test-binary-windows-native-suites\node\test\common\child_process.js:82
10:18:57 console.error(stderrStr === undefined ? child.stderr.toString() : stderrStr);
10:18:57 ^
10:18:57
10:18:57 TypeError: Cannot read properties of null (reading 'toString')
10:18:57 at logAndThrow (c:\workspace\node-test-binary-windows-native-suites\node\test\common\child_process.js:82:58)
10:18:57 at expectSyncExit (c:\workspace\node-test-binary-windows-native-suites\node\test\common\child_process.js:91:5)
10:18:57 at spawnSyncAndAssert (c:\workspace\node-test-binary-windows-native-suites\node\test\common\child_process.js:131:10)
10:18:57 at Object.<anonymous> (c:\workspace\node-test-binary-windows-native-suites\node\test\embedding\test-embedding.js:27:1)
10:18:57 at Module._compile (node:internal/modules/cjs/loader:1480:14)
10:18:57 at Module._extensions..js (node:internal/modules/cjs/loader:1564:10)
10:18:57 at Module.load (node:internal/modules/cjs/loader:1287:32)
10:18:57 at Module._load (node:internal/modules/cjs/loader:1103:12)
10:18:57 at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:168:12)
10:18:57 at node:internal/main/run_main_module:30:49
Failed to start CI
⚠ Something was pushed to the Pull Request branch since the last approving review. ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/8958495093
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
I had to rebase the PR since the test break on Windows seemed to be caused by PR being submitted against Node.js version 22 while now we have the version 23.
I see that the embedtest fails to run on Windows because the embedtest.exe is missing.
We must add it to the BINARY_FILES variable in
https://github.com/nodejs/build/blob/main/jenkins/scripts/windows/node-compile-windows.cmd
~~I guess I must split up this PR into two:~~
~~- add building embedtest.exe, then change the nodejs/build~~
~~- add testing embedtest.exe after changes to nodejs/build~~
(Edit: see the comment below - no need to split up the PR)
I have found that the embedtest.exe is already built. So, we just need to update the https://github.com/nodejs/build scripts to package it before we can merge this PR.
I have switched this PR state to draft until the CI scripts are ready.
The build script to package embedtest.exe for testing is merged (https://github.com/nodejs/build/pull/3730).
It should enable CI for this PR to pass.
The PR is rebased on the latest code. No other changes are added.
@mhdawson , could you please review it again? The CI cannot be started without newer bits review. Thank you!
@StefanStojanovic , it seems that the VS2022 build for ARM64 fails because of insufficient hard drive space. Is there a way to increase it?
06:29:57 C:\workspace\node-compile-windows\node\out\Release\obj\global_intermediate\torque-generated\src\builtins\string-substring-tq-csa.cc : fatal error C1085: Cannot write compiler generated file: 'C:\workspace\node-compile-windows\node\out\Release\obj\v8_initializers_host\obj\global_intermediate\torque-generated\src\builtins\string-substring-tq-csa.obj': No space left on device [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers_host.vcxproj]
@StefanStojanovic , it seems that the VS2022 build for ARM64 fails because of insufficient hard drive space. Is there a way to increase it?
This happens sometimes. I'm investigating ARM64 cross-compilation issues in CI and will hopefully have a more permanent solution soon.
Commit Queue failed
- Loading data for nodejs/node/pull/52646 ✔ Done loading data for nodejs/node/pull/52646 ----------------------------------- PR info ------------------------------------ Title test,doc: enable running embedtest for Windows (#52646) Author Vladimir Morozovhttps://github.com/nodejs/node/actions/runs/9212395583(@vmoroz) Branch vmoroz:pr/enable_embedtest_for_windows -> nodejs:main Labels windows, build, needs-ci Commits 2 - test,doc: enable running embedtest for Windows - use existing executable_wrapper.h Committers 1 - Vladimir Morozov PR-URL: https://github.com/nodejs/node/pull/52646 Reviewed-By: Joyee Cheung Reviewed-By: Michael Dawson Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52646 Reviewed-By: Joyee Cheung Reviewed-By: Michael Dawson Reviewed-By: James M Snell -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 22 Apr 2024 17:29:50 GMT ✔ Approvals: 3 ✔ - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/52646#pullrequestreview-2069697036 ✔ - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/52646#pullrequestreview-2071969985 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/52646#pullrequestreview-2039461466 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-05-23T07:38:01Z: https://ci.nodejs.org/job/node-test-pull-request/59361/ - Querying data for job/node-test-pull-request/59361/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 52646 From https://github.com/nodejs/node * branch refs/pull/52646/merge -> FETCH_HEAD ✔ Fetched commits as c137d6eb3101..91b5225d7946 -------------------------------------------------------------------------------- [main c9fde46b15] test,doc: enable running embedtest for Windows Author: Vladimir Morozov Date: Mon Apr 22 10:12:59 2024 -0700 7 files changed, 82 insertions(+), 4 deletions(-) create mode 100644 test/embedding/utf8_args.c create mode 100644 test/embedding/utf8_args.h [main 0b310539dd] use existing executable_wrapper.h Author: Vladimir Morozov Date: Tue Apr 23 14:30:34 2024 -0700 5 files changed, 5 insertions(+), 76 deletions(-) delete mode 100644 test/embedding/utf8_args.c delete mode 100644 test/embedding/utf8_args.h ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- test,doc: enable running embedtest for Windows
PR-URL: https://github.com/nodejs/node/pull/52646 Reviewed-By: Joyee Cheung [email protected] Reviewed-By: Michael Dawson [email protected] Reviewed-By: James M Snell [email protected]
[detached HEAD bfaf50e60f] test,doc: enable running embedtest for Windows Author: Vladimir Morozov [email protected] Date: Mon Apr 22 10:12:59 2024 -0700 7 files changed, 82 insertions(+), 4 deletions(-) create mode 100644 test/embedding/utf8_args.c create mode 100644 test/embedding/utf8_args.h Rebasing (3/4) Rebasing (4/4)
Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- use existing executable_wrapper.h
PR-URL: https://github.com/nodejs/node/pull/52646 Reviewed-By: Joyee Cheung [email protected] Reviewed-By: Michael Dawson [email protected] Reviewed-By: James M Snell [email protected]
[detached HEAD 455cd1577d] use existing executable_wrapper.h Author: Vladimir Morozov [email protected] Date: Tue Apr 23 14:30:34 2024 -0700 5 files changed, 5 insertions(+), 76 deletions(-) delete mode 100644 test/embedding/utf8_args.c delete mode 100644 test/embedding/utf8_args.h
Successfully rebased and updated refs/heads/main.
ℹ Add
commit-queue-squashlabel to land the PR as one commit, orcommit-queue-rebaseto land as separate commits.