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

process: move more dependency of environment variables to pre-execution #26466

Closed
wants to merge 3 commits into from
Closed

process: move more dependency of environment variables to pre-execution #26466

wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

process: call prepareMainThreadExecution in node inspect

Since we should treat the node-inspect as third-party
user code.

process: set up process warning handler in pre-execution

Since it depends on environment variables.

process: handle process.env.NODE_V8_COVERAGE in pre-execution

Since this depends on environment variable, and the worker threads
do not need to persist the variable value because they cannot
switch cwd.

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

@joyeecheung
process: call prepareMainThreadExecution in node inspect
bdbcd0f 
Since we should treat the node-inspect as third-party
user code.
@joyeecheung
process: set up process warning handler in pre-execution
0e97fa3 
Since it depends on environment variables.
@joyeecheung
process: handle process.env.NODE_V8_COVERAGE in pre-execution
410c588 
Since this depends on environment variable, and the worker threads
do not need to persist the variable value because they cannot
switch cwd.
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2779/pipeline

@joyeecheung joyeecheung changed the title process: move more dependency of environment variabels to pre-execution process: move more dependency of environment variables to pre-execution Mar 6, 2019
@joyeecheung
Copy link
Member Author

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

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. process Issues and PRs related to the process subsystem. labels Mar 6, 2019
@joyeecheung
Copy link
Member Author

Landed in e029bc9...f617a73

@joyeecheung joyeecheung closed this Mar 8, 2019
joyeecheung added a commit that referenced this pull request Mar 8, 2019
@joyeecheung
process: call prepareMainThreadExecution in node inspect
f9b76c3 
Since we should treat the node-inspect as third-party
user code.

PR-URL: #26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
joyeecheung added a commit that referenced this pull request Mar 8, 2019
@joyeecheung
process: set up process warning handler in pre-execution
d7543a7 
Since it depends on environment variables.

PR-URL: #26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
joyeecheung added a commit that referenced this pull request Mar 8, 2019
@joyeecheung
process: handle process.env.NODE_V8_COVERAGE in pre-execution
f617a73 
Since this depends on environment variable, and the worker threads
do not need to persist the variable value because they cannot
switch cwd.

PR-URL: #26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

This breaks on v11. @joyeecheung I guess we should try to backport this to reduce other conflicts. Would you be so kind and have a look?

@BridgeAR
Copy link
Member

BridgeAR commented Mar 13, 2019 •
edited
Loading

The first commit and third commit seem fine but the warning handler breaks. The third relies on the second one though.

@BridgeAR BridgeAR mentioned this pull request Mar 13, 2019
Closed
2 tasks
@joyeecheung
Copy link
Member Author

I think this should land cleanly after #25685 is backported?

@joyeecheung
Copy link
Member Author

Oh wait, this needs a non-semver-major form of #25828 as well

joyeecheung added a commit to joyeecheung/node that referenced this pull request Mar 13, 2019
@joyeecheung
process: call prepareMainThreadExecution in node inspect
6ddb7f6 
Since we should treat the node-inspect as third-party
user code.

PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Mar 13, 2019
@joyeecheung
process: set up process warning handler in pre-execution
15842af 
Since it depends on environment variables.

PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Mar 13, 2019
@joyeecheung
process: handle process.env.NODE_V8_COVERAGE in pre-execution
b7de782 
Since this depends on environment variable, and the worker threads
do not need to persist the variable value because they cannot
switch cwd.

PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@ZYSzys
Copy link
Member

ZYSzys commented Mar 14, 2019 •
edited
Loading

This can be backported without conflicts now (after #26659) , do we still need to backport this manually(open a backport PR) ?

Screen Shot 2019-03-14 at 9 05 09 PM

EDIT: there was an error if landed directly since #25828 was semver-major.

Screen Shot 2019-03-14 at 9 49 43 PM

@BridgeAR
Copy link
Member

@ZYSzys do you know if the tests pass as well? If so, I'll remove the label. In that case it will be pulled in the next release.

@ZYSzys
Copy link
Member

ZYSzys commented Mar 14, 2019

@BridgeAR Sadly tests weren't passed.

This PR dependent on #25828(that was semver-major) as @joyeecheung said.

@BridgeAR
Copy link
Member

@ZYSzys thanks for checking again!

@refack refack mentioned this pull request Mar 14, 2019
Closed
2 tasks
@refack refack self-assigned this Mar 14, 2019
@refack
Copy link
Contributor

refack commented Mar 14, 2019 •
edited
Loading

PR to backport #26662

@joyeecheung joyeecheung mentioned this pull request Mar 14, 2019
Closed
4 tasks
joyeecheung added a commit to joyeecheung/node that referenced this pull request Mar 15, 2019
@joyeecheung
process: call prepareMainThreadExecution in node inspect
e71c74e 
Since we should treat the node-inspect as third-party
user code.

PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Mar 15, 2019
@joyeecheung
process: set up process warning handler in pre-execution
50e4722 
Since it depends on environment variables.

PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Mar 15, 2019
@joyeecheung
process: handle process.env.NODE_V8_COVERAGE in pre-execution
95a3844 
Since this depends on environment variable, and the worker threads
do not need to persist the variable value because they cannot
switch cwd.

PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
refack pushed a commit to refack/node that referenced this pull request Mar 17, 2019
@joyeecheung @refack
process: call prepareMainThreadExecution in node inspect
4be5076 
Since we should treat the node-inspect as third-party
user code.

PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
refack pushed a commit to refack/node that referenced this pull request Mar 17, 2019
@joyeecheung @refack
process: set up process warning handler in pre-execution
7ab1a11 
Since it depends on environment variables.

PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
refack pushed a commit to refack/node that referenced this pull request Mar 17, 2019
@joyeecheung @refack
process: handle process.env.NODE_V8_COVERAGE in pre-execution
00e7dd2 
Since this depends on environment variable, and the worker threads
do not need to persist the variable value because they cannot
switch cwd.

PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
@joyeecheung @targos
process: call prepareMainThreadExecution in node inspect
03dba72 
Since we should treat the node-inspect as third-party
user code.

Backport-PR-URL: nodejs#26662
PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
@joyeecheung @targos
process: set up process warning handler in pre-execution
cc606e2 
Since it depends on environment variables.

Backport-PR-URL: nodejs#26662
PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
@joyeecheung @targos
process: handle process.env.NODE_V8_COVERAGE in pre-execution
4200fc3 
Since this depends on environment variable, and the worker threads
do not need to persist the variable value because they cannot
switch cwd.

Backport-PR-URL: nodejs#26662
PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos targos mentioned this pull request Mar 27, 2019
Merged
@refack refack removed their assignment Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@joyeecheung @nodejs-github-bot @BridgeAR @ZYSzys @refack @addaleax @targos