changes to support Node.js v16 & v18 #404
Conversation
kvakil
changed the title
kvakil
changed the title
Codecov Report
@@ Coverage Diff @@
## main #404 +/- ##
==========================================
- Coverage 77.52% 73.14% -4.38%
==========================================
Files 34 34
Lines 5027 5072 +45
==========================================
- Hits 3897 3710 -187
- Misses 1130 1362 +232
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@No9 this is now ready for review! I split it into four commits, but I can also split it into four separate PRs if you'd prefer that. |
|
Hey @kvakil Stoked you've got so far on this. |
Without this, `nodejsVersion()` returns `[NaN]` when using a release build of Node.js.
I still expect them to be red, but enable them anyway.
Exclude a couple of configurations which error with a glibc error
("version `GLIBC_2.28` not found (required by node)").
|
I added the later versions (16/18). I am not really sure if this diff means they should be added in CI, since they are still failing(?) But you can see the progress made since they are not completely failing anymore.
I excluded some combinations which looked like they were failing for unrelated reasons (see the comments).
|
|
@No9 Given https://nodejs.org/en/about/releases should unsupported Node.js releases < v14 continue to be tested? |
|
@cclauss We started a discussion on what we can support at the last collab meeting. @kvakil It seems like 16 is a lot closer than 18 right now. Given that this PR is to |
As of the mentioned upstream commit, the hack we use to detect the possibility of unboxed doubles no longer works. Rather than trying to create another hack, simply hardcode it to false. This requires us to also remove support for EOL versions of Node.js which may still have unboxed doubles.
`NativeModule` appears to no longer show up in the heap. Neither do promises. I'm commenting out the latter test for now, the current promise support is extremely rudimentary and somebody should decide if it's worth fixing.
|
I pushed this along a little further. Some notes:
Looking through the remaining features which are still broken, I really don't think we need to support them; they all look more like nice-to-haves. I'd much rather just skip them & get this merged in. Thoughts? |
|
Refs: nodejs#403 Co-authored-by: Christian Clauss <[email protected]> Co-authored-by: Anton Whalley <[email protected]>
kvakil
changed the title
This fails as of v18.6.0.
|
I pushed this along a little further, skipping basically any failures which I felt weren't important and taking the changes & suggestions from #403. I'll try to capture some more details about the tests I skipped and why I did so later. I think all tests should be green now, although there is still some flakiness even after bumping the timeouts. Meta note: I have been pretty sick this past week and am still not feeling great, so may be a bit slow to respond. Feel free to directly push changes to my branch if that's easier. |
|
Re-ran the test and it's now all green 🥳 Meta notes:
|
|
This works for me on node16 after compiling from source code. Hope it will be merged into master branch asap. |
|
What are the challenges of using |
|
@RogerKang thanks for testing - I haven't been able to get the impact assessment done for the release announcement So I might just merge and publish this as 4.0.0 and do the notes as a follow up issue. |
|
(Meta note: I'm mostly feeling better. :) As promised, here's a summary of changes in this PR & the user impact:
I think it should work fine: you need to compile from source & point $ npm install --nodedir=~/node-source .The Node.js 19 builds are just disabled here because of some CI issue, not because of any intrinsic difficulty. |
|
Excellent - I released 3.3.0 last night to have a release that works with older nodes but newer lldb and verify the signing process. |
After nodejs#404, we no longer support unboxed doubles. Let's remove the relevant code.
After nodejs#404, we no longer support unboxed doubles. Let's remove the relevant code.
I disabled this in nodejs#404. This is a PR so I can futz around with it and try to figure out why the mirror isn't working.
After #404, we no longer support unboxed doubles. Let's remove the relevant code.
With this diff, our tests are much better. Previously they were entirely
failing on Node 16. Now:
stack-testpasses (12/12)inspect-testmostly passes (57/59)frame-testsomewhat passes (7/25)I marked the failing tests as skipped / optional.
There are a couple of big commits in this diff. It is best reviewed
commit-by-commit. They are in order:
Postmortem data fixes
class_Map__constructor_or_backpointer__Objectis nowclass_Map__constructor_or_back_pointer__Object(note the extra_).class_Script__source__Objectis weirdly not present in Node 16but is present in both Node 14 and Node 18. I added a default to
the correct value of 8.
ScopeInfois no longer aFixedArrayas of v8/v8@ecaac329. TheLengthfield was also removed in v8/v8@f731e13f.that a bunch of offsets changed.
ScopeInfois no longer aFixedArray, I changed callsitesto use
HeapObject::LoadFieldValuerather thanFixedArray::Get.Changes to V8 calling conventions
frame-test.jsto match.JSFrame::LeaParamSlotto match.A minor fix to
nodejsVersion, which was returning[NaN]whenrun on a release build.
Remove support for unboxed doubles, which fixes printing doubles
since V8 broke our previous detection mechanism.
Remove a couple of tests which seemed unnecessary (
NativeModuleand
Promise).