tools: add check for using process.binding crypto #17867

danbev · 2017-12-26T10:09:30Z

Currently, when configuring --without-ssl any tests that use
process.binding('crypto') will not report a lint warning. This is
because the eslint check only generates a warning when using require.

This commit adds a check for using binding in addition to require.

Checklist

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

Affected core subsystem(s)

tools

Currently, when configuring --without-ssl any tests that use process.binding('crypto') will not report a lint warning. This is because the eslint check only generates a warning when using require. This commit adds a check for using binding in addition to require.

Currently, when configured --without-ssl this test fails with the following error: === release test-accessor-properties === Path: parallel/test-accessor-properties node/test/parallel/test-accessor-properties.js:16 const crypto = process.binding('crypto'); ^ Error: No such module: crypto at Object.<anonymous> (test-accessor-properties.js:16:24) at Module._compile (module.js:660:30) at Object.Module._extensions..js (module.js:671:10) at Module.load (module.js:577:32) at tryModuleLoad (module.js:517:12) at Function.Module._load (module.js:509:3) at Function.Module.runMain (module.js:701:10) at startup (bootstrap_node.js:194:16) at bootstrap_node.js:645:3 This commit adds a hasCrypto check.

Currently, when configured --without-ssl test-repl-tab-complete fails with the following error: assert.js:43 throw new errors.AssertionError(obj); ^ AssertionError [ERR_ASSERTION]: [ [], 'lexicalL' ] deepStrictEqual [] at testRepl.complete.common.mustCall (node/test/parallel/test-repl-tab-complete.js:549:14) at /node/test/common/index.js:530:15 at completionGroupsLoaded (repl.js:1204:5) at REPLServer.complete (repl.js:1090:11) at REPLServer.completer (repl.js:450:14) at REPLServer.complete (repl.js:919:18) at __dirname.forEach (parallel/test-repl-tab-complete.js:548:14) at Array.forEach (<anonymous>) at Object.<anonymous> (parallel/test-repl-tab-complete.js:545:29) at Module._compile (module.js:660:30) This commit attempts to fix this test but I'm not sure if this is a proper fix as I'm not familiar with the repl code base yet.

danbev · 2017-12-26T10:11:36Z

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

danbev · 2017-12-26T14:15:19Z

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

Trott · 2017-12-26T15:02:38Z

test/parallel/test-accessor-properties.js


+if (!common.hasCrypto)
+  common.skip('missing crypto');


Rather than skipping this entire test, can we move the crypto stuff to its if block and only run those tests if hasCrypto is true? This way, we aren't skipping the non-crypto tests in the file.

Sounds good, I'll do that.

Trott · 2017-12-26T15:06:14Z

Does our http2 implementation require OpenSSL? If we compile without OpenSSL, does process.binding('http2') fail or merely return different results than with OpenSSL? @nodejs/http2

Trott · 2017-12-26T15:07:05Z

test/parallel/test-repl-tab-complete.js

@@ -544,7 +544,8 @@ editor.completer('var log = console.l', common.mustCall((error, data) => {

  ['Let', 'Const', 'Klass'].forEach((type) => {
    const query = `lexical${type[0]}`;
-    const expected = hasInspector ? [[`lexical${type}`], query] : [];
+    const expected = hasInspector ? [[`lexical${type}`], query] :
+      [[], `lexical${type[0]}`];


Unrelated change?

Actually it is related. Without this I get the error mentioned in the commit:

assert.js:43 throw new errors.AssertionError(obj); ^ AssertionError [ERR_ASSERTION]: [ [], 'lexicalL' ] deepStrictEqual [] at testRepl.complete.common.mustCall (node/test/parallel/test-repl-tab-complete.js:549:14) at /node/test/common/index.js:530:15 at completionGroupsLoaded (repl.js:1204:5) at REPLServer.complete (repl.js:1090:11) at REPLServer.completer (repl.js:450:14) at REPLServer.complete (repl.js:919:18) at __dirname.forEach (parallel/test-repl-tab-complete.js:548:14) at Array.forEach (<anonymous>) at Object.<anonymous> (parallel/test-repl-tab-complete.js:545:29) at Module._compile (module.js:660:30)

jasnell · 2017-12-26T16:58:19Z

process.binding('http2') is still present without openssl but require('http2') is not

Trott · 2017-12-26T17:52:08Z

process.binding('http2') is still present without openssl but require('http2') is not

If I'm understanding correctly, then perhaps in some or even all cases, instead of skipping tests that include process.binding('http2') when the node binary is compiled without OpenSSL, perhaps the expected results of the test need to be modified based on the value of common.hasCrypto.

jasnell · 2017-12-26T18:51:28Z

It's better to just skip all http2 tests when openssl is not present

danbev · 2017-12-27T05:48:02Z

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

Trott · 2017-12-27T21:18:35Z

It's better to just skip all http2 tests when openssl is not present

@jasnell Is that because there is no expected behavior without OpenSSL other than "absolutely nothing whatsoever in http2 works at all"? Or is there a different reason?

jasnell · 2017-12-27T23:44:18Z

It's because I'm likely going to change it so that process.binding('http2') doesn't exist if openssl isn't enabled.

BridgeAR

No strong opinion, so LGTM

danbev · 2017-12-29T05:01:37Z

test/node-test-commit failure looks unrelated

console output:

g++: internal compiler error: Killed (program cc1plus)
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-5/README.Bugs> for instructions.
deps/v8/src/v8_base.target.mk:608: recipe for target '/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64_odroid_c2/out/Release/obj.target/v8_base/deps/v8/src/objects.o' failed
make[2]: *** [/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64_odroid_c2/out/Release/obj.target/v8_base/deps/v8/src/objects.o] Error 4
make[2]: *** Waiting for unfinished jobs....
rm fa4b5595818ee3c6b8e353e33594a7e9a94ec498.intermediate
Makefile:87: recipe for target 'node' failed
make[1]: *** [node] Error 2
Makefile:611: recipe for target 'build-ci' failed
make: *** [build-ci] Error 2
Build step 'Conditional steps (multiple)' marked build as failure
Run condition [Always] enabling perform for step [[]]
TAP Reports Processing: START
Looking for TAP results report in workspace using pattern: *.tap
Did not find any matching files. Setting build result to FAILURE.
Checking ^not ok
Jenkins Text Finder: File set '*.tap' is empty
Notifying upstream projects of job completion
Finished: FAILURE

Currently, when configuring --without-ssl any tests that use process.binding('crypto') will not report a lint warning. This is because the eslint check only generates a warning when using require. This commit adds a check for using binding in addition to require. PR-URL: #17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Currently, when configured --without-ssl tests that use process.binding('crypto') fail with the following error: === release test-accessor-properties === Path: parallel/test-accessor-properties node/test/parallel/test-accessor-properties.js:16 const crypto = process.binding('crypto'); ^ Error: No such module: crypto at Object.<anonymous> (test-accessor-properties.js:16:24) at Module._compile (module.js:660:30) at Object.Module._extensions..js (module.js:671:10) at Module.load (module.js:577:32) at tryModuleLoad (module.js:517:12) at Function.Module._load (module.js:509:3) at Function.Module.runMain (module.js:701:10) at startup (bootstrap_node.js:194:16) at bootstrap_node.js:645:3 This commit adds a hasCrypto check. PR-URL: #17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Currently, when configured --without-ssl test-repl-tab-complete fails with the following error: assert.js:43 throw new errors.AssertionError(obj); ^ AssertionError [ERR_ASSERTION]: [ [], 'lexicalL' ] deepStrictEqual [] at testRepl.complete.common.mustCall (node/test/parallel/test-repl-tab-complete.js:549:14) at /node/test/common/index.js:530:15 at completionGroupsLoaded (repl.js:1204:5) at REPLServer.complete (repl.js:1090:11) at REPLServer.completer (repl.js:450:14) at REPLServer.complete (repl.js:919:18) at __dirname.forEach (parallel/test-repl-tab-complete.js:548:14) at Array.forEach (<anonymous>) at Object.<anonymous> (parallel/test-repl-tab-complete.js:545:29) at Module._compile (module.js:660:30) This commit attempts to fix this test but I'm not sure if this is a proper fix as I'm not familiar with the repl code base yet. PR-URL: #17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

danbev · 2017-12-29T05:35:00Z

Landed in 4117e22, 6fc9c33, and acbe007.

Currently, when configuring --without-ssl any tests that use process.binding('crypto') will not report a lint warning. This is because the eslint check only generates a warning when using require. This commit adds a check for using binding in addition to require. PR-URL: #17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Currently, when configured --without-ssl tests that use process.binding('crypto') fail with the following error: === release test-accessor-properties === Path: parallel/test-accessor-properties node/test/parallel/test-accessor-properties.js:16 const crypto = process.binding('crypto'); ^ Error: No such module: crypto at Object.<anonymous> (test-accessor-properties.js:16:24) at Module._compile (module.js:660:30) at Object.Module._extensions..js (module.js:671:10) at Module.load (module.js:577:32) at tryModuleLoad (module.js:517:12) at Function.Module._load (module.js:509:3) at Function.Module.runMain (module.js:701:10) at startup (bootstrap_node.js:194:16) at bootstrap_node.js:645:3 This commit adds a hasCrypto check. PR-URL: #17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Currently, when configured --without-ssl test-repl-tab-complete fails with the following error: assert.js:43 throw new errors.AssertionError(obj); ^ AssertionError [ERR_ASSERTION]: [ [], 'lexicalL' ] deepStrictEqual [] at testRepl.complete.common.mustCall (node/test/parallel/test-repl-tab-complete.js:549:14) at /node/test/common/index.js:530:15 at completionGroupsLoaded (repl.js:1204:5) at REPLServer.complete (repl.js:1090:11) at REPLServer.completer (repl.js:450:14) at REPLServer.complete (repl.js:919:18) at __dirname.forEach (parallel/test-repl-tab-complete.js:548:14) at Array.forEach (<anonymous>) at Object.<anonymous> (parallel/test-repl-tab-complete.js:545:29) at Module._compile (module.js:660:30) This commit attempts to fix this test but I'm not sure if this is a proper fix as I'm not familiar with the repl code base yet. PR-URL: #17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Currently, when configuring --without-ssl any tests that use process.binding('crypto') will not report a lint warning. This is because the eslint check only generates a warning when using require. This commit adds a check for using binding in addition to require. PR-URL: #17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Currently, when configured --without-ssl tests that use process.binding('crypto') fail with the following error: === release test-accessor-properties === Path: parallel/test-accessor-properties node/test/parallel/test-accessor-properties.js:16 const crypto = process.binding('crypto'); ^ Error: No such module: crypto at Object.<anonymous> (test-accessor-properties.js:16:24) at Module._compile (module.js:660:30) at Object.Module._extensions..js (module.js:671:10) at Module.load (module.js:577:32) at tryModuleLoad (module.js:517:12) at Function.Module._load (module.js:509:3) at Function.Module.runMain (module.js:701:10) at startup (bootstrap_node.js:194:16) at bootstrap_node.js:645:3 This commit adds a hasCrypto check. PR-URL: #17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Currently, when configured --without-ssl test-repl-tab-complete fails with the following error: assert.js:43 throw new errors.AssertionError(obj); ^ AssertionError [ERR_ASSERTION]: [ [], 'lexicalL' ] deepStrictEqual [] at testRepl.complete.common.mustCall (node/test/parallel/test-repl-tab-complete.js:549:14) at /node/test/common/index.js:530:15 at completionGroupsLoaded (repl.js:1204:5) at REPLServer.complete (repl.js:1090:11) at REPLServer.completer (repl.js:450:14) at REPLServer.complete (repl.js:919:18) at __dirname.forEach (parallel/test-repl-tab-complete.js:548:14) at Array.forEach (<anonymous>) at Object.<anonymous> (parallel/test-repl-tab-complete.js:545:29) at Module._compile (module.js:660:30) This commit attempts to fix this test but I'm not sure if this is a proper fix as I'm not familiar with the repl code base yet. PR-URL: #17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

MylesBorins · 2018-01-09T09:13:12Z

This will need to be manually backported as it is breaking v9.x-staging

Currently, when configuring --without-ssl any tests that use process.binding('crypto') will not report a lint warning. This is because the eslint check only generates a warning when using require. This commit adds a check for using binding in addition to require. PR-URL: #17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Currently, when configured --without-ssl tests that use process.binding('crypto') fail with the following error: === release test-accessor-properties === Path: parallel/test-accessor-properties node/test/parallel/test-accessor-properties.js:16 const crypto = process.binding('crypto'); ^ Error: No such module: crypto at Object.<anonymous> (test-accessor-properties.js:16:24) at Module._compile (module.js:660:30) at Object.Module._extensions..js (module.js:671:10) at Module.load (module.js:577:32) at tryModuleLoad (module.js:517:12) at Function.Module._load (module.js:509:3) at Function.Module.runMain (module.js:701:10) at startup (bootstrap_node.js:194:16) at bootstrap_node.js:645:3 This commit adds a hasCrypto check. PR-URL: #17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

gibfahn · 2018-01-24T02:52:46Z

@MylesBorins did you end up getting this to work in 9.x?

MylesBorins · 2018-02-27T04:42:25Z

@gibfahn it would seem so!

@danbev should this be backported to v6.x or v8.x?

Currently, when configuring --without-ssl any tests that use process.binding('crypto') will not report a lint warning. This is because the eslint check only generates a warning when using require. This commit adds a check for using binding in addition to require. PR-URL: nodejs#17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Currently, when configured --without-ssl test-repl-tab-complete fails with the following error: assert.js:43 throw new errors.AssertionError(obj); ^ AssertionError [ERR_ASSERTION]: [ [], 'lexicalL' ] deepStrictEqual [] at testRepl.complete.common.mustCall (node/test/parallel/test-repl-tab-complete.js:549:14) at /node/test/common/index.js:530:15 at completionGroupsLoaded (repl.js:1204:5) at REPLServer.complete (repl.js:1090:11) at REPLServer.completer (repl.js:450:14) at REPLServer.complete (repl.js:919:18) at __dirname.forEach (parallel/test-repl-tab-complete.js:548:14) at Array.forEach (<anonymous>) at Object.<anonymous> (parallel/test-repl-tab-complete.js:545:29) at Module._compile (module.js:660:30) This commit attempts to fix this test but I'm not sure if this is a proper fix as I'm not familiar with the repl code base yet. PR-URL: nodejs#17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Currently, when configuring --without-ssl any tests that use process.binding('crypto') will not report a lint warning. This is because the eslint check only generates a warning when using require. This commit adds a check for using binding in addition to require. PR-URL: #17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Currently, when configured --without-ssl tests that use process.binding('crypto') fail with the following error: === release test-accessor-properties === Path: parallel/test-accessor-properties node/test/parallel/test-accessor-properties.js:16 const crypto = process.binding('crypto'); ^ Error: No such module: crypto at Object.<anonymous> (test-accessor-properties.js:16:24) at Module._compile (module.js:660:30) at Object.Module._extensions..js (module.js:671:10) at Module.load (module.js:577:32) at tryModuleLoad (module.js:517:12) at Function.Module._load (module.js:509:3) at Function.Module.runMain (module.js:701:10) at startup (bootstrap_node.js:194:16) at bootstrap_node.js:645:3 This commit adds a hasCrypto check. PR-URL: nodejs#17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Currently, when configured --without-ssl tests that use process.binding('crypto') fail with the following error: === release test-accessor-properties === Path: parallel/test-accessor-properties node/test/parallel/test-accessor-properties.js:16 const crypto = process.binding('crypto'); ^ Error: No such module: crypto at Object.<anonymous> (test-accessor-properties.js:16:24) at Module._compile (module.js:660:30) at Object.Module._extensions..js (module.js:671:10) at Module.load (module.js:577:32) at tryModuleLoad (module.js:517:12) at Function.Module._load (module.js:509:3) at Function.Module.runMain (module.js:701:10) at startup (bootstrap_node.js:194:16) at bootstrap_node.js:645:3 This commit adds a hasCrypto check. Backport-PR-URL: #20456 PR-URL: #17867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

danbev added 3 commits December 26, 2017 11:08

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Dec 26, 2017

squash: add hasCrypto check

4d1c122

Trott reviewed Dec 26, 2017

View reviewed changes

squash: guard crypto asserts with check

9faf7dd

jasnell approved these changes Dec 27, 2017

View reviewed changes

BridgeAR approved these changes Dec 28, 2017

View reviewed changes

danbev closed this Dec 29, 2017

danbev deleted the eslint-crypto-binding-check branch December 29, 2017 05:35

MylesBorins added the backport-requested-v9.x label Jan 9, 2018

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins removed the backport-requested-v9.x label Feb 27, 2018

danbev mentioned this pull request Feb 27, 2018

[v8.x backport] tools: add check for using process.binding crypto #19029

Closed

2 tasks

danbev added the dont-land-on-v6.x label Feb 27, 2018

MylesBorins mentioned this pull request Mar 6, 2018

v9.8.0 proposal #19181

Merged

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

MylesBorins added the backported-to-v8.x label Jul 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: add check for using process.binding crypto #17867

tools: add check for using process.binding crypto #17867

danbev commented Dec 26, 2017

danbev commented Dec 26, 2017

danbev commented Dec 26, 2017

Trott Dec 26, 2017

danbev Dec 27, 2017

Trott commented Dec 26, 2017

Trott Dec 26, 2017

danbev Dec 27, 2017

jasnell commented Dec 26, 2017

Trott commented Dec 26, 2017

jasnell commented Dec 26, 2017

danbev commented Dec 27, 2017

Trott commented Dec 27, 2017

jasnell commented Dec 27, 2017

BridgeAR left a comment

danbev commented Dec 29, 2017

danbev commented Dec 29, 2017

MylesBorins commented Jan 9, 2018

gibfahn commented Jan 24, 2018

MylesBorins commented Feb 27, 2018

tools: add check for using process.binding crypto #17867

tools: add check for using process.binding crypto #17867

Conversation

danbev commented Dec 26, 2017

Checklist

Affected core subsystem(s)

danbev commented Dec 26, 2017

danbev commented Dec 26, 2017

Trott Dec 26, 2017

Choose a reason for hiding this comment

danbev Dec 27, 2017

Choose a reason for hiding this comment

Trott commented Dec 26, 2017

Trott Dec 26, 2017

Choose a reason for hiding this comment

danbev Dec 27, 2017

Choose a reason for hiding this comment

jasnell commented Dec 26, 2017

Trott commented Dec 26, 2017

jasnell commented Dec 26, 2017

danbev commented Dec 27, 2017

Trott commented Dec 27, 2017

jasnell commented Dec 27, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

danbev commented Dec 29, 2017

danbev commented Dec 29, 2017

MylesBorins commented Jan 9, 2018

gibfahn commented Jan 24, 2018

MylesBorins commented Feb 27, 2018