FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v11.x] backport warning handler refactoring by joyeecheung · Pull Request #26670 · nodejs/node · GitHub

/ node Public

[v11.x] backport warning handler refactoring - #26670

Closed
joyeecheung wants to merge 4 commits into
nodejs:v11.x-stagingfrom
joyeecheung:backport-warning
ConversationCommits (4)ChecksFiles changed
Closed

[v11.x] backport warning handler refactoring#26670
joyeecheung wants to merge 4 commits into
nodejs:v11.x-stagingfrom
joyeecheung:backport-warning

Conversation

joyeecheung commented Mar 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

process: handle node --debug deprecation in pre-execution

In addition, shim process._deprecatedDebugBrk in pre-execution.
This is a non-semver-major v11.x backport for
#25828.

Refs: #25828

process: call prepareMainThreadExecution in node inspect

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

process: set up process warning handler in pre-execution

Since it depends on environment variables.

PR-URL: #26466
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

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.

PR-URL: #26466
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

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

nodejs-github-bot commented Mar 14, 2019

Copy link
Copy Markdown
Collaborator

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

joyeecheung commented Mar 14, 2019

Copy link
Copy Markdown
Member Author

cc @BridgeAR

joyeecheung commented Mar 14, 2019

Copy link
Copy Markdown
Member Author

Ah, didn't see #26662

@refack did you do something similar to the first commit added in this PR? otherwise the tests would fail

refack commented Mar 14, 2019

Copy link
Copy Markdown
Contributor

I did something similar to eef44c4
in
93795d9

(since it's a backport I didn't go for finesse ;)

BridgeAR commented Mar 15, 2019

Copy link
Copy Markdown
Member

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

BridgeAR commented Mar 15, 2019

Copy link
Copy Markdown
Member

@joyeecheung this seems to fail some tests.

joyeecheung commented Mar 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

hmm, I just realized we can't just semi-backport #25828 like this, because currently there is no way for the JS land to precisely know about the invalid deprecation that don't exit the process directly (--inspect --debug-brk), which also blocks #26517 if we want to shim process._invalidDebug and process._deprecatedDebugBrk precisely on v11.x. So the question is:

  1. Is it worth it to add some separate logic in v11.x that informs the JS land about the precise values of --debug-brk? That would result in conflicts for any future changes to internal/options.js and src/node_options.*.
  2. Or is it okay to just throw warning and set process._deprecatedDebugBrk to true instead of exiting for --inspect-brk --debug?

I am leaning towards 2, because it doesn't seem to hurt much to not exit the process with some combinations of options that probably should be unused by now.

joyeecheung commented Mar 15, 2019

Copy link
Copy Markdown
Member Author

Huh, wait, looks like even before #25828 there was already no way from C++ to tell the difference between --inspect-brk --debug and --debug-brk --inspect. Both of them would result in a warning, and nobody really had noticed the difference in v11 so far..so I guess we can just go with option 2.

@refack The additional patch should also shim process._deprecatedDebugBrk in the user land in order to unblock #26517. (we don't need to shim process._invalidDebug though because it will exit the process directly)

process: handle node --debug deprecation in pre-execution
190a830 
In addition, shim `process._deprecatedDebugBrk` in pre-execution.
This is a non-semver-major v11.x backport for
nodejs#25828.

Refs: nodejs#25828
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>
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>
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>

joyeecheung commented Mar 15, 2019

Copy link
Copy Markdown
Member Author

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

joyeecheung commented Mar 22, 2019

Copy link
Copy Markdown
Member Author

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

targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
process: handle node --debug deprecation in pre-execution
0d4ccfe 
In addition, shim `process._deprecatedDebugBrk` in pre-execution.
This is a non-semver-major v11.x backport for
nodejs#25828.

PR-URL: nodejs#26670
Refs: nodejs#25828
targos pushed a commit that referenced this pull request Mar 27, 2019
process: handle node --debug deprecation in pre-execution
be3ea2a 
In addition, shim `process._deprecatedDebugBrk` in pre-execution.
This is a non-semver-major v11.x backport for
#25828.

PR-URL: #26670
Refs: #25828

targos commented Mar 27, 2019

Copy link
Copy Markdown
Member

Landed the first commit in v11.x-staging.
Others landed with #26662.

targos closed this Mar 27, 2019
targos mentioned this pull request Mar 27, 2019
Merged
kissofire mentioned this pull request Mar 7, 2021
Open
aliceUnhinged613 mentioned this pull request Mar 7, 2021
Open
ron-a20 mentioned this pull request Mar 8, 2021
Open
sitedata mentioned this pull request Mar 8, 2021
Open
meghasfdc mentioned this pull request Mar 8, 2021
Open
137717unity mentioned this pull request Mar 8, 2021
Open
Rednickle mentioned this pull request Mar 8, 2021
Open
Dmarch28 mentioned this pull request Mar 11, 2021
Open
Dmarch28 mentioned this pull request Mar 11, 2021
Open
This was referenced May 25, 2021
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Apr 7, 2022
Open
Open
Open
Open
Open
Open
Open
Open
Closed
snyk-bot mentioned this pull request Jan 30, 2023
Open
wedataintelligence mentioned this pull request Jan 30, 2023
Open
snyk-bot mentioned this pull request Jan 30, 2023
Open
molllyn1 mentioned this pull request Jan 31, 2023
Open
snyk-bot mentioned this pull request Jan 31, 2023
Open
baby636 mentioned this pull request Jan 31, 2023
Open
This was referenced Jan 31, 2023
Open
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL