zlib: move process.binding('zlib') to internalBinding #23307

addaleax · 2018-10-07T04:19:19Z

Checklist

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

nodejs-github-bot · 2018-10-07T04:19:20Z

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

addaleax · 2018-10-09T02:47:52Z

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

thefourtheye

Isn't this a major change?

cjihrig · 2018-10-09T12:43:32Z

Yes, it's part of the larger #22160. I've added the label.

addaleax · 2018-10-09T16:48:47Z

@thefourtheye @cjihrig Can you explain why this is semver-major? We specifically have the whitelist in internal/bootstrap/node.js to avoid breakage, any this doesn’t introduce any new runtime or documentation deprecation.

jasnell · 2018-10-09T21:46:28Z

We have been treating these all as semver-major even with the whitelist, just to be extra careful.

addaleax · 2018-10-10T00:57:12Z

I get that we can decide to be careful if we can imagine a chance of breakage – but process.binding('zlib') is the only thing being changed here, and that keeps working and returns the same object.

I’m sorry, but labelling these PRs semver-major seems to have no effect to me except creating merge future conflicts.

If there are no firm objections, I’ll remove the label here and for the other PRs that added whitelisted modules. If you do have an objection, I’d really appreciate a reason or even just example code using process.binding() that would break.

jasnell · 2018-10-10T01:11:09Z

The process.binding method had been replaced entirely to enable the whitelist. It should be ok but we can't guarantee it. I'd prefer semver-major.

danbev · 2018-10-10T03:57:17Z

Travis CI is failing on a missing subsystem but this does not seem correct as the commit does have a subsystem specified.

addaleax · 2018-10-10T03:58:36Z

@Trott ^^^ ?

Trott · 2018-10-10T04:42:29Z

@Trott ^^^ ?

Travis does this:

$ git clone --depth=50 https://github.com/nodejs/node.git nodejs/node
$ cd nodejs/node
$ git fetch origin +refs/pull/23307/merge:
$ git checkout -qf FETCH_HEAD

For some reason, in this case but not in other pull requests as far as I have seen, that results in a merge commit only.

$ git log -2
commit 32b0d865929eff831bd6c00f7784ae74e06ebbca
Merge: eddfa2c52e 7765c16c72
Author: Anna Henningsen <[email protected]>
Date:   Wed Oct 10 00:49:29 2018 +0000

    Merge 7765c16c72eeb9572b347357c5eb482030d69dd2 into eddfa2c52ee91e657c1a3e42815e6dff960a7fe3

commit eddfa2c52ee91e657c1a3e42815e6dff960a7fe3
Author: Rich Trott <[email protected]>
Date:   Mon Sep 24 21:54:13 2018 -0700

    doc: simplify governance info in README intro
    
    Remove unnecessary wordiness in the governance sentence. It appears very
    early in the README so it should be short and clear. This also removes
    an instance of passive voice.
    
    PR-URL: https://github.com/nodejs/node/pull/23320
    Reviewed-By: Colin Ihrig <[email protected]>
    Reviewed-By: Vse Mozhet Byt <[email protected]>
    Reviewed-By: James M Snell <[email protected]>
$

The merge commit is skipped for commit linting, and since there are no other commits in the PR, this ends up linting the very first commit ever added to Node.js which indeed does not have a subsystem.

$ git log 9d7895c567
commit 9d7895c567e8f38abfff35da1b6d6d6a0a06f9aa
Author: Ryan <[email protected]>
Date:   Mon Feb 16 01:02:00 2009 +0100

    add dependencies
$

If someone more knowledgable about git can explain why this PR results in a merge commit when others apparently do not, I'd be grateful. I'm guessing that the PR branch is missing some commits on master that edit the same files as this PR, so there's a merge that has to happen? But that's a guess. If so, I imagine a rebase would fix the issue for this PR. Will have to think about how to fix it more globally, though...

Trott · 2018-10-10T04:48:00Z

I think I can fix it in a robust way using TRAVIS_COMMIT environment variable or something similar. PR coming soon...

Use $TRAVIS_COMMIT_RANGE in .travis.yml to avoid merge commits in some situations. Refs: nodejs#23307 (comment)

Trott · 2018-10-10T05:09:52Z

#23397 should fix that Travis issue. Feel free to head over to approve fast-tracking.

Use $TRAVIS_COMMIT_RANGE in .travis.yml to avoid merge commits in some situations. Refs: nodejs#23307 (comment) PR-URL: nodejs#23397 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

Trott · 2018-10-10T05:20:40Z

Travis issue (hopefully) fixed in a1edecc. A rebase here to get those changes should show a clean Travis lint run (although I suspect a rebase would have fixed it anyway).

Use $TRAVIS_COMMIT_RANGE in .travis.yml to avoid merge commits in some situations. Refs: #23307 (comment) PR-URL: #23397 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

addaleax · 2018-10-12T21:58:00Z

Landed in 09c42f4

PR-URL: #23307 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Use $TRAVIS_COMMIT_RANGE in .travis.yml to avoid merge commits in some situations. Refs: #23307 (comment) PR-URL: #23397 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

PR-URL: #23307 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Use $TRAVIS_COMMIT_RANGE in .travis.yml to avoid merge commits in some situations. Refs: #23307 (comment) PR-URL: #23397 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

zlib: move process.binding('zlib') to internalBinding

7765c16

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. zlib Issues and PRs related to the zlib subsystem. labels Oct 7, 2018

cjihrig approved these changes Oct 7, 2018

View reviewed changes

devsnek approved these changes Oct 7, 2018

View reviewed changes

lpinca approved these changes Oct 7, 2018

View reviewed changes

trivikr approved these changes Oct 7, 2018

View reviewed changes

starkwang approved these changes Oct 8, 2018

View reviewed changes

jasnell approved these changes Oct 8, 2018

View reviewed changes

thefourtheye approved these changes Oct 9, 2018

View reviewed changes

cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 9, 2018

jasnell mentioned this pull request Oct 9, 2018

Tracking Issue: process.binding to internalBinding #22160

Closed

33 tasks

Trott added a commit to Trott/io.js that referenced this pull request Oct 10, 2018

tools: make Travis commit linting more robust

ae9c90c

Use $TRAVIS_COMMIT_RANGE in .travis.yml to avoid merge commits in some situations. Refs: nodejs#23307 (comment)

Trott mentioned this pull request Oct 10, 2018

tools: make Travis commit linting more robust #23397

Closed

2 tasks

addaleax closed this Oct 12, 2018

addaleax deleted the zlib-internalbinding branch October 12, 2018 21:58

Trott mentioned this pull request Oct 18, 2018

timers: run nextTicks after each immediate and timer #22842

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zlib: move process.binding('zlib') to internalBinding #23307

zlib: move process.binding('zlib') to internalBinding #23307

addaleax commented Oct 7, 2018

nodejs-github-bot commented Oct 7, 2018

addaleax commented Oct 9, 2018

thefourtheye left a comment

cjihrig commented Oct 9, 2018

addaleax commented Oct 9, 2018

jasnell commented Oct 9, 2018

addaleax commented Oct 10, 2018 •

edited

Loading

jasnell commented Oct 10, 2018

danbev commented Oct 10, 2018

addaleax commented Oct 10, 2018

Trott commented Oct 10, 2018 •

edited

Loading

Trott commented Oct 10, 2018

Trott commented Oct 10, 2018

Trott commented Oct 10, 2018

addaleax commented Oct 12, 2018

zlib: move process.binding('zlib') to internalBinding #23307

zlib: move process.binding('zlib') to internalBinding #23307

Conversation

addaleax commented Oct 7, 2018

Checklist

nodejs-github-bot commented Oct 7, 2018

addaleax commented Oct 9, 2018

thefourtheye left a comment

Choose a reason for hiding this comment

cjihrig commented Oct 9, 2018

addaleax commented Oct 9, 2018

jasnell commented Oct 9, 2018

addaleax commented Oct 10, 2018 • edited Loading

jasnell commented Oct 10, 2018

danbev commented Oct 10, 2018

addaleax commented Oct 10, 2018

Trott commented Oct 10, 2018 • edited Loading

Trott commented Oct 10, 2018

Trott commented Oct 10, 2018

Trott commented Oct 10, 2018

addaleax commented Oct 12, 2018

addaleax commented Oct 10, 2018 •

edited

Loading

Trott commented Oct 10, 2018 •

edited

Loading