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

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

Closed

Conversation

Projects
None yet
9 participants
@BeniCheni
Copy link
Contributor

commented Dec 11, 2018

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

@BeniCheni BeniCheni force-pushed the BeniCheni:misc-process-to-internal-binding branch 2 times, most recently from 0f1fd58 to 76f7815 Dec 11, 2018

@BeniCheni 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 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

Show resolved Hide resolved lib/stream.js Outdated
Show resolved Hide resolved deps/node-inspect/lib/internal/inspect_repl.js Outdated

@BeniCheni BeniCheni force-pushed the BeniCheni:misc-process-to-internal-binding branch from 76f7815 to 41e2f02 Dec 15, 2018

@BeniCheni

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2018

@addaleax,

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

Show resolved Hide resolved test/parallel/test-child-process-spawnsync-kill-signal.js Outdated
Show resolved Hide resolved test/parallel/test-child-process-spawnsync-validation-errors.js Outdated
Show resolved Hide resolved test/parallel/test-dgram-bind-fd.js Outdated
Show resolved Hide resolved test/parallel/test-fs-readdir-types.js Outdated
Show resolved Hide resolved test/parallel/test-process-constants-noatime.js Outdated
Show resolved Hide resolved test/parallel/test-process-binding.js Outdated

@BeniCheni BeniCheni force-pushed the BeniCheni:misc-process-to-internal-binding branch from 8bcc0a5 to cc398f9 Dec 17, 2018

@BeniCheni

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

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

@BeniCheni BeniCheni force-pushed the BeniCheni:misc-process-to-internal-binding branch from f2e81f6 to 9e83336 Jan 8, 2019

@BeniCheni

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

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!)

@cjihrig cjihrig referenced this pull request Jan 8, 2019

Merged

tools: lint for use of internalBinding() #25395

3 of 3 tasks complete
@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

@BeniCheni I think you'll need #25395.

@BeniCheni

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

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

cjihrig added a commit to cjihrig/node-1 that referenced this pull request Jan 11, 2019

tools: lint for use of internalBinding()
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>

@BeniCheni BeniCheni force-pushed the BeniCheni:misc-process-to-internal-binding branch from 9e83336 to 025cc6c Jan 14, 2019

@BeniCheni

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

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

addaleax added a commit that referenced this pull request Jan 14, 2019

tools: lint for use of internalBinding()
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>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019

tools: lint for use of internalBinding()
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>

@BeniCheni BeniCheni force-pushed the BeniCheni:misc-process-to-internal-binding branch from 025cc6c to f80a90f Feb 22, 2019

@BeniCheni

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

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.

@BeniCheni BeniCheni force-pushed the BeniCheni:misc-process-to-internal-binding branch from f80a90f to 1cba09f Feb 23, 2019

@BeniCheni

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

// Flags: --expose_internals

The flag is probably not needed?

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

@BridgeAR BridgeAR referenced this pull request Feb 26, 2019

Closed

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

3 of 3 tasks complete

@addaleax addaleax added the needs-ci label Feb 26, 2019

@addaleax

This comment has been minimized.

@addaleax addaleax added author ready and removed needs-ci labels Feb 28, 2019

@addaleax

This comment has been minimized.

@addaleax addaleax removed the author ready label Mar 1, 2019

@addaleax

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

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/

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

@richardlau

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

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.)

@BridgeAR BridgeAR removed the author ready label Mar 5, 2019

@addaleax

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

@addaleax

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

Landed in 53d4e04 🎉

@BridgeAR BridgeAR closed this Mar 6, 2019

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 6, 2019

lib: migrate process.binding to internalBinding
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>
@BridgeAR

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

I updated the commit message while landing.

@BeniCheni BeniCheni deleted the BeniCheni:misc-process-to-internal-binding branch Mar 6, 2019

@BeniCheni

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

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

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 12, 2019

lib: migrate process.binding to internalBinding
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>

BridgeAR added a commit that referenced this pull request Mar 14, 2019

lib: migrate process.binding to internalBinding
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>
@davalapar

This comment has been minimized.

Copy link

commented Mar 17, 2019

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?

@addaleax

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

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.

BethGriggs added a commit that referenced this pull request Apr 28, 2019

tools: lint for use of internalBinding()
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>

BethGriggs added a commit that referenced this pull request May 10, 2019

tools: lint for use of internalBinding()
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>

MylesBorins added a commit that referenced this pull request May 16, 2019

tools: lint for use of internalBinding()
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.