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

[11.x] test-require-deps-deprecation fails on --without-ssl builds #26480

Closed
richardlau opened this issue Mar 6, 2019 · 6 comments
Closed

[11.x] test-require-deps-deprecation fails on --without-ssl builds #26480

richardlau opened this issue Mar 6, 2019 · 6 comments

Comments

@richardlau
Copy link
Member

It seems a deprecation warning is emitted twice for such builds while requireing 'node-inspect/lib/internal/inspect_client'. https://ci.nodejs.org/job/node-test-commit-linux-containered/11111/nodes=ubuntu1604_sharedlibs_withoutssl_x64/console

I would like to ignore that test for now as it does not seem critical to block the release. Any other opinions? @nodejs/releasers @nodejs/tsc

We have only just enabled the --without-ssl builds in the last week: nodejs/build#1574

The test in question doesn't exist in master (removed in #25138 which is semver-major). I kicked off https://ci.nodejs.org/job/node-test-commit-linux-containered/11117/ against v11.x (i.e. 11.10.1) and the test failed there so it's not a new regression in this proposed release so I'm okay with ignoring this for now but we do need to address it so the builds pass for the next 11.x release.

Originally posted by @richardlau in #26322 (comment)

@richardlau richardlau changed the title [11.x] test-require-deps-deprecation fails --with-ssl [11.x] test-require-deps-deprecation fails on --without-ssl builds Mar 6, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2019

I was not able to reproduce the issue locally. I suggest we just remove the tests in v11. Any objections?

@richardlau
Copy link
Member Author

It does reproduce for me.

@danbev
Copy link
Contributor

danbev commented Mar 7, 2019

Regarding the --without-ssl issue, perhaps we can add a crypto check:

patch
$ git diff
diff --git a/test/parallel/test-require-deps-deprecation.js b/test/parallel/test-require-deps-deprecation.js
index 5bee24a5db..aba84b5af7 100644
--- a/test/parallel/test-require-deps-deprecation.js
+++ b/test/parallel/test-require-deps-deprecation.js
@@ -4,9 +4,6 @@ const common = require('../common');
 const assert = require('assert');

 const deprecatedModules = [
-  'node-inspect/lib/_inspect',
-  'node-inspect/lib/internal/inspect_client',
-  'node-inspect/lib/internal/inspect_repl',
   'v8/tools/SourceMap',
   'v8/tools/codemap',
   'v8/tools/consarray',
@@ -19,6 +16,12 @@ const deprecatedModules = [
   'v8/tools/tickprocessor-driver'
 ];

+if (common.hasCrypto) {
+  deprecatedModules.push('node-inspect/lib/_inspect',
+                         'node-inspect/lib/internal/inspect_client',
+                         'node-inspect/lib/internal/inspect_repl');
+}
+
 // Newly added deps that do not have a deprecation wrapper around it would
 // throw an error, but no warning would be emitted.
 const deps = [

@richardlau
Copy link
Member Author

For posterity, git bisect points to b280d90 being the first bad commit.

@richardlau
Copy link
Member Author

Prior to b280d90:

-bash-4.2$ ./node
> require('node-inspect/lib/_inspect')
Thrown:
Error [ERR_NO_CRYPTO]: Node.js is not compiled with OpenSSL crypto support
    at assertCrypto (internal/util.js:101:11)
    at crypto.js:31:1
    at NativeModule.compile (internal/bootstrap/loaders.js:341:5)
    at Function.NativeModule.require (internal/bootstrap/loaders.js:213:16)
    at NativeModule.requireForDeps (internal/bootstrap/loaders.js:228:23)
    at internal/deps/node-inspect/lib/internal/inspect_client.js:24:16
    at NativeModule.compile (internal/bootstrap/loaders.js:341:5)
    at Function.NativeModule.require (internal/bootstrap/loaders.js:213:16)
    at NativeModule.requireForDeps (internal/bootstrap/loaders.js:228:23)
> (node:3877) [DEP0084] DeprecationWarning: Requiring Node.js-bundled 'node-inspect/lib/_inspect' module is deprecated. Please install the necessary module locally.

> .exit

After we get an extra second deprecation warning which then causes the test to fail because the test is expecting a 1:1 mapping between the list of modules in it and warnings:

-bash-4.2$ ./node
> require('node-inspect/lib/_inspect')
Thrown:
Error [ERR_NO_CRYPTO]: Node.js is not compiled with OpenSSL crypto support
    at assertCrypto (internal/util.js:101:11)
    at crypto.js:31:1
    at NativeModule.compile (internal/bootstrap/loaders.js:317:5)
    at Function.NativeModule.require (internal/bootstrap/loaders.js:220:7)
    at NativeModule.requireWithFallbackInDeps (internal/bootstrap/loaders.js:239:23)
    at internal/deps/node-inspect/lib/internal/inspect_client.js:24:16
    at NativeModule.compile (internal/bootstrap/loaders.js:317:5)
    at NativeModule.require (internal/bootstrap/loaders.js:220:7)
> (node:5615) [DEP0084] DeprecationWarning: Requiring Node.js-bundled 'node-inspect/lib/_inspect' module is deprecated. Please install the necessary module locally.
(node:5615) [DEP0084] DeprecationWarning: Requiring Node.js-bundled 'node-inspect/lib/internal/inspect_client' module is deprecated. Please install the necessary module locally.
.exit
-bash-4.2$

const deprecatedModules = [
'node-inspect/lib/_inspect',
'node-inspect/lib/internal/inspect_client',
'node-inspect/lib/internal/inspect_repl',
'v8/tools/SourceMap',
'v8/tools/codemap',
'v8/tools/consarray',
'v8/tools/csvparser',
'v8/tools/logreader',
'v8/tools/profile',
'v8/tools/profile_view',
'v8/tools/splaytree',
'v8/tools/tickprocessor',
'v8/tools/tickprocessor-driver'
];

common.expectWarning('DeprecationWarning', deprecatedModules.map((m) => {
return [`Requiring Node.js-bundled '${m}' module is deprecated. ` +
'Please install the necessary module locally.', 'DEP0084'];
}));

Of the modules in the list only node-inspect/lib/_inspect appears to be affected. The test passes if that module is removed from the list.

diff --git a/test/parallel/test-require-deps-deprecation.js b/test/parallel/
index 5bee24a..e2c8e9c 100644
--- a/test/parallel/test-require-deps-deprecation.js
+++ b/test/parallel/test-require-deps-deprecation.js
@@ -4,7 +4,6 @@ const common = require('../common');
 const assert = require('assert');

 const deprecatedModules = [
-  'node-inspect/lib/_inspect',
   'node-inspect/lib/internal/inspect_client',
   'node-inspect/lib/internal/inspect_repl',
   'v8/tools/SourceMap',

BridgeAR added a commit that referenced this issue Mar 14, 2019
If this file is loaded with Node.js build without ssl, it will load
'node-inspect/lib/internal/inspect_client' as well. Due to that two
deprecation warnings will be emitted instead of one and the test
fails.

It should be safe to just test all other cases and to ignore this
specific file.

PR-URL: #26619
Fixes: #26480
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this issue Mar 14, 2019
If this file is loaded with Node.js build without ssl, it will load
'node-inspect/lib/internal/inspect_client' as well. Due to that two
deprecation warnings will be emitted instead of one and the test
fails.

It should be safe to just test all other cases and to ignore this
specific file.

PR-URL: nodejs#26619
Fixes: nodejs#26480
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BridgeAR added a commit that referenced this issue Mar 14, 2019
If this file is loaded with Node.js build without ssl, it will load
'node-inspect/lib/internal/inspect_client' as well. Due to that two
deprecation warnings will be emitted instead of one and the test
fails.

It should be safe to just test all other cases and to ignore this
specific file.

PR-URL: #26619
Fixes: #26480
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BethGriggs pushed a commit that referenced this issue Apr 16, 2019
If this file is loaded with Node.js build without ssl, it will load
'node-inspect/lib/internal/inspect_client' as well. Due to that two
deprecation warnings will be emitted instead of one and the test
fails.

It should be safe to just test all other cases and to ignore this
specific file.

PR-URL: #26619
Fixes: #26480
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
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

No branches or pull requests

3 participants