node
node copied to clipboard
nodejs
build,tools: make addons tests work with GN
This PR refactors the install and build_addons scripts so they can be reused by node-ci and electron projects to build the addons tests.
In detail, this PR does following things:
- Minor modifications of the GN configurations to fix loading native modules.
- Add some flags to
tools/install.pyscript to allow customize locations ofconfig.gypi,v8, etc. With some refactoring to remove global variables. - Rewrite the
tools/build_addons.pyscript:- Rewrite it from js to python, because implementing cmd args handling with vanilla Node.js would take much longer time than a simple rewrite.
- Add a few more flags to make it easier to use for embedders.
- Modify the
Makefileto generate headers in out dir first, and then callsbuild_addons.py --headers-dir=out_dir/headers(which callsnode-gyp --nodedir=out_dir/headers) to build addons. Previously the build system was just callingnode-gyp --nodedir=., which relied on quirky behavior of node-gyp and did not really test whether the generated headers work.
Users that rely on make or vcbuild.bat to work on Node are not affected.
I have separated some changes out of this PR so review should be easier.
/cc @nodejs/gyp @joyeecheung @richardlau for reviewing changes on tools.
I have tested following commands with and without this PR, and compared the file list of generated tarballs:
-
make tar tar-headers binaryon linux/x64: The only difference is thetools/build-addons.mjsbecametools/build_addons.pyinnode-v22.0.0.tar.gz. -
make tar-headers binaryon mac/x64: The tarballs contain the same files. -
make tar-headers binary pkgon mac/arm64: The tarballs contain the same files. The pkg files contain the same files. -
vcbuild.bat build-releaseon win/x64: The msi files contain the same files.
I have a high confidence that this change won't break Node's release process, @nodejs/build please let me know if there is anything that you want to verify.
From the CI this is failing on Windows:
21:27:57 > python "C:\workspace\node-test-binary-windows-native-suites\node\tools\build_addons.py" "C:\workspace\node-test-binary-windows-native-suites\node\test\addons"
21:28:28 Generating headers
21:28:28 Traceback (most recent call last):
21:28:28 File "C:\workspace\node-test-binary-windows-native-suites\node\tools\build_addons.py", line 129, in <module>
21:28:28 main()
21:28:28 File "C:\workspace\node-test-binary-windows-native-suites\node\tools\build_addons.py", line 124, in main
21:28:28 rebuild_addons(args)
21:28:28 File "C:\workspace\node-test-binary-windows-native-suites\node\tools\build_addons.py", line 43, in rebuild_addons
21:28:28 shutil.copy2(os.path.join(args.out_dir, 'node.lib'),
21:28:28 File "C:\Python310\lib\shutil.py", line 434, in copy2
21:28:28 copyfile(src, dst, follow_symlinks=follow_symlinks)
21:28:28 File "C:\Python310\lib\shutil.py", line 254, in copyfile
21:28:28 with open(src, 'rb') as fsrc:
21:28:28 FileNotFoundError: [Errno 2] No such file or directory: 'out/Release\\node.lib'
I have pushed a fix for the CI failure. The scripts in CI assume the out dir is Release instead of out/Release, which was an very old behavior that happens to work.
Commit Queue failed
- Loading data for nodejs/node/pull/50737 ✔ Done loading data for nodejs/node/pull/50737 ----------------------------------- PR info ------------------------------------ Title build,tools: make addons tests work with GN (#50737) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch zcbenz:build-addons-gn -> nodejs:main Labels windows, build, openssl, libuv, tools, python, needs-ci, dependencies, commit-queue-squash Commits 3 - build,tools: make addons tests work with GN - build,tools: generate node headers on request - build,tools: there are 2 out dirs on windows Committers 1 - Cheng Zhaohttps://github.com/nodejs/node/actions/runs/8015803850PR-URL: https://github.com/nodejs/node/pull/50737 Reviewed-By: Joyee Cheung Reviewed-By: Benjamin Gruenbaum Reviewed-By: Yagiz Nizipli ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/50737 Reviewed-By: Joyee Cheung Reviewed-By: Benjamin Gruenbaum Reviewed-By: Yagiz Nizipli -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - build,tools: make addons tests work with GN ⚠ - build,tools: generate node headers on request ⚠ - build,tools: there are 2 out dirs on windows ℹ This PR was created on Wed, 15 Nov 2023 08:29:46 GMT ✔ Approvals: 3 ✔ - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/50737#pullrequestreview-1810123695 ✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/50737#pullrequestreview-1756204771 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/50737#pullrequestreview-1805222548 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-02-23T01:17:58Z: https://ci.nodejs.org/job/node-test-pull-request/57320/ - Querying data for job/node-test-pull-request/57320/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
Sorry to report it, but this PR caused regression, build in --shared mode now fails:
11:02:41: stdout installing /some_dir/build/prototype/i386/some_bindir/node
11:02:41: stderr Traceback (most recent call last):
11:02:41: stderr File "/some_dir/build/amd64/tools/install.py", line 419, in <module>
11:02:41: stderr run(parse_options(sys.argv[1:]))
11:02:41: stderr File "/some_dir/build/amd64/tools/install.py", line 372, in run
11:02:41: stderr files(options, install)
11:02:41: stderr File "/some_dir/build/amd64/tools/install.py", line 179, in files
11:02:41: stderr action(options, [os.path.join(output_prefix, output_lib)],
11:02:41: stderr ^^^^^^^^^^^^^
11:02:42: stderr UnboundLocalError: cannot access local variable 'output_prefix' where it is not associated with a value
The problem in source code is obvious, the variable 'output_prefix' is only defined in some branches. Later in code the variable is used in a branch where it was not defined before.
