Skip to content

feat: add Node 22 patch (#45) (by @faulpeltz)#45

Merged
robertsLando merged 1 commit into
yao-pkg:mainfrom
faulpeltz:main
Sep 10, 2024
Merged

feat: add Node 22 patch (#45) (by @faulpeltz)#45
robertsLando merged 1 commit into
yao-pkg:mainfrom
faulpeltz:main

Conversation

@faulpeltz

@faulpeltz faulpeltz commented Sep 9, 2024

Copy link
Copy Markdown

Adds the patch for Node 22(.8) - consider this experimental for now

Patch:

  • The new V8 version moved things around a bit, but the patch is mainly concered with "no source" handling on Script objects, seems to work
  • On the Node side, parsing/stating package.json content has been moved to Node internals, and I had to restore parts of the original code using the patched "fs" module otherwise it doesn't know about the snapshot fs
  • I had to rewrite some code in V8 which uses C++20 features but the musl cross-platform tooling seems not to support that (the muslcc docker image and project seem to be abandoned, maybe its just some missing CC flag)

Build env:

  • I was able to get all builds to work except macos-arm64, because it times out or crashes with out of memory on the GH actions workers
  • The linux images work on a hello-world app, both with and without bytecode
  • Bumped the alpine and linuxstatic images to Ubuntu 22.04 and Python inside the Oracle linux build container to 3.12
  • Bumped the windows build image to vs-2022

This still requires extensive testing on real-world apps, but the basics should be in place.

@robertsLando

Copy link
Copy Markdown
Member

Hi @faulpeltz and thanks a lot for this PR!

I was able to get all builds to work except macos-arm64, because it times out or crashes with out of memory on the GH actions workers

I was having the same issue in #40, I fixed it by reducing job counts to 2, try using 1 maybe?

@robertsLando

Copy link
Copy Markdown
Member

@faulpeltz New majors usually require some work on pkg side too, would you mind to open a PR there and reference this?

Comment thread .github/workflows/build-alpine.yml Outdated
Comment thread .github/workflows/build-windows.yml Outdated
@faulpeltz

Copy link
Copy Markdown
Author

Hi @faulpeltz and thanks a lot for this PR!

I was able to get all builds to work except macos-arm64, because it times out or crashes with out of memory on the GH actions workers

I was having the same issue in #40, I fixed it by reducing job counts to 2, try using 1 maybe?

I tried setting it to 1, but then it runs into the 6h build time limit

@faulpeltz

Copy link
Copy Markdown
Author

@faulpeltz New majors usually require some work on pkg side too, would you mind to open a PR there and reference this?

Didn't change anything for now, will open a PR as soon as there are any changes

@robertsLando

Copy link
Copy Markdown
Member

@faulpeltz About macos-arm64 you could try to use macos-14 runner as it's ARM64, you can then remove the compiler env vars (also rememver to change the arch check step at start to check for arm64 instead.

Comment thread .github/workflows/build-macos.yml
@faulpeltz

Copy link
Copy Markdown
Author

@faulpeltz About macos-arm64 you could try to use macos-14 runner as it's ARM64, you can then remove the compiler env vars (also rememver to change the arch check step at start to check for arm64 instead.

no luck with the macos-14 runner, setting jobs to 1 helps but it seems to run out of disk space every time

@robertsLando

Copy link
Copy Markdown
Member

Thanks for giving it a try, I'm sincerly out of ideas about arm64, I have lost a week tring to make it work for latest images, I think the problem is that each new major version requires more resources so the error happens. If you are aware of any build flag and/or env that could help mitigate the issue let me know. For what I saw x64 cross build still preforms better then the native arm64 in terms of RAM so I would restore it for now and work on that.

The alternative could be to use the -large runners but I don't think them are free to use

@robertsLando robertsLando left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a few comments to help me understand the changes you did (I'm not a C++ expert)

Comment thread patches/node.v22.8.0.cpp.patch
Comment thread patches/node.v22.8.0.cpp.patch
Comment thread patches/node.v22.8.0.cpp.patch
Comment thread patches/node.v22.8.0.cpp.patch
Comment thread patches/node.v22.8.0.cpp.patch
Comment thread patches/node.v22.8.0.cpp.patch
@robertsLando

Copy link
Copy Markdown
Member

@faulpeltz thanks so much for all the explainations 🙏🏼 Do you think this can be merged and released?

@robertsLando

Copy link
Copy Markdown
Member

@faulpeltz I would disable arm64 build on nodejs 22 for now as we need it to end successgully otherwise the build all job will not end successfully and I cannot do a bump

@faulpeltz

Copy link
Copy Markdown
Author

@faulpeltz thanks so much for all the explainations 🙏🏼 Do you think this can be merged and released?

I think we can merge it, but maybe make some sort of prerelease or release Node 22 as experimental because I think this should be tested by the community.

I'll just clean up history and remove the arm64 macos build for now

@robertsLando

Copy link
Copy Markdown
Member

I think we can merge it, but maybe make some sort of prerelease or release Node 22 as experimental because I think this should be tested by the community.

I would make a normal minor release of pkg and tell on changelog that node 22 support is experimental

@robertsLando robertsLando changed the title feat: add Node 22 patch feat: add Node 22 patch (#45) (by @faulpeltz) Sep 10, 2024
@robertsLando robertsLando merged commit 1703cd2 into yao-pkg:main Sep 10, 2024
@robertsLando

Copy link
Copy Markdown
Member

Let's see how this goes: https://github.com/yao-pkg/pkg-fetch/actions/runs/10789654031

@faulpeltz

Copy link
Copy Markdown
Author

Let's see how this goes: https://github.com/yao-pkg/pkg-fetch/actions/runs/10789654031

I just saw this issue in the main NodeJS repo:
nodejs/build#3878

We could try this for pkg fetch builds as well

@robertsLando

robertsLando commented Sep 10, 2024

Copy link
Copy Markdown
Member

@faulpeltz do you mean:

https://github.com/nodejs/node/pull/54658/files ?

May be worth a try, yeah! :)

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