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

[v10.x] deps: increase V8 deprecation levels #23159

Closed
wants to merge 17 commits into
base: v10.x-staging
from

Conversation

Projects
@addaleax
Member

addaleax commented Sep 29, 2018

Bump from V8_DEPRECATE_SOON to V8_DEPRECATE for APIs
that are removed in V8 7.0+. Also, provide the replacement APIs
in cases where they were absent on Node 10.

Refs: #23122

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

@addaleax addaleax requested review from hashseed and targos Sep 29, 2018

@targos

targos approved these changes Sep 29, 2018

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Sep 29, 2018

Member

Sample output from windows (screenshots for color effect ;) node@10.11.0
nan test with this PR:
image

nan test with just 'V8_IMMINENT_DEPRECATION_WARNINGS=1'
image

Member

refack commented Sep 29, 2018

Sample output from windows (screenshots for color effect ;) node@10.11.0
nan test with this PR:
image

nan test with just 'V8_IMMINENT_DEPRECATION_WARNINGS=1'
image

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 30, 2018

Member

@refack Thanks, I’ve undone the changes to v8-platform.h. There might be other ways to get this done without causing deprecation warnings for just including the header, but since the V8 platform API targets embedders and not addons, I’d guess that it’s fine to not care about them in this patch.

Member

addaleax commented Sep 30, 2018

@refack Thanks, I’ve undone the changes to v8-platform.h. There might be other ways to get this done without causing deprecation warnings for just including the header, but since the V8 platform API targets embedders and not addons, I’d guess that it’s fine to not care about them in this patch.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@@ -2227,6 +2227,10 @@ int PrimitiveArray::Length() const {
return array->length();
}
void PrimitiveArray::Set(Isolate* isolate, int index, Local<Primitive> item) {

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 1, 2018

Member

This is problematic because it means prebuilt add-ons that call these methods won't work with older Node.js v10.x releases that don't have them. The options I see:

  1. Don't warn.
  2. Don't care.

There probably aren't too many users of these methods, possibly none at all.

@bnoordhuis

bnoordhuis Oct 1, 2018

Member

This is problematic because it means prebuilt add-ons that call these methods won't work with older Node.js v10.x releases that don't have them. The options I see:

  1. Don't warn.
  2. Don't care.

There probably aren't too many users of these methods, possibly none at all.

This comment has been minimized.

@addaleax

addaleax Oct 1, 2018

Member

This is problematic because it means prebuilt add-ons that call these methods won't work with older Node.js v10.x releases that don't have them.

I mean, that’s a general issue with semver-minor patches for the C++ API, right? I’m not too worried here. If you think we should do something, I guess we could include the Node.js version number in the warning message?

@addaleax

addaleax Oct 1, 2018

Member

This is problematic because it means prebuilt add-ons that call these methods won't work with older Node.js v10.x releases that don't have them.

I mean, that’s a general issue with semver-minor patches for the C++ API, right? I’m not too worried here. If you think we should do something, I guess we could include the Node.js version number in the warning message?

@targos targos added this to Open PRs in v10.x Oct 3, 2018

Trott and others added some commits Aug 12, 2018

doc: leave pull requests open for 72 hours
Currently, we have a 48/72 rule for how many hours a pull request should
be left open at a minimum. Unfortunately, whether a pull request should
be left open for 48 or 72 hours is often unclear. The 72 hours is
required if it is a weekend. If I open a pull request on a Friday
morning, does it need to stay open 48 hours or 72 or something in
between? Does it matter if I'm in one time zone or another?

The 48/72 rule predates our fast-tracking process. Given the ability to
fast-track trivial pull requests, there should be little disadvantage to
leaving significant changes open for 72 hours instead of 48 hours, and
arguably considerable advantage in terms of allowing people sufficient
time to review things.

So to simplify, standardize on 72 hours. Weekend or not, 72 hours. Easy.

PR-URL: #22275
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Revert "tty: make _read throw ERR_TTY_WRITABLE_NOT_READABLE"
This reverts commit 91eec00.

Refs: #21654
Refs: #21203

PR-URL: #23053
Reviewed-By: James M Snell <jasnell@gmail.com>
tools: allow input for TTY tests
Since faking TTY input is not otherwise fake-able, we need
support in the test runner for it.

PR-URL: #23053
Reviewed-By: James M Snell <jasnell@gmail.com>
process: allow reading from stdout/stderr sockets
Allow reading from stdio streams that are conventionally
associated with process output, since this is only convention.

This involves disabling the oddness around closing stdio
streams. Its purpose is to prevent the file descriptors
0 through 2 from being closed, since doing so can lead
to information leaks when new file descriptors are being
opened; instead, not doing anything seems like a more
reasonable choice.

Fixes: #21203

PR-URL: #23053
Reviewed-By: James M Snell <jasnell@gmail.com>
test: add stdin writable regression test
Make sure that `process.stdin.write()`, and in particular
ending the stream, works.

PR-URL: #23053
Reviewed-By: James M Snell <jasnell@gmail.com>
tools: remove useless assignment from configure.py
PR-URL: #23200
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
build: add pgo specific variables to common.gypi
Refs: #22772 (comment)

PR-URL: #23102
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
build: cleanup in .gitignore
* explicitly unignore files that we track.

The following are not created anymore (only as subdirs of v8/gypfiles)
/deps/v8/src/debug/obj
deps/v8/src/Debug/
deps/v8/src/Release/
deps/v8/src/inspector/Debug/
deps/v8/src/inspector/Release/

PR-URL: #23180
Refs: #23156
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
inspector: add virtual destructor to WorkerDelegate
Currently the WorkerDelegate class has a virtual function
but no virtual destructor which means that if delete is called on a
WorkerDelegate pointer to a derived instance, the derived destructor
will not get called.

The following warning is currently being printed when
compiling:

warning: delete called on 'node::inspector::WorkerDelegate' that is
abstract but has non-virtual destructor [-Wdelete-non-virtual-dtor]
    delete __ptr;
    ^

This commit adds a virtual destructor.

PR-URL: #23215
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
crypto: add virtual dtor to KeyPairGenerationConfig
Currently the KeyPairGenerationConfigs class has a virtual function
but no virtual destructor which means that if delete is called on a
KeyPairGenerationConfig pointer to a derived instance, the derived
destructor will not get called.

The following warning is currently being printed when
compiling:

warning: delete called on 'node::crypto::KeyPairGenerationConfig' that
is abstract but has non-virtual destructor [-Wdelete-non-virtual-dtor]
    delete __ptr;

This commit adds a virtual destructor.

PR-URL: #23215
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
src: add virtual desctructor to Options class
Currently the Options class has a virtual function but no virtual
destructor which means that if delete is called on a Options pointer
to a derived instance, the derived destructor will not get called.

The following warning is currently being printed when
compiling:

warning: delete called on non-final 'node::PerIsolateOptions' that has
virtual functions but non-virtual destructor [-Wdelete-non-virtual-dtor]
    delete __ptr;

This commit adds a virtual destructor.

PR-URL: #23215
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
build: make lint-addon-docs quiet
This commit adds the --quiet flag to cpplint for the lint-addon-docs
target to be consistent with the lint-cpp target.

PR-URL: #23217
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
tools: fix ICU shrinker and docs
- tools: path to ICU datafile moved
- docs: configure is now configure.py

Fixes: #23245

PR-URL: #23266
Reviewed-By: Refael Ackermann <refack@gmail.com>
build: toggle lint-cpp using verbose (V) variable
This commit the verbosity of cpplint to be toggled by using the V
variable. The default setting is verbose but by passing an empty string
cpplint will be quiet.

PR-URL: #23217
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
assert: improve diff output
The output is now a tiny bit improved by sorting object properties
when inspecting the values that are compared with each other. That
reduces the overall diff for identical objects with a different
property insertion order.

Backport-PR-URL: #23226
PR-URL: #22788
Refs: #22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
util: move inspect in separate file
The inspect function became very big and it's better to handle this
in a separate file.

Backport-PR-URL: #23226
PR-URL: #22845
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Oct 5, 2018

Member

This needs a rebase and another approval.

Member

targos commented Oct 5, 2018

This needs a rebase and another approval.

deps: increase V8 deprecation levels
Bump from `V8_DEPRECATE_SOON` to `V8_DEPRECATE` for APIs
that are removed in V8 7.0+. Also, provide the replacement APIs
in cases where they were absent on Node 10.

Refs: #23122
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax
Member

addaleax commented Oct 6, 2018

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Oct 7, 2018

Member

@addaleax do you think this can land?

Member

targos commented Oct 7, 2018

@addaleax do you think this can land?

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Oct 7, 2018

Member

(It should land with a bump to V8's embedder string)

Member

targos commented Oct 7, 2018

(It should land with a bump to V8's embedder string)

addaleax added a commit that referenced this pull request Oct 7, 2018

deps: increase V8 deprecation levels
Bump from `V8_DEPRECATE_SOON` to `V8_DEPRECATE` for APIs
that are removed in V8 7.0+. Also, provide the replacement APIs
in cases where they were absent on Node 10.

Refs: #23122
PR-URL: #23159
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Oct 7, 2018

Member

@targos Yes, landed in 46c7d0d on v10.x-staging with the embedder string bumped 👍

Member

addaleax commented Oct 7, 2018

@targos Yes, landed in 46c7d0d on v10.x-staging with the embedder string bumped 👍

@addaleax addaleax closed this Oct 7, 2018

@addaleax addaleax deleted the addaleax:v10.x-v8-dep branch Oct 7, 2018

targos added a commit that referenced this pull request Oct 10, 2018

2018-10-10, Version 10.12.0 (Current)
Notable changes:

* assert
  * The diff output is now a tiny bit improved by sorting object
    properties when inspecting the values that are compared with each
    other. #22788
* cli
  * The options parser now normalizes `_` to `-` in all multi-word
    command-line flags, e.g. `--no_warnings` has the same effect as
    `--no-warnings`. #23020
  * Added bash completion for the `node` binary. To generate a bash
    completion script, run `node --completion-bash`. The output can be
    saved to a file which can be sourced to enable completion.
    #20713
* crypto
  * Added support for PEM-level encryption.
    #23151
  * Added an API asymmetric key pair generation. The new methods
    `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be
    used to generate public and private key pairs. The API supports
    RSA, DSA and EC and a variety of key encodings (both PEM and DER).
    #22660
* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to true, non-existing parent folders will be
    automatically created. #21875
* http2
  * Added a `'ping'` event to `Http2Session` that is emitted whenever a
    non-ack `PING` is received.
    #23009
  * Added support for the `ORIGIN` frame.
    #22956
  * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect
    protocol support to allow use of WebSockets over HTTP/2.
    #23284
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom require function that will resolve
    modules relative to the filename path.
    #19360
* process
  * Added a `'multipleResolves'` process event that is emitted whenever
    a `Promise` is attempted to be resolved multiple times, e.g. if the
    `resolve` and `reject` functions are both called in a `Promise`
    executor. #22218
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between file: URLs and
    absolute paths. #22506
* util
  * Added the `sorted` option to `util.inspect()`. If set to `true`,
    all properties of an object and Set and Map entries will be sorted
    in the returned string. If set to a function, it is used as a
    compare function. #22788
  * The `util.instpect.custom` symbol is now defined in the global
    symbol registry as `Symbol.for('nodejs.util.inspect.custom')`.
    #20857
  * Added support for `BigInt` numbers in `util.format()`.
    #22097
* V8 API
  * A number of V8 C++ APIs have been marked as deprecated since they
    have been removed in the upstream repository. Replacement APIs
    are added where necessary. #23159
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Workers
  * Debugging support for Workers using the DevTools protocol has been
    implemented. #21364
  * The public `inspector` module is now enabled in Workers.
    #22769
* Added new collaborators:
  * digitalinfinity - Hitesh Kanwathirtha

PR-URL: #23313

targos added a commit that referenced this pull request Oct 10, 2018

2018-10-10, Version 10.12.0 (Current)
Notable changes:

* assert
  * The diff output is now a tiny bit improved by sorting object
    properties when inspecting the values that are compared with each
    other. #22788
* cli
  * The options parser now normalizes `_` to `-` in all multi-word
    command-line flags, e.g. `--no_warnings` has the same effect as
    `--no-warnings`. #23020
  * Added bash completion for the `node` binary. To generate a bash
    completion script, run `node --completion-bash`. The output can be
    saved to a file which can be sourced to enable completion.
    #20713
* crypto
  * Added support for PEM-level encryption.
    #23151
  * Added an API asymmetric key pair generation. The new methods
    `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be
    used to generate public and private key pairs. The API supports
    RSA, DSA and EC and a variety of key encodings (both PEM and DER).
    #22660
* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to true, non-existing parent folders will be
    automatically created. #21875
* http2
  * Added a `'ping'` event to `Http2Session` that is emitted whenever a
    non-ack `PING` is received.
    #23009
  * Added support for the `ORIGIN` frame.
    #22956
  * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect
    protocol support to allow use of WebSockets over HTTP/2.
    #23284
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom require function that will resolve
    modules relative to the filename path.
    #19360
* process
  * Added a `'multipleResolves'` process event that is emitted whenever
    a `Promise` is attempted to be resolved multiple times, e.g. if the
    `resolve` and `reject` functions are both called in a `Promise`
    executor. #22218
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between file: URLs and
    absolute paths. #22506
* util
  * Added the `sorted` option to `util.inspect()`. If set to `true`,
    all properties of an object and Set and Map entries will be sorted
    in the returned string. If set to a function, it is used as a
    compare function. #22788
  * The `util.instpect.custom` symbol is now defined in the global
    symbol registry as `Symbol.for('nodejs.util.inspect.custom')`.
    #20857
  * Added support for `BigInt` numbers in `util.format()`.
    #22097
* V8 API
  * A number of V8 C++ APIs have been marked as deprecated since they
    have been removed in the upstream repository. Replacement APIs
    are added where necessary. #23159
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Workers
  * Debugging support for Workers using the DevTools protocol has been
    implemented. #21364
  * The public `inspector` module is now enabled in Workers.
    #22769
* Added new collaborators:
  * digitalinfinity - Hitesh Kanwathirtha

PR-URL: #23313

targos added a commit that referenced this pull request Oct 10, 2018

2018-10-10, Version 10.12.0 (Current)
Notable changes:

* assert
  * The diff output is now a tiny bit improved by sorting object
    properties when inspecting the values that are compared with each
    other. #22788
* cli
  * The options parser now normalizes `_` to `-` in all multi-word
    command-line flags, e.g. `--no_warnings` has the same effect as
    `--no-warnings`. #23020
  * Added bash completion for the `node` binary. To generate a bash
    completion script, run `node --completion-bash`. The output can be
    saved to a file which can be sourced to enable completion.
    #20713
* crypto
  * Added support for PEM-level encryption.
    #23151
  * Added an API asymmetric key pair generation. The new methods
    `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be
    used to generate public and private key pairs. The API supports
    RSA, DSA and EC and a variety of key encodings (both PEM and DER).
    #22660
* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to true, non-existing parent folders will be
    automatically created. #21875
* http2
  * Added a `'ping'` event to `Http2Session` that is emitted whenever a
    non-ack `PING` is received.
    #23009
  * Added support for the `ORIGIN` frame.
    #22956
  * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect
    protocol support to allow use of WebSockets over HTTP/2.
    #23284
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom require function that will resolve
    modules relative to the filename path.
    #19360
* process
  * Added a `'multipleResolves'` process event that is emitted whenever
    a `Promise` is attempted to be resolved multiple times, e.g. if the
    `resolve` and `reject` functions are both called in a `Promise`
    executor. #22218
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between file: URLs and
    absolute paths. #22506
* util
  * Added the `sorted` option to `util.inspect()`. If set to `true`,
    all properties of an object and Set and Map entries will be sorted
    in the returned string. If set to a function, it is used as a
    compare function. #22788
  * The `util.instpect.custom` symbol is now defined in the global
    symbol registry as `Symbol.for('nodejs.util.inspect.custom')`.
    #20857
  * Added support for `BigInt` numbers in `util.format()`.
    #22097
* V8 API
  * A number of V8 C++ APIs have been marked as deprecated since they
    have been removed in the upstream repository. Replacement APIs
    are added where necessary. #23159
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Workers
  * Debugging support for Workers using the DevTools protocol has been
    implemented. #21364
  * The public `inspector` module is now enabled in Workers.
    #22769
* Added new collaborators:
  * digitalinfinity - Hitesh Kanwathirtha

PR-URL: #23313
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment