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: support for latest NodeJS v20 #11

Merged
merged 13 commits into from
Dec 4, 2023
Merged

feat: support for latest NodeJS v20 #11

merged 13 commits into from
Dec 4, 2023

Conversation

glektarssza
Copy link

Seems to be working!

image

@glektarssza glektarssza mentioned this pull request Dec 2, 2023
@robertsLando
Copy link
Member

@glektarssza thanks so much for your PR, I can give it a review tomorrow 🙏🏻 did you tried to run all the tests locally? Also did you tried to spawn a binary build on your fork? If not give it a try to see if the build ends correctly 🙌🏻

@glektarssza
Copy link
Author

@glektarssza thanks so much for your PR, I can give it a review tomorrow 🙏🏻 did you tried to run all the tests locally? Also did you tried to spawn a binary build on your fork? If not give it a try to see if the build ends correctly 🙌🏻

@robertsLando Morning! No rush, I understand it's the weekend and it's an open-source project. I ran the verify script and it did pass. If there are more tests I should run please point me to them and I'd be happy to run them. I'll try to build a binary off my glektarssza/ftb-tool project later today and report back how it works.

@glektarssza
Copy link
Author

Building using my prebuilt binary.

image

Build works in the wild, at least on my WSL Debian instance.

image

@robertsLando robertsLando changed the title Add support for latest NodeJS v20. feat: support for latest NodeJS v20 Dec 3, 2023
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.

Can't say much about the patch as I'm not a NodeJS core/C++ expert but for the rest it looks good to me

Thanks so much for your contribution @glektarssza, this is a long awaited feature 🙏🏼

@robertsLando robertsLando merged commit 31bbc32 into yao-pkg:main Dec 4, 2023
6 checks passed
@robertsLando
Copy link
Member

@robertsLando
Copy link
Member

robertsLando commented Dec 4, 2023

Failed on alpine and linuxstatic ARM64:

https://github.com/yao-pkg/pkg-fetch/actions/runs/7083731774/job/19276653688
https://github.com/yao-pkg/pkg-fetch/actions/runs/7083731774/job/19276658686

Error seems related to an unrecognized option on g++:

g++: error: unrecognized command-line option '-msign-return-address=all'

https://github.com/yao-pkg/pkg-fetch/actions/runs/7083731774/job/19276653688#step:4:30520

And that seems a known error: nodejs/node#45756

Maybe that should be added to patch? Seems to only happen when cross-building arm64

UPDATE

Also windows x64 has failed: https://github.com/yao-pkg/pkg-fetch/actions/runs/7083731774/job/19276659687#step:6:31323

What I suggest is to try running those actions on your fork when doing tests

@ptaferner
Copy link

UPDATE

Also windows x64 has failed: https://github.com/yao-pkg/pkg-fetch/actions/runs/7083731774/job/19276659687#step:6:31323

What I suggest is to try running those actions on your fork when doing tests

This is a trivial syntax error: Just need to remove the double "return" on line 93 of node_main.cc.
I just did that and got a successful build on win64.

Btw, I will use the binary for an internal application at my company, so I can give some feedback if I see any issues with the patched node binary.

robertsLando added a commit that referenced this pull request Dec 4, 2023
@robertsLando
Copy link
Member

Yeah just submitted a fix for the patch, need to sort out how to fix the arm64 problem now...

@robertsLando
Copy link
Member

robertsLando commented Dec 4, 2023

Seems the ARM64 error was fixed in vercel#280 by @baparham. Seems it has been ported to v20 patch too so I don't understand why it's failing.

@baparham any clue?

@ptaferner
Copy link

ptaferner commented Dec 4, 2023

My application currently fails with:

node:internal/bootstrap/realm:422
  if (!mod) throw new TypeError(`Missing internal module '${id}'`);
            ^

TypeError: Missing internal module 'internal/modules/cjs/helpers'
    at requireBuiltin (node:internal/bootstrap/realm:422:19)
    at pkg/prelude/bootstrap.js:1883:10
    at pkg/prelude/bootstrap.js:1982:3
    at pkg/prelude/bootstrap.js:2255:3
    at readPrelude (node:internal/bootstrap/pkg:36:12)
    at node:internal/bootstrap/pkg:41:18
    at node:internal/bootstrap/pkg:48:4
    at node:internal/bootstrap/pkg:49:2

Node.js v20.10.0

This is probably unrelated to pkg-fetch and rather an issue with pkg. I'm analyzing that now.

@robertsLando
Copy link
Member

@ptaferner Are you using pkg master branch? Dunno if it could be someway related to yao-pkg/pkg#8

@robertsLando
Copy link
Member

robertsLando commented Dec 4, 2023

I may have a fix for arm64 too: https://github.com/yao-pkg/pkg-fetch/actions/runs/7087579394/job/19288161530, for some reason the patch could not be needed anymore with node20

@ptaferner
Copy link

@ptaferner Are you using pkg master branch? Dunno if it could be someway related to yao-pkg/pkg#8

Ah, many thanks! This has indeed resolved the problem and my app runs now! :)

@robertsLando
Copy link
Member

@ptaferner Did you reverted that change and it worked so? Strange...

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.

3 participants