zlib: move process.binding('zlib') to internalBinding#23307
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added c++ 
addaleax commented Oct 9, 2018
thefourtheye left a comment
thefourtheye left a comment There was a problem hiding this comment.
Isn't this a major change?
cjihrig added the semver-major 
cjihrig commented Oct 9, 2018
Yes, it's part of the larger #22160. I've added the label. |
addaleax commented Oct 9, 2018
@thefourtheye@cjihrig Can you explain why this is semver-major? We specifically have the whitelist in |
jasnell commented Oct 9, 2018
We have been treating these all as semver-major even with the whitelist, just to be extra careful. |
I get that we can decide to be careful if we can imagine a chance of breakage – but I’m sorry, but labelling these PRs 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 |
jasnell commented Oct 10, 2018
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 commented Oct 10, 2018
Travis CI is failing on a missing subsystem but this does not seem correct as the commit does have a subsystem specified. |
addaleax commented Oct 10, 2018
@Trott ^^^ ? |
Travis does this: 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 -2commit 32b0d865929eff831bd6c00f7784ae74e06ebbcaMerge: eddfa2c52e 7765c16c72Author: Anna Henningsen <[email protected]>Date: Wed Oct 10 00:49:29 2018 +0000 Merge 7765c16c72eeb9572b347357c5eb482030d69dd2 into eddfa2c52ee91e657c1a3e42815e6dff960a7fe3commit eddfa2c52ee91e657c1a3e42815e6dff960a7fe3Author: 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 9d7895c567commit 9d7895c567e8f38abfff35da1b6d6d6a0a06f9aaAuthor: 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 commented Oct 10, 2018
I think I can fix it in a robust way using |
Use $TRAVIS_COMMIT_RANGE in .travis.yml to avoid merge commits in some situations. Refs: nodejs#23307 (comment)
Trott commented Oct 10, 2018
#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 commented Oct 10, 2018
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 commented Oct 12, 2018
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]>
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]>
11 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes