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

+constcommon=require('../common');

+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

 constquery=`lexical${type[0]}`;
-constexpected=hasInspector ? [[`lexical${type}`],query] : [];
+constexpected=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' failedmake[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 4make[2]: *** Waiting for unfinished jobs....rm fa4b5595818ee3c6b8e353e33594a7e9a94ec498.intermediateMakefile:87: recipe for target 'node' failedmake[1]: *** [node] Error 2Makefile:611: recipe for target 'build-ci' failedmake: *** [build-ci] Error 2Build step 'Conditional steps (multiple)' marked build as failureRun condition [Always] enabling perform for step [[]]TAP Reports Processing: STARTLooking for TAP results report in workspace using pattern: *.tapDid not find any matching files. Setting build result to FAILURE.Checking ^not okJenkins Text Finder: File set '*.tap' is emptyNotifying upstream projects of job completionFinished: 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

Uh oh!

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

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

Uh oh!

Conversation

danbev commented Dec 26, 2017

Checklist

Affected core subsystem(s)

Uh oh!

danbev commented Dec 26, 2017

Uh oh!

danbev commented Dec 26, 2017

Uh oh!

TrottDec 26, 2017

Choose a reason for hiding this comment

Uh oh!

danbevDec 27, 2017

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 26, 2017

Uh oh!

TrottDec 26, 2017

Choose a reason for hiding this comment

Uh oh!

danbevDec 27, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 26, 2017

Uh oh!

Trott commented Dec 26, 2017

Uh oh!

jasnell commented Dec 26, 2017

Uh oh!

danbev commented Dec 27, 2017

Uh oh!

Trott commented Dec 27, 2017

Uh oh!

jasnell commented Dec 27, 2017

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

danbev commented Dec 29, 2017

Uh oh!

danbev commented Dec 29, 2017

Uh oh!

MylesBorins commented Jan 9, 2018

Uh oh!

gibfahn commented Jan 24, 2018

Uh oh!

MylesBorins commented Feb 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants