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

spawn_sync: move process.binding('spawn_sync') to internalBinding #22260

Closed
wants to merge 1 commit into from

Conversation

antsmartian
Copy link
Contributor

@antsmartian antsmartian 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-bot nodejs-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

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

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

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

jasnell commented Aug 15, 2018

@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
Contributor Author

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

@antsmartian
Copy link
Contributor Author

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

@antsmartian antsmartian force-pushed the spawn_sync branch 3 times, most recently from b8ab324 to 221afde Compare September 3, 2018 15:38
@antsmartian
Copy link
Contributor Author

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
Contributor Author

Ok seeing a lint issue, let me fix that.

Copy link
Member

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

BridgeAR commented Sep 5, 2018

@BridgeAR BridgeAR 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
Contributor Author

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

@targos
Copy link
Member

targos commented Sep 15, 2018

Landed in 9c9c01f

@targos targos 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
Contributor Author

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

@antsmartian antsmartian deleted the spawn_sync branch October 17, 2018 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs 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_process Issues and PRs related to the child_process subsystem. semver-major PRs 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