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

Support for node version 20.17.0 #976

Merged
merged 4 commits into from
Oct 9, 2024
Merged

Support for node version 20.17.0 #976

merged 4 commits into from
Oct 9, 2024

Conversation

toshiyuki-rs
Copy link
Contributor

I modified some implementations to be able to compile for node version 20.17.0.

This modification may resolve #973 .

fixed linkage error for multiple function definitions.

refs nodejs#973
@kkoopa
Copy link
Collaborator

kkoopa commented Sep 26, 2024

Thank you, I will take a closer look at this in the coming days. However, assuming that this works backwards for relatively old versions of Node.js (I do not know where to reasonably draw the cut-off line), I still have some concerns about the issues mentioned at the end of my latest comment on the issue related to (javascript) exception handling. That is, even if it compiles, there might be runtime inconsistencies lurking beneath the surface.

@agracio
Copy link

agracio commented Sep 27, 2024

Just wanted to add that the issue is also present in upcoming Node.js 23 as this commit deps: update V8 to 12.8.374.13 was added to main node branch about a week before #973 was opened.

I tested this PR with main node branch (AKA 23) and it resolves issues with node native modules that have dependency on Node.js without Electron. Without it the failure is the exact same as with Electron 32, my findings are documented in this comment: #973 (comment).

Multiple people including myself reported that this PR does resolve #973.

@kkoopa
Copy link
Collaborator

kkoopa commented Oct 3, 2024

test/cpp/methodswithdata.cpp:174 and test/cpp/methodswithdata.cpp:180 SetAccessor seem to fail on almost every entry in the test matrix

@agracio
Copy link

agracio commented Oct 7, 2024

@toshiyuki-rs Would you be able to take a look at failing tests by any chance?

@toshiyuki-rs
Copy link
Contributor Author

@agracio

Would you be able to take a look at failing tests by any chance?

Yes, I will check test/cpp/methodswithdata.cpp and test/cpp/methodswithdata.cpp

I removed wrong preprocessor directive.

refs nodejs#973
@toshiyuki-rs
Copy link
Contributor Author

I removed wrong preprocessor directive and ran test.

> nan@2.20.0 test
> tap --gc --stderr test/js/*-test.js

ok test/js/accessors2-test.js ........................... 5/5
ok test/js/accessors-test.js ............................ 8/8
ok test/js/asyncprogressqueueworkerstream-test.js ....... 2/2
ok test/js/asyncprogressqueueworker-test.js ............. 7/7
ok test/js/asyncprogressworkersignal-test.js ............ 7/7
ok test/js/asyncprogressworkerstream-test.js ............ 2/2
ok test/js/asyncprogressworker-test.js .................. 7/7
ok test/js/asyncresource-test.js ........................ 8/8
ok test/js/asyncworkererror-test.js ..................... 4/4
ok test/js/asyncworker-test.js ........................ 10/10
ok test/js/buffer-test.js ............................... 9/9
ok test/js/bufferworkerpersistent-test.js ............... 8/8
ok test/js/callbackcontext-test.js ...................... 8/8
ok test/js/converters-test.js ......................... 33/33
ok test/js/error-test.js .............................. 61/61
ok test/js/gc-test.js ................................... 4/4
ok test/js/indexedinterceptors-test.js .................. 6/6
ok test/js/isolatedata-test.js .......................... 3/3
ok test/js/json-parse-test.js ........................... 9/9
ok test/js/json-stringify-test.js ..................... 23/23
ok test/js/makecallback-test.js ......................... 2/2
ok test/js/maybe-test.js ................................ 2/2
ok test/js/methodswithdata-test.js ...................... 9/9
ok test/js/morenews-test.js ........................... 17/17
ok test/js/multifile-test.js ............................ 3/3
ok test/js/namedinterceptors-test.js .................... 6/6
ok test/js/nancallback-test.js ........................ 20/20
ok test/js/nannew-test.js ............................. 95/95
ok test/js/news-test.js ............................... 53/53
ok test/js/objectwraphandle-test.js ................... 10/10
ok test/js/persistent-test.js ......................... 16/16
ok test/js/private-test.js .............................. 9/9
ok test/js/returnemptystring-test.js .................... 4/4
ok test/js/returnnull-test.js ........................... 4/4
ok test/js/returnundefined-test.js ...................... 4/4
ok test/js/returnvalue-test.js ........................ 10/10
ok test/js/setcallhandler-test.js ....................... 5/5
ok test/js/settemplate-test.js ........................ 23/23
ok test/js/strings-test.js .............................. 7/7
ok test/js/symbols-test.js .............................. 3/3
ok test/js/threadlocal-test.js .......................... 8/8
ok test/js/trycatch-test.js ............................. 3/3
ok test/js/typedarrays-test.js ........................ 29/29
ok test/js/weak2-test.js ................................ 4/4
ok test/js/weak-test.js ................................. 6/6
ok test/js/wrappedobjectfactory-test.js ................. 5/5
total ............................................... 581/581

ok

@kkoopa
Copy link
Collaborator

kkoopa commented Oct 7, 2024

Wonderful, this is looking a lot better: https://ci.appveyor.com/project/RodVagg/nan/builds/50749401 I shall take a closer look once it has finished.

@kkoopa
Copy link
Collaborator

kkoopa commented Oct 9, 2024

Looks great, I only have one small request: Could you redo the IdleNotification part like the corresponding commit I had here? #974 That is, make it a dummy function that only returns true, mark them all as NAN_DEPRECATED and apply the documentation updates?

@SchizoDuckie
Copy link

SchizoDuckie commented Oct 9, 2024

Can confirm this pr allows me to compile node_sleep on a Raspi (aarch64) running Debian Bookworm with node v20.18.0 and node-gyp v10.2.0

[edit]
Can confirm it also builds on node@22.9.0 with process.versions.modules = 127

The patch made IdleNotification deprecated and updated documents.

refs nodejs#974
@toshiyuki-rs
Copy link
Contributor Author

toshiyuki-rs commented Oct 9, 2024

I made IdleNotification deprecated and updated documents.

@kkoopa
Copy link
Collaborator

kkoopa commented Oct 9, 2024

Thank you. I will wait for the results of the regression tests to finish, then merge this and push a new release.

@kkoopa kkoopa merged commit a7df36e into nodejs:main Oct 9, 2024
@danielweck
Copy link

Unfortunately NAN v2.21.0 still fails with Electron (v32.2.0):

#973 (comment)

@danielweck danielweck mentioned this pull request Oct 16, 2024
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.

support for electron 32
5 participants