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.

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

src,lib: remove dead process.binding() code #25829

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

There are no non-internal builtin modules left, so this
should be safe to remove to a large degree.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 30, 2019
@addaleax
Copy link
Member Author

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 30, 2019
@@ -33,9 +33,6 @@ enum {
nullptr}; \
void _register_##modname() { node_module_register(&_module); }

#define NODE_BUILTIN_MODULE_CONTEXT_AWARE(modname, regfunc) \
Copy link
Member

Choose a reason for hiding this comment

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

🎉

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

By the way and apropos nothing, node_binding.h includes itself. Accidental change from 13abc6a?

env->ThrowError("Built-in module self-registered.");
return false;
}
CHECK_EQ(mp->nm_flags & NM_F_BUILTIN, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe invert this to CHECK_EQ(0, mp->nm_flags & ~(NM_F_INTERNAL | NM_F_LINKED))? That way you can get rid of NM_F_BUILTIN and it'll also help catch accidental corruption.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we aren’t introducing new flags too often, but maybe at some point we’ll want to add a new flag in a backwards-compatible way?

@addaleax
Copy link
Member Author

By the way and apropos nothing, node_binding.h includes itself.

Yeah, removed that – thanks for noticing 👍

@joyeecheung
Copy link
Member

ping @addaleax this needs a rebase

There are no non-internal builtin modules left, so this
should be safe to remove to a large degree.
@addaleax
Copy link
Member Author

addaleax commented Feb 1, 2019

@addaleax
Copy link
Member Author

addaleax commented Feb 3, 2019

Landed in 270ffb0

@addaleax addaleax closed this Feb 3, 2019
@addaleax addaleax deleted the dead-process-binding branch February 3, 2019 19:52
addaleax added a commit that referenced this pull request Feb 3, 2019
There are no non-internal builtin modules left, so this
should be safe to remove to a large degree.

PR-URL: #25829
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@addaleax
Copy link
Member Author

addaleax commented Feb 3, 2019

(I’m adding backport-requested-v11.x – this should not be hard to backport, but it would be good to backport other PRs first and have this apply cleanly.)

@targos
Copy link
Member

targos commented Feb 10, 2019

This lands cleanly on v11.x-staging and tests pass but is it safe to land?

@targos
Copy link
Member

targos commented Feb 10, 2019

Feel free to land it on staging if the answer is yes.

addaleax added a commit that referenced this pull request Feb 10, 2019
There are no non-internal builtin modules left, so this
should be safe to remove to a large degree.

PR-URL: #25829
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@targos targos mentioned this pull request Feb 14, 2019
deepak1556 added a commit to electron/electron that referenced this pull request Mar 6, 2019
NODE_BUILTING_MODULE_CONTEXT_AWARE and process.binding are
removed in nodejs/node#25829. This changes
uses the alternative available without any functionality change.
deepak1556 added a commit to electron/electron that referenced this pull request Mar 7, 2019
NODE_BUILTING_MODULE_CONTEXT_AWARE and process.binding are
removed in nodejs/node#25829. This changes
uses the alternative available without any functionality change.
nornagon pushed a commit to electron/electron that referenced this pull request Mar 8, 2019
…17247)

* refactor: load electron builtin modules with process._linkedBinding

NODE_BUILTING_MODULE_CONTEXT_AWARE and process.binding are
removed in nodejs/node#25829. This changes
uses the alternative available without any functionality change.

* chore: roll node
nitsakh pushed a commit to electron/electron that referenced this pull request Mar 27, 2019
…17247)

* refactor: load electron builtin modules with process._linkedBinding

NODE_BUILTING_MODULE_CONTEXT_AWARE and process.binding are
removed in nodejs/node#25829. This changes
uses the alternative available without any functionality change.

* chore: roll node
nitsakh pushed a commit to electron/electron that referenced this pull request Mar 28, 2019
…17247)

* refactor: load electron builtin modules with process._linkedBinding

NODE_BUILTING_MODULE_CONTEXT_AWARE and process.binding are
removed in nodejs/node#25829. This changes
uses the alternative available without any functionality change.

* chore: roll node
zcbenz pushed a commit to electron/electron that referenced this pull request Apr 10, 2019
…17247)

* refactor: load electron builtin modules with process._linkedBinding

NODE_BUILTING_MODULE_CONTEXT_AWARE and process.binding are
removed in nodejs/node#25829. This changes
uses the alternative available without any functionality change.

* chore: roll node
ckerr pushed a commit to electron/electron that referenced this pull request Apr 16, 2019
* fix: backport boringssl patches for node compat

* refactor: prevent node macros from overriding base (#17178)

* fix: backport boring ssl patch OPENSSL_clear_free

* refactor: load electron builtin modules with process._linkedBinding (#17247)

* refactor: load electron builtin modules with process._linkedBinding

NODE_BUILTING_MODULE_CONTEXT_AWARE and process.binding are
removed in nodejs/node#25829. This changes
uses the alternative available without any functionality change.

* chore: roll node

* fix: add boringssl backport to support node upgrade

* fix: Update node_includes.h, add DCHECK macros

* fix: Update node Debug Options parser usage

* fix: Fix asar setup

* fix: using v8Util in isolated context

* fix: make "process" available in preload scripts

* fix: use proper options parser and remove setting of _breakFirstLine

_breakFirstLine was being set on the process, but that has changed in node 12 and so is no longer needed. Node will handle it properly when --inspect-brk is provided

* fix: process.binding => _linkedBinding in sandboxed isolated preload

* chore: update node dep sha

* fix: make original-fs work with streams
kiku-jw pushed a commit to kiku-jw/electron that referenced this pull request May 16, 2019
…lectron#17247)

* refactor: load electron builtin modules with process._linkedBinding

NODE_BUILTING_MODULE_CONTEXT_AWARE and process.binding are
removed in nodejs/node#25829. This changes
uses the alternative available without any functionality change.

* chore: roll node
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants