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

deps,v8: silence V8 self-deprecation warnings #25394

Closed
wants to merge 1 commit into from

Conversation

Projects
8 participants
@refack
Copy link
Member

commented Jan 8, 2019

With current config we get C++ deprecation warning while building V8 from it's own code for use of methods that the V8 team deprecated. IMO this is not directly informative for node's build process, so this PR silences these warnings, unless explicitly configured to show them.

CI: https://ci.nodejs.org/job/node-test-pull-request/19994/

/CC @nodejs/v8 @nodejs/build-files

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@ryzokuken
Copy link
Member

left a comment

Harsh but fair. It's not really up to contributors to core to fix those anyway.

@jasnell

jasnell approved these changes Jan 8, 2019

@mhdawson

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

@refack just to clarify, these are warnings only for V8 code itself, not Node.js using V8 deprecated functions right?

@addaleax

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

@refack As I understand it, this would disable deprecation warnings for addons as well, unless they explicitly enable it? How are addon authors going to know that their code will be broken?

@refack

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2019

just to clarify, these are warnings only for V8 code itself, not Node.js using V8 deprecated functions right?

As I understand it, this would disable deprecation warnings for addons as well, unless they explicitly enable it?

These settings are just for the the compilation of the V8 code itself (I should not have turned them on in the first place when we migrated them from the GN setup).

For all other code (the files in /src/ and addons) to get v8.h to emit those warnings we explicitly define these flags in

node/common.gypi

Lines 300 to 305 in fe7ce8c

# Defines these mostly for node-gyp to pickup, and warn addon authors of
# imminent V8 deprecations, also to sync how dependencies are configured.
'defines': [
'V8_DEPRECATION_WARNINGS',
'V8_IMMINENT_DEPRECATION_WARNINGS',
],

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/20077/

@addaleax

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

Landed in d1b7193

@addaleax addaleax closed this Jan 14, 2019

addaleax added a commit that referenced this pull request Jan 14, 2019

deps,v8: silence V8 self-deprecation warnings
PR-URL: #25394
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>

@refack refack added the landed label Jan 14, 2019

@refack refack deleted the refack:v8-dont-self-deprecate branch Jan 14, 2019

@addaleax

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

I’m labelling this dont-land-on-v11.x because it doesn’t apply cleanly and, since it’s developer-targeting, shouldn’t cause trouble to not have it in v11.x. Feel free to backport it if that makes sense to you, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.