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

Fix version guard for ScriptOrigin constructors #989

Merged
merged 2 commits into from
Feb 26, 2025
Merged

Conversation

SuibianP
Copy link
Contributor

The current version guard for Isolate-less ScriptOrigin constructors checks both V8_MAJOR_VERSION > 11 and V8_MINOR_VERSION > 7. Newer V8s with a minor version not greater than 7, such as V8 v13.2.152.36 of Electron v34.2.0, still fall back to deprecated constructors removed since V8 v12.7.23 and fail to compile.

Fix the preprocessor logic to properly detect V8 v11.7+.

The current version guard for Isolate-less ScriptOrigin constructors
checks both V8_MAJOR_VERSION > 11 and V8_MINOR_VERSION > 7. Newer V8s
with a minor version not greater than 7, such as V8 v13.2.152.36 of
Electron v34.2.0, still fall back to deprecated constructors removed
since V8 v12.7.23 and fail to compile.

Fix the preprocessor logic to properly detect V8 v11.7+.
@SuibianP
Copy link
Contributor Author

Also, where did the v11.7 cutoff come from? Isolate-taking constructors were deprecate_soon'd in v8/v8@2282b3d (12.1.140), deprecated in v8/v8@0b52081 (12.6.175) and removed in v8/v8@7cd2b0c (12.7.23).

@kkoopa
Copy link
Collaborator

kkoopa commented Feb 26, 2025

The cutoff looks like a simple oversight. Thank you for fixing it. I will release a new version.

@kkoopa
Copy link
Collaborator

kkoopa commented Feb 26, 2025

@kkoopa
Copy link
Collaborator

kkoopa commented Feb 26, 2025

21.7.3 has

#define V8_MAJOR_VERSION 11
#define V8_MINOR_VERSION 8
#define V8_BUILD_NUMBER 172
#define V8_PATCH_LEVEL 17

Isolate-taking constructors were deprecate_soon'd in 2282b3d (12.1.140),
deprecated in 0b52081 (12.6.175) and removed in 7cd2b0c (12.7.23).

Corrected the version cutoff to 12.6.175 when the constructors in
question were officially deprecated.
@SuibianP
Copy link
Contributor Author

Corrected it to 12.6.175. Tested locally on Node 21 and all CI tests are passing as well.

@kkoopa kkoopa merged commit 053239d into nodejs:main Feb 26, 2025
16 checks passed
@kkoopa
Copy link
Collaborator

kkoopa commented Feb 26, 2025

LGTM. I will get this out now.

@agracio agracio mentioned this pull request Feb 28, 2025
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