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

bootstrap: use different scripts to setup different configurations #30862

Closed
wants to merge 2 commits into from

Conversation

@joyeecheung
Copy link
Member

joyeecheung commented Dec 9, 2019

bootstrap: use different scripts to setup different configurations

This patch splits the handling of isMainThread and
ownsProcessState from conditionals in
lib/internal/bootstrap/node.js into different scripts under
lib/internal/bootstrap/switches/, and call them accordingly
from C++ after node.js is run.

This:

  • Creates a common denominator of the main thread and the worker
    thread bootstrap that can be snapshotted and shared by
    both.
  • Makes it possible to override the configurations on-the-fly.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Dec 9, 2019

@nodejs-github-bot

This comment has been minimized.

@mscdex

This comment has been minimized.

Copy link
Contributor

mscdex commented Dec 9, 2019

nit: i think if we decide we really need/want to separate these out, we should stick to more straight-forward filenames for them, such as is_main_thread.js/is_not_main_thread.js and owns_process_state.js/not_owns_process_state.js instead of simply true.js/false.js.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 13, 2019

@joyeecheung The C++ linter is failing due to some long lines.

This patch splits the handling of `isMainThread` and
`ownsProcessState` from conditionals in
`lib/internal/bootstrap/node.js` into different scripts under
`lib/internal/bootstrap/switches/`, and call them accordingly
from C++ after `node.js` is run.

This:

- Creates a common denominator of the main thread and the worker
  thread bootstrap that can be snapshotted and shared by
  both.
- Makes it possible to override the configurations on-the-fly.
@joyeecheung joyeecheung force-pushed the joyeecheung:post-worker branch from ab3f032 to a36cad6 Dec 14, 2019
@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 14, 2019

Still just one remaining C++ lint failure:

07:10:11 File "src/node.cc" does not use "Boolean"
…ions
@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Dec 16, 2019

Oops, sorry, I no longer run the linter locally ever since there's #29918 ..

@nodejs-github-bot

This comment has been minimized.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Dec 16, 2019

CI is green. ping @nodejs/process can I have some review please?

joyeecheung added a commit that referenced this pull request Dec 20, 2019
This patch splits the handling of `isMainThread` and
`ownsProcessState` from conditionals in
`lib/internal/bootstrap/node.js` into different scripts under
`lib/internal/bootstrap/switches/`, and call them accordingly
from C++ after `node.js` is run.

This:

- Creates a common denominator of the main thread and the worker
  thread bootstrap that can be snapshotted and shared by
  both.
- Makes it possible to override the configurations on-the-fly.

PR-URL: #30862
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Dec 20, 2019

Landed in c63d511 , thanks!

BridgeAR added a commit that referenced this pull request Jan 3, 2020
This patch splits the handling of `isMainThread` and
`ownsProcessState` from conditionals in
`lib/internal/bootstrap/node.js` into different scripts under
`lib/internal/bootstrap/switches/`, and call them accordingly
from C++ after `node.js` is run.

This:

- Creates a common denominator of the main thread and the worker
  thread bootstrap that can be snapshotted and shared by
  both.
- Makes it possible to override the configurations on-the-fly.

PR-URL: #30862
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos added a commit that referenced this pull request Jan 14, 2020
This patch splits the handling of `isMainThread` and
`ownsProcessState` from conditionals in
`lib/internal/bootstrap/node.js` into different scripts under
`lib/internal/bootstrap/switches/`, and call them accordingly
from C++ after `node.js` is run.

This:

- Creates a common denominator of the main thread and the worker
  thread bootstrap that can be snapshotted and shared by
  both.
- Makes it possible to override the configurations on-the-fly.

PR-URL: #30862
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos added a commit that referenced this pull request Jan 14, 2020
This patch splits the handling of `isMainThread` and
`ownsProcessState` from conditionals in
`lib/internal/bootstrap/node.js` into different scripts under
`lib/internal/bootstrap/switches/`, and call them accordingly
from C++ after `node.js` is run.

This:

- Creates a common denominator of the main thread and the worker
  thread bootstrap that can be snapshotted and shared by
  both.
- Makes it possible to override the configurations on-the-fly.

PR-URL: #30862
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.