feat: node 24 support and attempt some Node 22 fixes #107
Conversation
- run arm64 build on arm64 GH action - deprecate muslcc because docker image is no longer maintained
- keep muslcc around for older node versions, Node 22 should also work with the new setup
- crossbuild from macos14 to x64 because macos14 x64 runners are not available on GH without paid runners
- install newer python version (3.12+) - use full-icu with Node 24 because of icu generator crash
- initial version is Node 24.7 - fixes some cross-build issues: x64 linux/win/alpine now work again
…tible again - in CSA_HOLE_SECURITY_CHECK macro, don't use a filename because it ends up on the readonly heap as a string - in readonly-serializer overwrite pointer with zero because if V8_COMPRESS_POINTERS is not enabled (node default), the tagged value is only 4 bytes leaving another 4 bytes of garbage in the serialized data
There was a problem hiding this comment.
Pull Request Overview
This PR adds Node 24 support and implements several fixes to improve cross-platform build compatibility, particularly addressing Node 22 snapshot determinism issues.
Key changes include:
- Added Node 24.7.0 patch and updated build configurations for Node 24 compatibility
- Fixed V8 snapshot determinism issues that caused cross-build compatibility problems in Node 22+
- Updated build infrastructure to use newer toolchains (GCC 12) and platforms (macOS 14)
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| patches/patches.json | Adds Node 24.7.0 patch entry |
| patches/node.v24.7.0.cpp.patch | Complete Node 24.7.0 patch with V8 fixes and pkg-specific modifications |
| patches/node.v22.19.0.cpp.patch | Backports snapshot determinism fixes to Node 22 |
| lib/build.ts | Updates build configuration for Node 24, macOS cross-compilation, and ICU handling |
| Dockerfile.* | Updates build environments to use newer toolchains and Alpine versions |
| .github/workflows/*.yml | Updates CI workflows to build Node 20/22/24 instead of 16/18/20/22 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // disabled for now - can't get cmdline flag to work with all builds | ||
| // log.info(header.name); |
There was a problem hiding this comment.
This commented-out logging should either be properly implemented with the cmdline flag or removed entirely. Leaving disabled code with TODO comments reduces code clarity.
| // disabled for now - can't get cmdline flag to work with all builds | |
| // log.info(header.name); | |
| log.info(header.name); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
i would leave it for later, the tar file logging is not needed IMHO
|
You did an awesome job here @faulpeltz ! Well done! 💪 took a quick look but will end up my review tomorrow as I have to go now, good to know you also find a way to fix the annoying cross compiling issue 🎉 |
|
@robertsLando the cross-compiling issue is unfortunately only partially fixed, and might be unfixable in some cases Latest asset build from my fork: |
| @@ -204,8 +213,16 @@ async function compileOnWindows( | |||
| args.push('ltcg'); | |||
| } | |||
|
|
|||
| // Node24 builds on Windows crash with small-icu at icudat codegen | |||
| // workaround for now is to enable full-icu | |||
| // TODO check with newer node/tooling/gh-image versions | |||
There was a problem hiding this comment.
would you like to open an issue for this so we can keep track of it or do you want to try that in this PR?
There was a problem hiding this comment.
Let'sl open an issue, I currently have no idea why this happens, the ICU codegen crashes on the GH windows runner with an Access Violation, on my local Windows VM it doesn't - I checked some of the tooling versions but couldn't find any differences, maybe try switching to 2025 images 🤷
| + // PrintF("[pkg] ReadOnlySnapshotChecksum mismatch: code=%08x built=%08x\n", | ||
| + // ro_snapshot_checksum, expected_ro_snapshot_checksum); |
There was a problem hiding this comment.
why not keeping this uncommented?
There was a problem hiding this comment.
I had this uncommented but it logs to console everytime you run something cross-platform which doesn't work with code cache but then falls back to the source included (which then works)
Need to find a better place to log this somwhere in Node code where we know if there is an empty Script or not
|
@faulpeltz Everything looks good to me, well done! Tell me how you want to proceed here, if you want to take some more time to do some other tries on the missing things or if we can merge and release 🙏🏼 Apart from this I would like to ask you a question. Do you think that the actual patches we are creating now could ever be ported to nodejs code? I mean could we make a proposal to NodeJS to accept our changes? Of course the PR should be different from the patch but something that could allow us to not having to deal with patches anymore... Unfortunately my knowledge is very limited on NodeJS source code so sorry if the question sounds dumb to you. I know some people working on NodeJS core team and I may ask them if they could ever accept this kind of PR but I would like your opinion first 🙏🏼 |
robertsLando
changed the title
Did you have the chance to test some of the binaries?
I don't know any of the folks working on Node, but IMHO they have decided to go the SEA route which is the technically clean approach. The way the patch currently works in pkg is 2 major parts:
The main problem with the pkg approach is that the code cache was never intended to be or designed as a portable "intermediate" code format, but as a cache for running the code inside the exact same env (arch/platform/v8 version), with the source script always available to fall back to |
|
@robertsLando you might want to remove the node 18 CI test from the PR checks |
|
Also regarding the Node patches: The two changes I made for better cross-platform determinism could be fixed upstream
|
I'm wondering if this could ever live behind a flag for example, about the VFS I heard they were considering implementing an abstract layer to allow patching it without doing a monkey patch but dunno if there have been some updates on this, I don't have the links to the issues unfortunately... I think ATM the most advanced framework for single executables is bun: https://bun.com/docs/bundler/executables |
This PR adds Node24 support
Node/V8 changes:
read-only-serializer:258the "live" pointer gets overwritten by the encoded slot (EncodedTagged).If V8_COMPRESS_POINTERS is not enabled, EncodedTagged is 4 bytes, but the live pointer is 8 bytes.
memcpy() only copies EncodeTagged leaving 4bytes of "garbage" (=previous upper pointer half) which is semi-random per run - fixed this by clearing the whole value first
Those fixes partially fix the sourceless cross-build issues introduced in Node 22 - For my test builds linux/linuxstatic/alpine/win on x64 are now compatible again. The heap contents on macos builds and other arm64 is wildly different, not sure why. I don't think this kind of compatibility is a goal for the V8 snapshot implementation in general. (This affects Node SEA as well, but SEA always has the source to fall back onto, which incurs a small performance hit)
Build Changes:
full-icufor now because the ICU code gen step crashes withsmall-icuon the GH action build runners (not on my Windows test machine though)Notes: