Skip to content

Conversation

@antsmartian
Copy link
Contributor

@antsmartianantsmartian commented Aug 11, 2018

Migration from process.binding to internalBinding (see : #22160)

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

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. labels Aug 11, 2018
Copy link
Member

@jasnelljasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with good CITGM run

@jasnelljasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 11, 2018
Copy link
Member

@jdaltonjdalton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding removals until migration is ironed out

@jasnell
Copy link
Member

@antsmartian ... when you get a moment, can you rebase this on master then apply the following patch which will be necessary for this to proceed:

Author: James M Snell <[email protected]> Date: Tue Aug 14 19:36:25 2018 -0700 [Squash] add `process.binding('spawn_sync')` to fallthrough whitelist diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 08daeb1915..ffae6e4cd9 100644 --- a/lib/internal/bootstrap/node.js+++ b/lib/internal/bootstrap/node.js@@ -327,7 +327,7 @@ // that are whitelisted for access via process.binding()... this is used // to provide a transition path for modules that are being moved over to // internalBinding. - const internalBindingWhitelist = new SafeSet(['uv']);+ const internalBindingWhitelist = new SafeSet(['uv', 'spawn_sync']); process.binding = function binding(name){return internalBindingWhitelist.has(name) ? internalBinding(name) : diff --git a/test/parallel/test-process-binding-internalbinding-whitelist.js b/test/parallel/test-process-binding-internalbinding-whitelist.js index ece967a0b7..cc0119917d 100644 --- a/test/parallel/test-process-binding-internalbinding-whitelist.js+++ b/test/parallel/test-process-binding-internalbinding-whitelist.js@@ -7,3 +7,4 @@ const assert = require('assert'); // Assert that whitelisted internalBinding modules are accessible via // process.binding(). assert(process.binding('uv')); +assert(process.binding('spawn_sync'));

@antsmartian
Copy link
ContributorAuthor

@jasnell Yes I'm working on that. I was following up the PR : #22269, so knew its coming ! :)

@antsmartian
Copy link
ContributorAuthor

@jasnell Now it should be good to go, thanks!

@antsmartianantsmartianforce-pushed the spawn_sync branch 3 times, most recently from b8ab324 to 221afdeCompareSeptember 3, 2018 15:38
@antsmartian
Copy link
ContributorAuthor

antsmartian commented Sep 3, 2018

@jasnell Have rebased, can we get this merge, before I get an another conflict :) Thanks for your help.

@antsmartian
Copy link
ContributorAuthor

Ok seeing a lint issue, let me fix that.

Copy link
Member

@lundibundilundibundi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after lint fix.

@BridgeAR
Copy link
Member

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 5, 2018
@antsmartian
Copy link
ContributorAuthor

Yet again conflicts, let me rebase it tonight.. sorry folks..

@targos
Copy link
Member

Landed in 9c9c01f

@targostargos closed this Sep 15, 2018
targos pushed a commit that referenced this pull request Sep 15, 2018
PR-URL: #22260 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@antsmartian
Copy link
ContributorAuthor

@targos Thanks for rebasing, didn't find time to do it. Thanks a ton.

@antsmartianantsmartian deleted the spawn_sync branch October 17, 2018 17:18
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.child_processIssues and PRs related to the child_process subsystem.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@antsmartian@jasnell@BridgeAR@targos@jdalton@cjihrig@lundibundi@trivikr@nodejs-github-bot