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.

Sign up for GitHub

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

Jump to bottom

[v11.x] src: move more process methods initialization in bootstrap/node.js #25496

Closed
wants to merge 8 commits into from
Closed

[v11.x] src: move more process methods initialization in bootstrap/node.js #25496

wants to merge 8 commits into from

Conversation

addaleax
Copy link
Member

Backport #25127, and clean cherry-picks of #25165 and #25289 which should go with it.

joyeecheung and others added 2 commits January 14, 2019 17:04
@joyeecheung @addaleax
src: move more process methods initialization in bootstrap/node.js
427839e 
Instead of:

- Writing methods onto the process directly in C++ during
  `SetupProcessObject()` and overwrite with argument checks later
- Or, wrapping and writing them in `internal/process/*.js`

Do:

- Move the C++ implementations in node_process.cc and mark them static
  wherever possible
- Expose the C++ methods through a new
  `internalBinding('process_methods')`
- Wrap the methods in `internal/process/*.js` in a
  side-effect-free manner and return them back to
  `internal/bootstrap/node.js`
- Centralize the write to the process object based on conditions
  in `bootstrap/node.js`

So it's easier to see what methods are attached to the process object
during bootstrap under what condition and in what order.

The eventual goal is to figure out the dependency of process methods
and the write/read access to the process object during bootstrap, group
these access properly and remove the process properties that should not
be exposed to users this way.

Also correct the NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE milestone
which should be marked before code execution.

Refs: nodejs#24961

PR-URL: nodejs#25127
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig @addaleax
src: fix compiler warnings
5b780eb 
The warnings in question are:

../src/node.cc:844:13: warning:
unused function 'DebugProcess' [-Wunused-function]
static void DebugProcess(const FunctionCallbackInfo<Value>& args);
            ^
../src/node.cc:845:13: warning:
unused function 'DebugEnd' [-Wunused-function]
static void DebugEnd(const FunctionCallbackInfo<Value>& args);

PR-URL: nodejs#25165
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

@addaleax sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v11.x labels Jan 14, 2019
@addaleax

This comment has been minimized.

Sign in to view
@cjihrig @addaleax
coverage: use process._rawDebug() during setup
172466c 
console is not ready to use at this point in the bootstrapping
process, so switch to process._rawDebug() instead.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig @addaleax
coverage: pass cwd to path.resolve() in setup
dcd7767 
During coverage setup, path.resolve() is called.
path.resolve() can potentially call process.cwd(), which
hasn't been bootstrapped yet. This commit passes the
current working directory directly so that path.resolve()
doesn't attempt to compute it.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig @addaleax
test: make test-v8-coverage.js more strict
066985a 
Update the coverage test to verify that nothing is printed to
stderr (which happens when coverage errors happen). Also add a
test case to verify that non-absolute coverage paths work.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
Copy link
Member Author

addaleax commented Jan 14, 2019 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/20108/ (:white_check_mark:)

aoberoi and others added 3 commits January 14, 2019 19:33
@aoberoi @addaleax
util: fixes type in argument type validation error
5771dc4 
PR-URL: nodejs#25103
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@joyeecheung @addaleax
src: move node::errno_string into node_errors.h/cc
7dffd82 
Move `node::errno_string` into node_errors.h/cc and move it into
the `node:errors` namespace to reduce the size of the header.
It's not on any performance-critical code path so does not need
to be inlined.

PR-URL: nodejs#25396
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@ZYSzys @addaleax
test: add test for fs.lchmod
7bf10e3 
PR-URL: nodejs#25439
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
Copy link
Member Author

Landed in a4f50a6...067d38f

@addaleax addaleax closed this Jan 15, 2019
addaleax pushed a commit that referenced this pull request Jan 15, 2019
@joyeecheung @addaleax
src: move more process methods initialization in bootstrap/node.js
dde7152 
Instead of:

- Writing methods onto the process directly in C++ during
  `SetupProcessObject()` and overwrite with argument checks later
- Or, wrapping and writing them in `internal/process/*.js`

Do:

- Move the C++ implementations in node_process.cc and mark them static
  wherever possible
- Expose the C++ methods through a new
  `internalBinding('process_methods')`
- Wrap the methods in `internal/process/*.js` in a
  side-effect-free manner and return them back to
  `internal/bootstrap/node.js`
- Centralize the write to the process object based on conditions
  in `bootstrap/node.js`

So it's easier to see what methods are attached to the process object
during bootstrap under what condition and in what order.

The eventual goal is to figure out the dependency of process methods
and the write/read access to the process object during bootstrap, group
these access properly and remove the process properties that should not
be exposed to users this way.

Also correct the NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE milestone
which should be marked before code execution.

Refs: #24961

PR-URL: #25127
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #25496
addaleax pushed a commit that referenced this pull request Jan 15, 2019
@cjihrig @addaleax
src: fix compiler warnings
f8ba488 
The warnings in question are:

../src/node.cc:844:13: warning:
unused function 'DebugProcess' [-Wunused-function]
static void DebugProcess(const FunctionCallbackInfo<Value>& args);
            ^
../src/node.cc:845:13: warning:
unused function 'DebugEnd' [-Wunused-function]
static void DebugEnd(const FunctionCallbackInfo<Value>& args);

PR-URL: #25165
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>

Backport-PR-URL: #25496
addaleax pushed a commit that referenced this pull request Jan 15, 2019
@cjihrig @addaleax
coverage: use process._rawDebug() during setup
daca318 
console is not ready to use at this point in the bootstrapping
process, so switch to process._rawDebug() instead.

PR-URL: #25289
Fixes: #25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #25496
addaleax pushed a commit that referenced this pull request Jan 15, 2019
@cjihrig @addaleax
coverage: pass cwd to path.resolve() in setup
63fbd00 
During coverage setup, path.resolve() is called.
path.resolve() can potentially call process.cwd(), which
hasn't been bootstrapped yet. This commit passes the
current working directory directly so that path.resolve()
doesn't attempt to compute it.

PR-URL: #25289
Fixes: #25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #25496
addaleax pushed a commit that referenced this pull request Jan 15, 2019
@cjihrig @addaleax
test: make test-v8-coverage.js more strict
067d38f 
Update the coverage test to verify that nothing is printed to
stderr (which happens when coverage errors happen). Also add a
test case to verify that non-absolute coverage paths work.

PR-URL: #25289
Fixes: #25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #25496
@addaleax addaleax deleted the backport-v11.x-25127 branch January 15, 2019 17:48
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@joyeecheung @BridgeAR
src: move more process methods initialization in bootstrap/node.js
940eb8b 
Instead of:

- Writing methods onto the process directly in C++ during
  `SetupProcessObject()` and overwrite with argument checks later
- Or, wrapping and writing them in `internal/process/*.js`

Do:

- Move the C++ implementations in node_process.cc and mark them static
  wherever possible
- Expose the C++ methods through a new
  `internalBinding('process_methods')`
- Wrap the methods in `internal/process/*.js` in a
  side-effect-free manner and return them back to
  `internal/bootstrap/node.js`
- Centralize the write to the process object based on conditions
  in `bootstrap/node.js`

So it's easier to see what methods are attached to the process object
during bootstrap under what condition and in what order.

The eventual goal is to figure out the dependency of process methods
and the write/read access to the process object during bootstrap, group
these access properly and remove the process properties that should not
be exposed to users this way.

Also correct the NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE milestone
which should be marked before code execution.

Refs: nodejs#24961

PR-URL: nodejs#25127
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: nodejs#25496
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@cjihrig @BridgeAR
src: fix compiler warnings
4e702e5 
The warnings in question are:

../src/node.cc:844:13: warning:
unused function 'DebugProcess' [-Wunused-function]
static void DebugProcess(const FunctionCallbackInfo<Value>& args);
            ^
../src/node.cc:845:13: warning:
unused function 'DebugEnd' [-Wunused-function]
static void DebugEnd(const FunctionCallbackInfo<Value>& args);

PR-URL: nodejs#25165
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>

Backport-PR-URL: nodejs#25496
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@cjihrig @BridgeAR
coverage: use process._rawDebug() during setup
d7534bf 
console is not ready to use at this point in the bootstrapping
process, so switch to process._rawDebug() instead.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: nodejs#25496
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@cjihrig @BridgeAR
coverage: pass cwd to path.resolve() in setup
0213d0a 
During coverage setup, path.resolve() is called.
path.resolve() can potentially call process.cwd(), which
hasn't been bootstrapped yet. This commit passes the
current working directory directly so that path.resolve()
doesn't attempt to compute it.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: nodejs#25496
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@cjihrig @BridgeAR
test: make test-v8-coverage.js more strict
c648bb6 
Update the coverage test to verify that nothing is printed to
stderr (which happens when coverage errors happen). Also add a
test case to verify that non-absolute coverage paths work.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: nodejs#25496
@addaleax addaleax mentioned this pull request Jan 23, 2019
Closed
This was referenced Aug 9, 2020
Open
Open
Open
Open
Open
Open
Merged
@snyk-bot snyk-bot mentioned this pull request Oct 29, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@addaleax @nodejs-github-bot @joyeecheung @cjihrig @aoberoi @ZYSzys