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

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

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

faulpeltz
Copy link

@faulpeltz faulpeltz commented Sep 9, 2024

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
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
Member

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

.github/workflows/build-alpine.yml Outdated Show resolved Hide resolved
.github/workflows/build-windows.yml Outdated Show resolved Hide resolved
@faulpeltz
Copy link
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
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
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.

@faulpeltz
Copy link
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
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

Copy link
Member

@robertsLando robertsLando left a comment

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)

patches/node.v22.8.0.cpp.patch Show resolved Hide resolved
patches/node.v22.8.0.cpp.patch Show resolved Hide resolved
patches/node.v22.8.0.cpp.patch Show resolved Hide resolved
patches/node.v22.8.0.cpp.patch Show resolved Hide resolved
patches/node.v22.8.0.cpp.patch Show resolved Hide resolved
patches/node.v22.8.0.cpp.patch Show resolved Hide resolved
@robertsLando
Copy link
Member

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

@robertsLando
Copy link
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
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
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
4 checks passed
@robertsLando
Copy link
Member

@faulpeltz
Copy link
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
Copy link
Member

robertsLando commented Sep 10, 2024

@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