test: migrate process.binding to internalBinding for misc. by BeniCheni · Pull Request #24952 · nodejs/node · GitHub

Refs #22160

For the migrated modules from the checklist of #22160, this PR identifies the places of source code & tests with "legacy" process.binding, and follows up to migrate to internalBinding.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

@addaleax,

Thanks for the insight. Reverted back to process.binding in lib/stream.js & deps/.

Thanks for the feedback, @joyeecheung. Updated per suggestions. Please review again at convenience.

Hi reviewer(s),

Following up on this PR w. a rebase, and ran into "make lint" error:

Running JS linter...

/Users/bchen/github/node/test/parallel/test-accessor-properties.js
  58:27  error  Unused eslint-disable directive (no problems were reported from 'node-core/crypto-check')

✖ 1 problem (1 error, 0 warnings)

The source of test/parallel/test-accessor-properties.js is:

if (common.hasCrypto) { // eslint-disable-line node-core/crypto-check

Is there any suggestion how to resolve this make lint error w. eslint-disable?

(The error shows "Unused", which seems to be a removal. But want to confirm w. your help. Thank you in advance!)

@BeniCheni I think you'll need #25395.

Thanks, @cjihrig. I'll look out for the landing of #25395 and rebase again.

Use of process.binding() has largely been replaced by internalBinding(). This commit updates the custom crypto check ESLint rule to check for both process.binding() and internalBinding(). Refs: nodejs#24952 PR-URL: nodejs#25395 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Hi reviewer(s), friendly reminder rebased after #25395, and CI is “green” again to further review. Thanks!

Use of process.binding() has largely been replaced by internalBinding(). This commit updates the custom crypto check ESLint rule to check for both process.binding() and internalBinding(). Refs: #24952 PR-URL: #25395 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Use of process.binding() has largely been replaced by internalBinding(). This commit updates the custom crypto check ESLint rule to check for both process.binding() and internalBinding(). Refs: nodejs#24952 PR-URL: nodejs#25395 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Hello, reviewer(s),

Trying to follow up with this pending PR by rebasing & passing the CI by the commit to bring the PR up-to-date.

Please review at convenience, or let me that if this PR is not needed. I'll be happy to close it as well.

// Flags: --expose_internals

The flag is probably not needed?

Per @BridgeAR’s comment ☝️, removed the unused flag. Thanks.

In places of process.binding, migrate to adapt internalBinding, for miscellaneous, identified source code & tests for various modules. Refs: #22160

Revert the previous incorrect migration of internalBinding from process.binding in lib/stream & inspect_repl. Refs: #22160

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

Looks like there is one failing test now when compiling with ./configure --without-intl?

https://ci.nodejs.org/job/node-test-commit-linux-containered/10923/nodes=ubuntu1604_sharedlibs_withoutintl_x64/testReport/junit/(root)/test/parallel_test_process_binding_internalbinding_whitelist/

The fixup is pretty simple, doing the same we already do for internalBinding('crypto').

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

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

tls_wrap also needs a common.hasCrypto check for the without ssl builds:

https://ci.nodejs.org/job/node-test-commit-linux-containered/11040/nodes=ubuntu1604_sharedlibs_withoutssl_x64/testReport/junit/(root)/test/parallel_test_process_binding_internalbinding_whitelist/

internal/bootstrap/loaders.js:131
      mod = bindingObj[module] = getInternalBinding(module);
                                 ^

Error: No such module: tls_wrap
    at internalBinding (internal/bootstrap/loaders.js:131:34)
    at process.binding (internal/bootstrap/loaders.js:109:14)
    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-process-binding-internalbinding-whitelist.js:32:16)
    at Module._compile (internal/modules/cjs/loader.js:807:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:818:10)
    at Module.load (internal/modules/cjs/loader.js:674:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:606:12)
    at Function.Module._load (internal/modules/cjs/loader.js:598:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:870:12)
    at internal/main/run_main_module.js:21:11

(The without ssl variant build (nodejs/build#1574) was only enabled in the last few days which is why it didn't show up in previous CI runs. This is exactly the kind of thing it's designed to catch.)

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

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

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

Landed in 53d4e04 🎉

Migrate various modules from using process.binding to internalBinding. PR-URL: nodejs#24952 Refs: nodejs#22160 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

I updated the commit message while landing.

Thanks for helping out with the CIs, @addaleax & @BridgeAR. Have learned a lot but observing the CIs errors and logs during landing workflow. 🙏

Migrate various modules from using process.binding to internalBinding. PR-URL: nodejs#24952 Refs: nodejs#22160 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Migrate various modules from using process.binding to internalBinding. PR-URL: #24952 Refs: #22160 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Hi, bit unrelated but does this mean there won't be a way anymore to access process.binding('buffer') entirely, or will there be other ways to access it?

Hi, bit unrelated but does this mean there won't be a way anymore to access process.binding('buffer') entirely, or will there be other ways to access it?

@davalapar Eventually, yes. There shouldn’t be anything on that object that isn’t available through public APIs, and there are no stability guarantess about the contents of process.binding('buffer') anyway.

Use of process.binding() has largely been replaced by internalBinding(). This commit updates the custom crypto check ESLint rule to check for both process.binding() and internalBinding(). Refs: #24952 PR-URL: #25395 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 11, 2018

BeniCheni changed the title src,test: migrate process.binding to internalBinding for misc. lib & test: migrate process.binding to internalBinding for misc. Dec 11, 2018

BeniCheni changed the title lib & test: migrate process.binding to internalBinding for misc. lib,test: migrate process.binding to internalBinding for misc. Dec 13, 2018

addaleax reviewed Dec 14, 2018

View reviewed changes

Comment thread lib/stream.js Outdated

Comment thread deps/node-inspect/lib/internal/inspect_repl.js Outdated

joyeecheung reviewed Dec 15, 2018

View reviewed changes

joyeecheung approved these changes Dec 18, 2018

View reviewed changes

cjihrig mentioned this pull request Jan 8, 2019

tools: lint for use of internalBinding() #25395

Merged

3 tasks

BridgeAR approved these changes Feb 22, 2019

View reviewed changes

Comment thread test/parallel/test-child-process-spawnsync-validation-errors.js Outdated

BridgeAR mentioned this pull request Feb 26, 2019

test: remove usage of process.binding() #26304

Closed

3 tasks

addaleax approved these changes Feb 26, 2019

View reviewed changes

addaleax added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 26, 2019

joyeecheung approved these changes Feb 26, 2019

View reviewed changes

thefourtheye reviewed Feb 26, 2019

View reviewed changes

Comment thread test/parallel/test-process-binding-internalbinding-whitelist.js Outdated

BeniCheni added 4 commits February 27, 2019 07:45

lib: migrate process.binding to internalBinding for misc modules

cca74a4

In places of process.binding, migrate to adapt internalBinding, for miscellaneous, identified source code & tests for various modules. Refs: #22160

src: revert incorrect internalBinding back to process.binding

6b348d3

Revert the previous incorrect migration of internalBinding from process.binding in lib/stream & inspect_repl. Refs: #22160

test: update require os constants' signals child process spawnsync tests

bc891a7

test: revert to process.binding in test-process-binding.js

1fa0d8f

addaleax added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed needs-ci PRs that need a full CI run. labels Feb 28, 2019

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 1, 2019

fixup

d9f1730

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 4, 2019

BridgeAR removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 5, 2019

fixup

feddd9d

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 5, 2019

BridgeAR closed this Mar 6, 2019

BeniCheni deleted the misc-process-to-internal-binding branch March 6, 2019 23:41

BridgeAR mentioned this pull request Mar 13, 2019

2019-03-14, Version 11.12.0 (Current) #26644

Merged

test: migrate process.binding to internalBinding for misc. by BeniCheni · Pull Request #24952 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: migrate process.binding to internalBinding for misc. - #24952

test: migrate process.binding to internalBinding for misc.#24952BeniCheni wants to merge 12 commits intonodejs:masternodejs/node:masterfrom BeniCheni:misc-process-to-internal-bindingCopy head branch name to clipboard

Conversation

BeniCheni commented Dec 11, 2018

Checklist

Uh oh!

Uh oh!

Uh oh!

BeniCheni commented Dec 15, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BeniCheni commented Dec 17, 2018

Uh oh!

BeniCheni commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Jan 8, 2019

Uh oh!

BeniCheni commented Jan 8, 2019

Uh oh!

BeniCheni commented Jan 14, 2019

Uh oh!

BeniCheni commented Feb 22, 2019

Uh oh!

Uh oh!

BeniCheni commented Feb 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

addaleax commented Feb 28, 2019

Uh oh!

addaleax commented Mar 1, 2019

Uh oh!

addaleax commented Mar 4, 2019

Uh oh!

BridgeAR commented Mar 5, 2019

Uh oh!

richardlau commented Mar 5, 2019

Uh oh!

addaleax commented Mar 5, 2019

Uh oh!

BridgeAR commented Mar 5, 2019

Uh oh!

addaleax commented Mar 6, 2019

Uh oh!

BridgeAR commented Mar 6, 2019

Uh oh!

BridgeAR commented Mar 6, 2019

Uh oh!

BeniCheni commented Mar 6, 2019

Uh oh!

davalapar commented Mar 17, 2019

Uh oh!

addaleax commented Mar 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

test: migrate process.binding to internalBinding for misc.#24952
BeniCheni wants to merge 12 commits into
nodejs:masterfrom
BeniCheni:misc-process-to-internal-binding

BeniCheni commented Jan 8, 2019 •
edited
Loading

BeniCheni commented Feb 23, 2019 •
edited
Loading