-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Update V8 before the release of v19.0.0 #44650
Comments
These are all C/C++ related issues, right? (If so, sorry, I haven't used that in ~10-15 years 😞) |
Only "Build error Windows" seems like a showstopper, that's a tier 1 platform. I'll take a look. The other platforms are not tier 1. It's okay to go ahead and release v19.0.0 without binaries for them and fix it later. W.r.t. nodejs/node-v8#236: #44049 - just needs minor fix-ups for Windows, IIRC. |
I guess the "crashes with debug build" are also relevant for release binaries as these checks are an indication that something wrong is done. |
They are, but I don't think it's really serious and something we can fix over time. The problem is well-known (calling into the runtime when it's tearing down) and there's a sledgehammer fix if necessary: diff --git a/src/env-inl.h b/src/env-inl.h
index e04c0c2cf17..49c16340a67 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -607,7 +607,9 @@ void Environment::RequestInterrupt(Fn&& cb) {
}
inline bool Environment::can_call_into_js() const {
- return can_call_into_js_ && !is_stopping();
+ return can_call_into_js_ &&
+ !is_stopping() &&
+ !isolate_->IsExecutionTerminating();
}
inline void Environment::set_can_call_into_js(bool can_call_into_js) { |
Question for curiosity, absolutely not blocking: is this potentially worth shifting 19.0.0 back a week for? |
I'd go one step further: we should wait releasing v19 until this is sorted. |
@mcollina I'm not sure about dealying v19. If we can't resolve in the next month then it could be months more before we do. Since v19 is not going to be promoted to LTS I think its a bit less crucial to have the latest LTS. I do agree that keeping V8 up to date is important but delaying v19 for a long time would have consequences for our LTS release, as by policy LTS has to happen after the new current, and also by policy things need to bake in Current, so Node.js 18 would stagnate. We've worked quite hard to make our releases predictable and to stick to the target dates. |
And to clarify, I'm not necessary concerned about a 1 week slip to give us a week, but delaying indefintely is a concern. |
Hi folks.
if v19 is not LTS would it be possible to bring the v8 bump in a later 19.x.x ? (a week later) then there would at least give the new v8 time to bake. |
Current versions of Node.js are subject to semver as much as LTS ones, if upgrading V8 is a breaking change (and it likely will be), it won't be able to land. There are of course ways to workaround this (e.g. by applying patches on top of V8 to address the breaking changes), and the TSC can also decide to disregard semver rules and land it even if it's breaking (there's precedent for it), but it would be better for everyone to have an up-to-date version of V8 already on Maybe discussions regarding the release schedule should be taken to nodejs/Release, and we should keep this issue focus on fixing the V8 issues. |
IMO no we should not do mid-line upgrade, it'd need to go into 20 (which would be 6 months away). |
@bahamat can you help resolve the smartos issue? |
@mhdawson Yes, I’ll see what we can do. |
Currently, V8 on
main
is at version 10.2.154.15 (the same as v18.x).The stable V8 version today is 10.5 and 10.7 will be stable on Oct 18 (the release date of Node.js 19.0.0).
Unfortunately, a few issues have been preventing us from upgrading for months and they are still unresolved.
Keeping V8 up to date is essential to Node.js' success in the long term, so it would be very unfortunate to release v19.0.0 without a newer version.
Here are the issues blocking the upgrade:
CI is still running with the latest canary (https://ci.nodejs.org/job/node-test-commit/56400/). I hope it won't find other issues.
/cc @nodejs/collaborators @nodejs/tsc @nodejs/v8 any help will be appreciated.
The text was updated successfully, but these errors were encountered: