deps: provide more V8 backwards compatibility by addaleax · Pull Request #23158 · nodejs/node

addaleax · 2018-09-29T02:54:25Z

Add back a number deprecated APIs, using shims that
should work well enough at least for the duration of Node 11
and do not come with significant maintenance overhead.

Refs: #23122

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2018-09-29T02:54:26Z

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1058/pipeline

Trott

LGTM. Worth doing a CITGM run?

targos · 2018-09-29T09:21:19Z

/cc @nodejs/v8-update

targos · 2018-09-29T09:27:05Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1565/

addaleax · 2018-09-29T14:06:52Z

CITGM looks a lot nicer now – still a bunch of failures, but nothing related to the V8 changes.

CI: https://ci.nodejs.org/job/node-test-pull-request/17506/ (:heavy_check_mark:)

refack · 2018-09-29T16:54:09Z

I think we can simply turn on all the V8_DEPRECATE_SOON to produce deprecation warnings
by setting 'v8_imminent_deprecation_warnings': 1, in common.gypi

node/deps/v8/gypfiles/features.gypi

Lines 137 to 139 in a21af5b

    
           ['v8_imminent_deprecation_warnings==1', { 
        
             'defines': ['V8_IMMINENT_DEPRECATION_WARNINGS',], 
        
           }],

node/deps/v8/include/v8config.h

Lines 337 to 349 in a21af5b

    
           // A macro (V8_DEPRECATE_SOON) to make it easier to see what will be deprecated. 
        
           #if defined(V8_IMMINENT_DEPRECATION_WARNINGS) && \ 
        
               V8_HAS_ATTRIBUTE_DEPRECATED_MESSAGE 
        
           #define V8_DEPRECATE_SOON(message, declarator) \ 
        
             declarator __attribute__((deprecated(message))) 
        
           #elif defined(V8_IMMINENT_DEPRECATION_WARNINGS) && V8_HAS_ATTRIBUTE_DEPRECATED 
        
           #define V8_DEPRECATE_SOON(message, declarator) \ 
        
             declarator __attribute__((deprecated)) 
        
           #elif defined(V8_IMMINENT_DEPRECATION_WARNINGS) && V8_HAS_DECLSPEC_DEPRECATED 
        
           #define V8_DEPRECATE_SOON(message, declarator) __declspec(deprecated) declarator 
        
           #else 
        
           #define V8_DEPRECATE_SOON(message, declarator) declarator 
        
           #endif

edit

Need to explicitly add 'V8_IMMINENT_DEPRECATION_WARNINGS=1' to defines in:

node/common.gypi

Line 107 in a21af5b

'defines': [ 'DEBUG', '_DEBUG', 'V8_ENABLE_CHECKS' ],

and
'defines': [ 'V8_IMMINENT_DEPRECATION_WARNINGS=1' ], somewhere in

node/common.gypi

Lines 164 to 168 in a21af5b

    
           'Release': { 
        
             'variables': { 
        
               'v8_enable_handle_zapping': 0, 
        
             }, 
        
             'cflags': [ '-O3' ],

addaleax · 2018-09-30T00:25:13Z

I think setting v8_imminent_deprecation_warnings would probably be too noisy for now? Feel free to open a PR with it if you think otherwise, but it seems independent of this PR.

refack · 2018-09-30T01:21:51Z

but it seems independent of this PR.

Discussion in #23167

addaleax · 2018-10-03T14:48:31Z

(please make sure the discussion in #23122 about conflicting with V8 updates is resolved before landing this)

jasnell · 2018-10-03T14:59:49Z

Does this need to be on the 11.0.0 milestone?

addaleax · 2018-10-03T15:04:48Z

@jasnell Yes – it unbreaks a large number of native add-ons, and in particular makes CITGM return more meaningful results again.

thefourtheye

I am no expert in this area, but the reason we are doing this and the changes are straight forward. So LGTM.

Refs: v8/v8@7.0.276.22...7.0.276.24

Reverting this enables us to provide slower, but longer-lasting replacements for the deprecated APIs. Original commit message: Put back deleted V8_DEPRECATE_SOON methods This partially reverts https://chromium-review.googlesource.com/c/v8/v8/+/1177861, which deleted many V8_DEPRECATE_SOON methods rather than moving them to V8_DEPRECATED first. This puts them back and marks them V8_DEPRECATED. Note V8_DEPRECATED that were deleted in the same CL stay deleted. NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Bug: v8:7786, v8:8240 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I00330036d957f98dab403465b25e30d8382aac22 Reviewed-on: https://chromium-review.googlesource.com/1251422 Commit-Queue: Dan Elphick <[email protected]> Reviewed-by: Yang Guo <[email protected]> Reviewed-by: Michael Hablich <[email protected]> Cr-Commit-Position: refs/branch-heads/7.0@{nodejs#49} Cr-Branched-From: 6e2adae6f7f8e891cfd01f3280482b20590427a6-refs/heads/7.0.276@{#1} Cr-Branched-From: bc08a8624cbbea7a2d30071472bc73ad9544eadf-refs/heads/master@{nodejs#55424} Refs: v8/v8@9136dd8 Refs: nodejs#23122

Add back a number deprecated APIs, using shims that should work well enough at least for the duration of Node 11 and do not come with significant maintenance overhead. Refs: nodejs#23122

addaleax · 2018-10-04T21:42:22Z

@hashseed @targos I’ve updated this – does this solution seem okay to you?

CI: https://ci.nodejs.org/job/node-test-pull-request/17637/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1730/

hashseed · 2018-10-05T06:53:32Z

deps/v8/src/api.cc


+bool Value::BooleanValue() const {
+  return BooleanValue(Isolate::GetCurrent()->GetCurrentContext())
+      .FromMaybe(false);


Depending on what behavior you prefer, you may want to use FromJust() here to cause a crash rather than failing silently.

Done – this is currently only theoretical, right?

Ah. Sorry. I actually meant all the other uses of FromMaybe, like in Value::IntegerValue. BooleanValue can actually never throw, so in newer V8 we restored this API already, without the Maybe return value.

Okay, I understand – that would at least technically be a breaking change, I guess, and we’re past the cut-off date for that for Node 11

addaleax · 2018-10-06T00:14:06Z

Landed in 2ba19ff...48d1335

Refs: v8/v8@7.0.276.22...7.0.276.24 PR-URL: #23158 Refs: #23122 Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Reverting this enables us to provide slower, but longer-lasting replacements for the deprecated APIs. Original commit message: Put back deleted V8_DEPRECATE_SOON methods This partially reverts https://chromium-review.googlesource.com/c/v8/v8/+/1177861, which deleted many V8_DEPRECATE_SOON methods rather than moving them to V8_DEPRECATED first. This puts them back and marks them V8_DEPRECATED. Note V8_DEPRECATED that were deleted in the same CL stay deleted. NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Bug: v8:7786, v8:8240 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I00330036d957f98dab403465b25e30d8382aac22 Reviewed-on: https://chromium-review.googlesource.com/1251422 Commit-Queue: Dan Elphick <[email protected]> Reviewed-by: Yang Guo <[email protected]> Reviewed-by: Michael Hablich <[email protected]> Cr-Commit-Position: refs/branch-heads/7.0@{#49} Cr-Branched-From: 6e2adae6f7f8e891cfd01f3280482b20590427a6-refs/heads/7.0.276@{#1} Cr-Branched-From: bc08a8624cbbea7a2d30071472bc73ad9544eadf-refs/heads/master@{#55424} Refs: v8/v8@9136dd8 Refs: #23122 PR-URL: #23158 Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Add back a number deprecated APIs, using shims that should work well enough at least for the duration of Node 11 and do not come with significant maintenance overhead. Refs: #23122 PR-URL: #23158 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Refs: v8/v8@7.0.276.22...7.0.276.24 PR-URL: #23158 Refs: #23122 Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Reverting this enables us to provide slower, but longer-lasting replacements for the deprecated APIs. Original commit message: Put back deleted V8_DEPRECATE_SOON methods This partially reverts https://chromium-review.googlesource.com/c/v8/v8/+/1177861, which deleted many V8_DEPRECATE_SOON methods rather than moving them to V8_DEPRECATED first. This puts them back and marks them V8_DEPRECATED. Note V8_DEPRECATED that were deleted in the same CL stay deleted. NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Bug: v8:7786, v8:8240 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I00330036d957f98dab403465b25e30d8382aac22 Reviewed-on: https://chromium-review.googlesource.com/1251422 Commit-Queue: Dan Elphick <[email protected]> Reviewed-by: Yang Guo <[email protected]> Reviewed-by: Michael Hablich <[email protected]> Cr-Commit-Position: refs/branch-heads/7.0@{#49} Cr-Branched-From: 6e2adae6f7f8e891cfd01f3280482b20590427a6-refs/heads/7.0.276@{#1} Cr-Branched-From: bc08a8624cbbea7a2d30071472bc73ad9544eadf-refs/heads/master@{#55424} Refs: v8/v8@9136dd8 Refs: #23122 PR-URL: #23158 Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Add back a number deprecated APIs, using shims that should work well enough at least for the duration of Node 11 and do not come with significant maintenance overhead. Refs: #23122 PR-URL: #23158 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

This was introduced in 48d1335. Previously, values such as `undefined` would not be coerced properly because `NumberValue()` returns `NaN` for them. Refs: nodejs#23158

This was introduced in 48d1335. Previously, values such as `undefined` would not be coerced properly because `NumberValue()` returns `NaN` for them. Refs: #23158 PR-URL: #23898 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>

Add back a number deprecated APIs, using shims that should work well enough at least for the duration of Node 11 and do not come with significant maintenance overhead. Refs: nodejs/node#23122 PR-URL: nodejs/node#23158 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Patch-Filename: deps_provide_more_v8_backwards_compatibility.patch

addaleax requested review from hashseed and targos September 29, 2018 02:54

nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Sep 29, 2018

addaleax mentioned this pull request Sep 29, 2018

APIs removed in V8 7.0 and native addons #23122

Closed

Trott approved these changes Sep 29, 2018

View reviewed changes

joyeecheung approved these changes Sep 29, 2018

View reviewed changes

cjihrig approved these changes Sep 29, 2018

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 29, 2018

jasnell approved these changes Oct 1, 2018

View reviewed changes

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 3, 2018

addaleax added this to the 11.0.0 milestone Oct 3, 2018

thefourtheye approved these changes Oct 4, 2018

View reviewed changes

targos and others added 3 commits October 4, 2018 14:38

deps: patch V8 to 7.0.276.24

3e3204f

Refs: v8/v8@7.0.276.22...7.0.276.24

deps: provide more V8 backwards compatibility

7a0dd5b

Add back a number deprecated APIs, using shims that should work well enough at least for the duration of Node 11 and do not come with significant maintenance overhead. Refs: nodejs#23122

addaleax force-pushed the v8-dep-back branch from 55591df to 7a0dd5b Compare October 4, 2018 21:40

hashseed approved these changes Oct 5, 2018

View reviewed changes

fixup! deps: provide more V8 backwards compatibility

cf7bb97

targos approved these changes Oct 5, 2018

View reviewed changes

addaleax closed this Oct 6, 2018

addaleax deleted the v8-dep-back branch October 6, 2018 00:14

addaleax pushed a commit that referenced this pull request Oct 6, 2018

deps: patch V8 to 7.0.276.24

3f77720

Refs: v8/v8@7.0.276.22...7.0.276.24 PR-URL: #23158 Refs: #23122 Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

addaleax added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 6, 2018

targos added dont-land-on-v6.x labels Oct 6, 2018

jasnell pushed a commit that referenced this pull request Oct 17, 2018

deps: patch V8 to 7.0.276.24

d25646b

Refs: v8/v8@7.0.276.22...7.0.276.24 PR-URL: #23158 Refs: #23122 Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

addaleax mentioned this pull request Oct 26, 2018

deps: fix shim for v8::Value::IntegerValue() #23898

Closed

2 tasks

Uh oh!

Conversation

addaleax commented Sep 29, 2018

Checklist

Uh oh!

nodejs-github-bot commented Sep 29, 2018

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Sep 29, 2018

Uh oh!

targos commented Sep 29, 2018

Uh oh!

addaleax commented Sep 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Sep 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

edit

Uh oh!

addaleax commented Sep 30, 2018

Uh oh!

refack commented Sep 30, 2018

Uh oh!

addaleax commented Oct 3, 2018

Uh oh!

jasnell commented Oct 3, 2018

Uh oh!

addaleax commented Oct 3, 2018

Uh oh!

thefourtheye left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 4, 2018

Uh oh!

hashseed Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

hashseed Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

addaleax commented Sep 29, 2018 •

edited

Loading

refack commented Sep 29, 2018 •

edited

Loading