build: add common `defines` #23426

refack · 2018-10-11T18:17:40Z

V8_DEPRECATION_WARNINGS is here for explicity. It is already defined
in node.gypi, and addon.gypi.
V8_IMMINENT_DEPRECATION_WARNINGS added to warn addons authors.
OPENSSL_THREADS apears in the openSSL .h files, and was only
defined via GYP for the node build.

Refs: #23122
Refs: #23414 (comment)
Fixes: #23167

Checklist

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

nodejs-github-bot · 2018-10-11T18:17:41Z

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

refack · 2018-10-11T18:18:26Z

/CC @nodejs/build-files @nodejs/addon-api @nodejs/node-gyp

refack · 2018-10-11T18:21:02Z

Currently this will cause a lot of deprecation warning for the node build:

Number	Method
3	~~`v8::Date::New`~~ #23288
2	`v8::Function::Call`
29	`v8::Object::Get`
1	`v8::Object::GetPropertyNames`
147	`v8::Object::Set`
2	`v8::String::NewFromTwoByte`
2	`v8::Value::ToObject`
(2)	`v8::WasmCompiledModule::CallerOwnedBuffer`

So we can consider unsetting V8_IMMINENT_DEPRECATION_WARNINGS for the node GYP target until we fix our code.

addaleax · 2018-10-11T18:21:24Z

As explained in #23167, I don’t think the OpenSSL defines should or need to end up here.

refack · 2018-10-11T18:26:15Z

As explained in #23167, I don’t think the OpenSSL defines should or need to end up here.

That does make sense, but ATM we do define it for the node build:

node/deps/openssl/config/archs/linux-x86_64/asm/openssl.gypi

Line 674 in d4ce144

'OPENSSL_THREADS',

(and every other architecture), so this just synchronizes how we build, and what we export to addons.

/CC @nodejs/crypto should it be undefined for the node build?

richardlau · 2018-10-11T18:27:35Z

If we're going to suddenly enable warnings for native addons, semver-major?

addaleax · 2018-10-11T18:38:03Z

If we're going to suddenly enable warnings for native addons, semver-major?

We can decide to do that, but I’d not consider it semver-major. Deprecation warnings in C++ are kind of different from runtime deprecations in JS, and we’ve been treating these as semver-minor so far.

refack · 2018-10-11T18:39:57Z

If we're going to suddenly enable warnings for native addons, semver-major?

It would be advantageous to have this in node11

thefourtheye · 2018-10-11T18:41:14Z

common.gypi

+      # `OPENSSL_THREADS` is defined via GYP for openSSL for all architectures.
+      'OPENSSL_THREADS'
+      'V8_DEPRECATION_WARNINGS',
+      'V8_IMMINENT_DEPRECATION_WARNINGS',


Just to understand this better, what are the differences between these two?

Example: https://github.com/nodejs/node/pull/23414/files
AFAIU V8_DEPRECATION_WARNINGS makes API points marked with V8_DEPRECATED marked [[deprecated]]
V8_IMMINENT_DEPRECATION_WARNINGS makes thant for API points marked with V8_DEPRECATE_SOON.
Similar to node's policy V8_DEPRECATED are not-recommended for use, and can be removed in the next V8 version. V8_DEPRECATE_SOON can only be demoted to V8_DEPRECATED in a next version.

/CC @hashseed, right?

ATM we do turn on V8_DEPRECATION_WARNINGS for node build, and native addons builds.

So V8_IMMINENT_DEPRECATION_WARNINGS will report DEPRECATE_SOON and V8_DEPRECATION_WARNINGS will report the normal deprecation, right?

richardlau · 2018-10-11T18:53:04Z

If we're going to suddenly enable warnings for native addons, semver-major?

We can decide to do that, but I’d not consider it semver-major. Deprecation warnings in C++ are kind of different from runtime deprecations in JS, and we’ve been treating these as semver-minor so far.

Fair enough. I'm just mindful that the majority of users seeing these will be users installing the addons (probably as dependencies of other modules) who are unlikely to be in a position to do something about them and may be asking why they are suddenly appearing with a new version of Node.js. Maybe at least it should be called out in the release notes?

addaleax · 2018-10-11T21:36:05Z

@jkrems Given nodejs/nan#811 (comment), do you maybe want to weigh in here?

jkrems · 2018-10-12T00:33:33Z

@addaleax Thanks for the ping!

Fair enough. I'm just mindful that the majority of users seeing these will be users installing the addons (probably as dependencies of other modules) who are unlikely to be in a position to do something about them and may be asking why they are suddenly appearing with a new version of Node.js.

Is there any way this could not be enabled for things coming from dependencies? I don't think there's any value for 3rd party dependencies, other than causing a lot of non-actionable noise. The issue is that I've seen a lot of teams we support start writing hacks and workarounds to get rid of these warnings if they originated in code they don't control. Even worse: warnings that cannot be directly addressed drown out other, more critical issues and can send people on wild goose chases.

In other words: Can we make sure that the people who see these new warnings are mostly (or even only) the people who can actually fix them?

refack · 2018-10-12T00:38:35Z

In other words: Can we make sure that the people who see these new warnings are mostly (or even only) the people who can actually fix them?

I think we can but we'll need to patch node-gyp.

refack · 2018-10-13T22:39:40Z

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

This could break addons with -Werror

joyeecheung · 2018-10-14T22:49:24Z

Hmm, I just learned that some addons use -Werror, this would break them so if we are actually turning the flags on.

refack · 2018-10-14T23:12:20Z

Hmm, I just learned that some addons use -Werror, this would break them so if we are actually turning the flags on.

IMHO that's a good thing, it will best way to notify the authors ahead of time or

They could 'defines!': [ 'V8_IMMINENT_DEPRECATION_WARNINGS' ]
Or they will break on next V8 tick, when IMMINENT turns to DEPRECATION

addaleax · 2018-10-14T23:15:16Z

That’s … interesting. @joyeecheung Do you know how much “some addons” is?

If we do enable warnings for those, every deprecation would already be as bad as the breaking change itself, which completely defeats the purpose of deprecations…

refack · 2018-10-15T01:04:13Z

some addons use -Werror

What happens when you combine -Werror -Wno-deprecated-declarations...

I'll go check...

refack · 2018-10-16T13:31:57Z

BTW what about we build Node with -Werror?

Much work to be done for that. (and we'll need to exclude all the /deps/).
BTW: I tried that 4 years ago nodejs/node-v0.x-archive#8485 👴

refack · 2018-10-24T17:39:48Z

/CC @nodejs/release @nodejs/addon-api PTAL

common.gypi

refack · 2018-11-04T22:46:32Z

Sample CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1491/

@nodejs/tsc PTAL:
This is a semver-major change since it turns on compilation time deprecation warnings for V8 APIs annotated with V8_DEPRECATE_SOON for node-core and native-addons built with node-gyp.
In addition, for native-addons, it also turns on compilation warnings for V8 APIs annotated with V8_DEPRECATED.
(Both of these can be turned off by addon authors - Ref: nodejs/nan#811 (comment))

ATM this will produce a significant amount of new warning when compiling node. Last audit gross number is 673, of which 175 are unique.

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

joyeecheung

Let's land it first - at least it will take a while to actually reach the addon ecosystem, in the meantime we can fix our warnings first

* `V8_DEPRECATION_WARNINGS` is here for explicity. It is already defined in `node.gypi`, and `addon.gypi`. * `V8_IMMINENT_DEPRECATION_WARNINGS` added to warn addons authors. * `OPENSSL_THREADS` apears in the openSSL `.h` files, and was only defined via GYP for the node build. PR-URL: nodejs#23426 Fixes: nodejs#23167 Refs: nodejs#23122 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

sam-github · 2019-01-04T21:10:17Z

common.gypi

@@ -503,7 +513,18 @@
        'ldflags': [
          '-Wl,--export-dynamic',
        ],
-      }]
+      }],
+      ['node_shared_openssl!="true"', {


@refack You suggested that to avoid OpenSSL flags being passed to icu (and thus triggering an icu rebuild when switching between shared/non-shared openssl, a change that doesn't effect icu), I could "Add OPENSSL_THREADS to the else clause.", but I'm not sure what you mean by else clause here. Can you elaborate, and I'll make the change and test it?

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 11, 2018

refack requested review from richardlau, joyeecheung and addaleax October 11, 2018 18:17

refack self-assigned this Oct 11, 2018

refack added the addons Issues and PRs related to native addons. label Oct 11, 2018

refack added the crypto Issues and PRs related to the crypto subsystem. label Oct 11, 2018

refack added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 11, 2018

thefourtheye reviewed Oct 11, 2018

View reviewed changes

joyeecheung previously approved these changes Oct 11, 2018

View reviewed changes

thefourtheye approved these changes Oct 11, 2018

View reviewed changes

refack mentioned this pull request Oct 14, 2018

src: update v8::Object::GetPropertyNames() usage #23660

Merged

2 tasks

richardlau reviewed Oct 24, 2018

View reviewed changes

common.gypi Outdated Show resolved Hide resolved

richardlau reviewed Oct 24, 2018

View reviewed changes

common.gypi Outdated Show resolved Hide resolved

refack force-pushed the warn-addons-on-v8-imminent-deprecation branch from a1060a4 to c159847 Compare October 24, 2018 23:46

refack force-pushed the warn-addons-on-v8-imminent-deprecation branch from c159847 to af968e2 Compare November 4, 2018 21:08

joyeecheung approved these changes Nov 4, 2018

View reviewed changes

refack force-pushed the warn-addons-on-v8-imminent-deprecation branch from af968e2 to 765766b Compare November 5, 2018 02:52

refack merged commit 765766b into nodejs:master Nov 5, 2018

refack added the landed label Nov 5, 2018

refack deleted the warn-addons-on-v8-imminent-deprecation branch November 5, 2018 02:54

refack removed the request for review from addaleax November 5, 2018 02:54

refack removed their assignment Nov 6, 2018

refack mentioned this pull request Nov 7, 2018

deps: partially revert 'increase V8 deprecation levels' #24195

Closed

2 tasks

refack mentioned this pull request Nov 21, 2018

APIs removed in V8 7.0 and native addons #23122

Closed

sam-github reviewed Jan 4, 2019

View reviewed changes

BethGriggs mentioned this pull request Mar 26, 2019

v12.0.0 proposal #26930

Merged

murgatroid99 mentioned this pull request May 2, 2019

Defining OPENSSL_THREADS in common.gypi broke my Electron native addon build #27540

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add common `defines` #23426

build: add common `defines` #23426

refack commented Oct 11, 2018

nodejs-github-bot commented Oct 11, 2018

refack commented Oct 11, 2018

refack commented Oct 11, 2018

addaleax commented Oct 11, 2018

refack commented Oct 11, 2018 •

edited

Loading

richardlau commented Oct 11, 2018

addaleax commented Oct 11, 2018

refack commented Oct 11, 2018

thefourtheye Oct 11, 2018

refack Oct 11, 2018

refack Oct 11, 2018

thefourtheye Oct 11, 2018

richardlau commented Oct 11, 2018

addaleax commented Oct 11, 2018

jkrems commented Oct 12, 2018

refack commented Oct 12, 2018

refack commented Oct 13, 2018

joyeecheung commented Oct 14, 2018

refack commented Oct 14, 2018

addaleax commented Oct 14, 2018

refack commented Oct 15, 2018

refack commented Oct 16, 2018

refack commented Oct 24, 2018

refack commented Nov 4, 2018 •

edited

Loading

joyeecheung left a comment

sam-github Jan 4, 2019

sam-github Jan 4, 2019

build: add common defines #23426

build: add common defines #23426

Conversation

refack commented Oct 11, 2018

Checklist

nodejs-github-bot commented Oct 11, 2018

refack commented Oct 11, 2018

refack commented Oct 11, 2018

addaleax commented Oct 11, 2018

refack commented Oct 11, 2018 • edited Loading

richardlau commented Oct 11, 2018

addaleax commented Oct 11, 2018

refack commented Oct 11, 2018

thefourtheye Oct 11, 2018

Choose a reason for hiding this comment

refack Oct 11, 2018

Choose a reason for hiding this comment

refack Oct 11, 2018

Choose a reason for hiding this comment

thefourtheye Oct 11, 2018

Choose a reason for hiding this comment

richardlau commented Oct 11, 2018

addaleax commented Oct 11, 2018

jkrems commented Oct 12, 2018

refack commented Oct 12, 2018

refack commented Oct 13, 2018

joyeecheung commented Oct 14, 2018

refack commented Oct 14, 2018

addaleax commented Oct 14, 2018

refack commented Oct 15, 2018

refack commented Oct 16, 2018

refack commented Oct 24, 2018

refack commented Nov 4, 2018 • edited Loading

joyeecheung left a comment

Choose a reason for hiding this comment

sam-github Jan 4, 2019

Choose a reason for hiding this comment

sam-github Jan 4, 2019

Choose a reason for hiding this comment

build: add common `defines` #23426

build: add common `defines` #23426

refack commented Oct 11, 2018 •

edited

Loading

refack commented Nov 4, 2018 •

edited

Loading