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.

Sign up for GitHub

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

Jump to bottom

[v10.x] bootstrapper: move internalBinding to NativeModule #23661

Closed
wants to merge 2 commits into from
Closed

[v10.x] bootstrapper: move internalBinding to NativeModule #23661

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Backport of #23025

One thing worth noting is that there were a large number of merge conflicts where v10.x used process.binding() and master used internalBinding(). I’ve decided to patch internalBinding() to fall back to process.binding() on v10.x, since we’re not going to backport the conversions.

The advantage of that is that the v10.x-master diff is smaller, and from now on, commits from master can always be backported when they introduce new internalBinding() calls, even for bindings that haven’t been moved yet.

@devsnek @addaleax
bootstrapper: move internalBinding to NativeModule
45ca385 
internalBinding is used so often that it should just automatically be
available for usage in internals.

PR-URL: nodejs#23025
Refs: nodejs@2a9eb31
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1246/pipeline

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v10.x labels Oct 14, 2018
@addaleax
Copy link
Member Author

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

@richardlau
Copy link
Member

from now on, commits from master can always be backported when they introduce new internalBinding() calls, even for bindings that haven’t been moved yet.

FYI we've already run into this with a commit from master being backported to v10.x-staging: #23633

@targos
Copy link
Member

targos commented Oct 17, 2018

Rebuild: https://ci.nodejs.org/job/node-test-pull-request/17948/

@addaleax
Copy link
Member Author

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

@addaleax
Copy link
Member Author

Landed in 5468509

@addaleax addaleax closed this Oct 20, 2018
addaleax pushed a commit that referenced this pull request Oct 20, 2018
@devsnek @addaleax
bootstrapper: move internalBinding to NativeModule
5468509 
internalBinding is used so often that it should just automatically be
available for usage in internals.

PR-URL: #23025
Refs: 2a9eb31
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Backport-PR-URL: #23661
Backport-Reviewed-By: Gus Caplan <me@gus.host>
Backport-Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Backport-Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Backport-Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@addaleax addaleax deleted the backport-23025 branch October 20, 2018 05:45
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
@devsnek @MylesBorins
bootstrapper: move internalBinding to NativeModule
c39b910 
internalBinding is used so often that it should just automatically be
available for usage in internals.

PR-URL: #23025
Refs: 2a9eb31
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Backport-PR-URL: #23661
Backport-Reviewed-By: Gus Caplan <me@gus.host>
Backport-Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Backport-Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Backport-Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@devsnek @rvagg
bootstrapper: move internalBinding to NativeModule
4a0f2f8 
internalBinding is used so often that it should just automatically be
available for usage in internals.

PR-URL: #23025
Refs: 2a9eb31
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Backport-PR-URL: #23661
Backport-Reviewed-By: Gus Caplan <me@gus.host>
Backport-Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Backport-Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Backport-Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@devsnek @MylesBorins
bootstrapper: move internalBinding to NativeModule
e872089 
internalBinding is used so often that it should just automatically be
available for usage in internals.

PR-URL: #23025
Refs: 2a9eb31
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Backport-PR-URL: #23661
Backport-Reviewed-By: Gus Caplan <me@gus.host>
Backport-Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Backport-Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Backport-Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@devsnek devsnek devsnek approved these changes

@richardlau richardlau richardlau approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@targos targos targos approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@addaleax @nodejs-github-bot @richardlau @targos @joyeecheung @devsnek @BridgeAR