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

streams: move process.binding('stream_wrap') to internalBinding #22345

Closed
wants to merge 1 commit into from
Closed

streams: move process.binding('stream_wrap') to internalBinding #22345

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 16, 2018

Refs: #22160

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

@jasnell jasnell added stream Issues and PRs related to the stream subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 16, 2018
@nodejs-github-bot
Copy link
Collaborator

@jasnell sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 16, 2018
@jasnell jasnell requested a review from a team August 16, 2018 00:16
mcollina
mcollina approved these changes Aug 16, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@danbev
Copy link
Contributor

danbev commented Aug 16, 2018

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

@danbev
Copy link
Contributor

danbev commented Aug 16, 2018

I'm seeing the following failure when running the test locally:

$ out/Release/node /Users/danielbevenius/work/nodejs/node/test/pseudo-tty/test-tty-wrap.js
internal/modules/cjs/loader.js:583
    throw err;
    ^

Error: Cannot find module 'internal/test/binding'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:581:15)
    at Function.Module._load (internal/modules/cjs/loader.js:507:25)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (/Users/danielbevenius/work/nodejs/node/test/pseudo-tty/test-tty-wrap.js:4:29)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)

maclover7
maclover7 approved these changes Aug 16, 2018
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

One small change, otherwise LGTM

test/pseudo-tty/test-tty-wrap.js
@@ -1,8 +1,9 @@
'use strict';
require('../common');

const TTY = process.binding('tty_wrap').TTY;
const WriteWrap = process.binding('stream_wrap').WriteWrap;
const { internalBinding } = require('internal/test/binding');

This comment was marked as resolved.

Sign in to view

@jasnell jasnell added lib / src Issues and PRs related to general changes in the lib or src directory. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 16, 2018
BridgeAR
BridgeAR approved these changes Aug 17, 2018
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM but I would rather have less churn in the benchmarks.

benchmark/net/tcp-raw-c2s.js
break;
default:
throw new Error(`invalid type: ${type}`);
function fail(err, syscall) {
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK not all function have to be moved inside main, e.g., fail should be fine as is.

It should also be less churn if the arguments are just passed to the functions instead of moving the functions.

@joyeecheung joyeecheung mentioned this pull request Aug 17, 2018
Closed
@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018

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

@jasnell jasnell force-pushed the stream-wrap-internal-binding branch from 86f9cf5 to f7ead8a Compare August 18, 2018 13:35
@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018 •
edited
Loading

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

@jasnell jasnell force-pushed the stream-wrap-internal-binding branch 4 times, most recently from cf6aced to d305dfe Compare August 18, 2018 23:31
@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018

CI, again, after rebase: https://ci.nodejs.org/job/node-test-pull-request/16561/

@jasnell
Copy link
Member Author

jasnell commented Aug 19, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Aug 19, 2018

linuxone build bot failed... trying again https://ci.nodejs.org/job/node-test-commit-linuxone/4131/

@jasnell jasnell force-pushed the stream-wrap-internal-binding branch from d305dfe to ba2c323 Compare August 19, 2018 14:00
@jasnell
Copy link
Member Author

jasnell commented Aug 19, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Aug 19, 2018

@nodejs/build ... linuxone build bot is failing consistently.

@Trott
Copy link
Member

Trott commented Aug 19, 2018

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16575/

jasnell added a commit to jasnell/node that referenced this pull request Aug 19, 2018
@jasnell
stream: move process.binding('stream_wrap') to internalBinding
884b23d 
PR-URL: nodejs#22345
Refs: nodejs#22160
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@jasnell
Copy link
Member Author

jasnell commented Aug 19, 2018

Landed in 884b23d

@jasnell jasnell closed this Aug 19, 2018
@joyeecheung joyeecheung mentioned this pull request Aug 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@maclover7 maclover7 maclover7 approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jasnell @nodejs-github-bot @danbev @Trott @mcollina @maclover7 @joyeecheung @BridgeAR