Skip to content

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Aug 6, 2018

Move process.binding('uv') to internalBinding('uv')

/cc #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
Copy link
Collaborator

@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 6, 2018
@jasnell
Copy link
Member Author

jasnell commented Aug 6, 2018

@maclover7
Copy link
Contributor

CITGM (sqlite3 and winston have been flaky as of late): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1490/

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.

CITGM results look okay, LGTM if CI passes

@lpinca lpinca added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 7, 2018
@jasnell jasnell force-pushed the uv-internal-binding branch from d49a404 to eb95a6a Compare August 9, 2018 18:25
@jasnell jasnell force-pushed the uv-internal-binding branch from eb95a6a to 300895b Compare August 9, 2018 18:26
@jasnell
Copy link
Member Author

jasnell commented Aug 9, 2018

New CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/16307/

@jasnell
Copy link
Member Author

jasnell commented Aug 9, 2018

Redo on the custom-suites job: https://ci.nodejs.org/job/node-test-commit-custom-suites/586/

jasnell added a commit that referenced this pull request Aug 9, 2018
PR-URL: #22163
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
@jasnell
Copy link
Member Author

jasnell commented Aug 9, 2018

Landed in c7962dc

@maclover7
Copy link
Contributor

Hmm, looks like the CITGM results actually show a regression in the test suite for yargs@12.0.1 (cc @bcoe):

execa/lib/errname: unable to establish process.binding('uv') Error: No such module: uv
     at process.binding (internal/bootstrap/loaders.js:81:36)
     at Object.<anonymous> (/home/iojs/build/workspace/citgm-smoker/nodes/ubuntu1404-64/citgm_tmp/5361800e-a5e4-4803-9e84-d666d26c0fce/yargs/node_modules/nyc/node_modules/execa/lib/errname.js:12:15)
     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)
     at Module.require (internal/modules/cjs/loader.js:637:17)
     at require (internal/modules/cjs/helpers.js:20:18)
     at Object.<anonymous> (/home/iojs/build/workspace/citgm-smoker/nodes/ubuntu1404-64/citgm_tmp/5361800e-a5e4-4803-9e84-d666d26c0fce/yargs/node_modules/nyc/node_modules/execa/index.js:11:17)

@maclover7
Copy link
Contributor

Relevant PR from @addaleax to the yargs submodule that's actually causing the error: sindresorhus/execa#125

@addaleax
Copy link
Member

@maclover7 That PR should have prevented exactly this – I think this might just be something that requires dependency updates to nyc (and/or yargs)?

Also, I think this is a case where a runtime deprecation should really have been considered before landing the PR.

@jasnell
Copy link
Member Author

jasnell commented Aug 11, 2018

Sigh. We can add back the process.binding export for now alongside the internalBinding.

@jasnell
Copy link
Member Author

jasnell commented Aug 11, 2018

Will get a PR opened

@addaleax
Copy link
Member

@jasnell Maybe we can wrap process.binding so that it forwards but warns on a specified set of bindings? Something like:

const forward = ['uv', ...];
const originalProcessBinding = process.binding;
process.binding = function(id) {
  if (forward.includes(id)) {
    process.emitWarning(...);
    return internalBinding(id);
  } 
  return originalProcessBinding(id);
};

That might transitioning a bit smoother in general…

@jasnell
Copy link
Member Author

jasnell commented Aug 11, 2018

That's a better approach than what I was thinking @addaleax :-) Will go that route

jasnell added a commit to jasnell/node that referenced this pull request Aug 11, 2018
Selectively deprecate `process.binding()` and fallthrough

Refs: nodejs#22163
@jasnell
Copy link
Member Author

jasnell commented Aug 11, 2018

See #22269

jasnell added a commit that referenced this pull request Aug 15, 2018
Selectively fallthrough `process.binding()` to `internalBinding()`

Refs: #22163

PR-URL: #22269
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
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

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants