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: use NativeModuleLoader to compile all the bootstrappers #24775

Closed
wants to merge 1 commit into from

Conversation

@joyeecheung
Copy link
Member

commented Dec 1, 2018

This patch moves all the bootstrapper compilation to use
NativeModuleLoader::CompileAndCall(). With this we no longer
need to mess with the error decoration and handling any more -
there is no point in handling the JS error occurred during bootstrapping
by ourselves, we should just crash or let the VM handle it.

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

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2018

cc @addaleax @jasnell @devsnek

Since the top-level indentation in the two bootstrappers are now eliminated, this patch would be easier to review with https://github.com/nodejs/node/pull/24775/files?w=1 (with the whitespace-only changes stripped)

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

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Dec 2, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2018

@addaleax @jasnell @devsnek @nodejs/process Can I have some reviews please? Thanks!

@devsnek
devsnek approved these changes Dec 3, 2018
src: use NativeModuleLoader to compile all the bootstrappers
This patch moves all the bootstrapper compilation to use
NativeModuleLoader::CompileAndCall(). With this we no longer
need to mess with the error decoration and handling any more -
there is no point in handling the JS error occurred during bootstrapping
by ourselves, we should just crash or let the VM handle it.

@joyeecheung joyeecheung force-pushed the joyeecheung:bootstrap-native branch from 5a0f201 to 47fd257 Dec 3, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2018

Landed in edcb950, thanks!

@joyeecheung joyeecheung closed this Dec 3, 2018

joyeecheung added a commit that referenced this pull request Dec 3, 2018
src: use NativeModuleLoader to compile all the bootstrappers
This patch moves all the bootstrapper compilation to use
NativeModuleLoader::CompileAndCall(). With this we no longer
need to mess with the error decoration and handling any more -
there is no point in handling the JS error occurred during bootstrapping
by ourselves, we should just crash or let the VM handle it.

PR-URL: #24775
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

This does not land cleanly on v11.x (it seems like it relies on the console changes which did not land cleanly and it is likely that they apply fine as soon as they got backported). Please open a backport or change the label accordingly.

@targos targos added this to Backport requested in v11.x Dec 7, 2018

@joyeecheung joyeecheung referenced this pull request Dec 22, 2018
7 of 7 tasks complete

@targos targos moved this from Backport requested to Don't land (for now) in v11.x Jan 1, 2019

@targos targos moved this from Don't land (for now) to Backport requested in v11.x Jan 1, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

This does not land cleanly or near cleanly even after applying the recent console commits :/.

@joyeecheung could you please have another look? There are a couple of PRs from you which still need backports but I do not know which is the first one to backport and if there's maybe a semver-major PR which should be backported without breaking changes similar to the console one.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2019

@BridgeAR I think this does not apply cleanly mostly because there are out-of-order commits backported to v11.x-staging before this one is backported. I'll try reverting those first before doing a clean backport

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

This could land cleanlier on v11.x-staging after

  1. Reverting these out-of-order commits first, in this order

e97acf7 lib: expose all type checks from the internal types module
c168672 inspector: move process.binding to internalBinding
617bfe4 lib: remove internalBinding('config').pendingDeprecation
55d185f os: move process.binding('os') to internalBinding
0cde1a4 lib: remove unused NativeModule/NativeModule wraps
88a5449 src,lib: make process.binding('config') internal
1ec4f8d lib: remove duplicated noop function

  1. Cherry-pick #22478

There will still be a small conflict in src/node.cc which I suspect is caused by the out-of-order cherry-pick of 56b2a7274c inspector: split the HostPort being used and the one parsed from CLI but simply reverting that could trigger some other bigger conflicts.

joyeecheung added a commit to joyeecheung/node that referenced this pull request Jan 11, 2019
src: use NativeModuleLoader to compile all the bootstrappers
This patch moves all the bootstrapper compilation to use
NativeModuleLoader::CompileAndCall(). With this we no longer
need to mess with the error decoration and handling any more -
there is no point in handling the JS error occurred during bootstrapping
by ourselves, we should just crash or let the VM handle it.

PR-URL: nodejs#24775
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax added a commit that referenced this pull request Jan 14, 2019
src: use NativeModuleLoader to compile all the bootstrappers
This patch moves all the bootstrapper compilation to use
NativeModuleLoader::CompileAndCall(). With this we no longer
need to mess with the error decoration and handling any more -
there is no point in handling the JS error occurred during bootstrapping
by ourselves, we should just crash or let the VM handle it.

PR-URL: #24775
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Backport-PR-URL: #25446
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
src: use NativeModuleLoader to compile all the bootstrappers
This patch moves all the bootstrapper compilation to use
NativeModuleLoader::CompileAndCall(). With this we no longer
need to mess with the error decoration and handling any more -
there is no point in handling the JS error occurred during bootstrapping
by ourselves, we should just crash or let the VM handle it.

PR-URL: nodejs#24775
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@BridgeAR BridgeAR moved this from Backport requested to Backported in v11.x Jan 14, 2019

@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
src: use NativeModuleLoader to compile all the bootstrappers
This patch moves all the bootstrapper compilation to use
NativeModuleLoader::CompileAndCall(). With this we no longer
need to mess with the error decoration and handling any more -
there is no point in handling the JS error occurred during bootstrapping
by ourselves, we should just crash or let the VM handle it.

PR-URL: nodejs#24775
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Backport-PR-URL: nodejs#25446
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.