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

Jump to bottom

src: allow setting a dir for all diagnostic output #33584

Closed
wants to merge 4 commits into from
Closed

src: allow setting a dir for all diagnostic output #33584

wants to merge 4 commits into from

Conversation

AshCripps
Copy link
Member

Add a flag that allows for the setting of a directory where all
diagnostic output will be written to.
e.g. --redirect-warnings

refs: #33010 (comment)

f.y.i @sam-github

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 nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. labels May 27, 2020
jasnell
jasnell reviewed May 27, 2020
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
jasnell
jasnell reviewed May 27, 2020
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
sam-github
sam-github suggested changes May 27, 2020
View reviewed changes
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea, I think it helps, a couple suggestions for consistency.

doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
src/node_options.cc Outdated Show resolved Hide resolved
src/node_options.h Outdated Show resolved Hide resolved
@AshCripps AshCripps changed the title [WIP] src: allow setting a dir for all diagnostic output src: allow setting a dir for all diagnostic output May 29, 2020
@AshCripps AshCripps marked this pull request as ready for review May 29, 2020 12:03
@richardlau
Copy link
Member

This could use some tests, particularly that the new option is only changing the default (i.e. if a more specific option is also used then the value of that is used instead of the more generic one).

sam-github
sam-github suggested changes May 29, 2020
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/node.1 Show resolved Hide resolved
@AshCripps AshCripps force-pushed the add-diagnostic-output branch 2 times, most recently from 88645e3 to 10b123b Compare May 29, 2020 17:04
@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@AshCripps
Copy link
Member Author

Ive added some testing for the --cpu-prof-dir option with use of diagnostic-dir (I adapted https://github.com/nodejs/node/blob/master/test/sequential/test-cpu-prof-dir-absolute.js) but before I write any more I would appreciate some feedback if these are correct as ive never written node test before so not 100% sure if this is the correct way.

@nodejs-github-bot
Copy link
Collaborator

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

joyeecheung
joyeecheung reviewed Jun 2, 2020
View reviewed changes
doc/api/cli.md Outdated
Defaults to current working directory.

Affects the default output directory of:
* [--cpu-prof-dir](#--cpu-prof-dir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering these two dir flags are still experimental, I think we could just emit a deprecation warning for them, and make them no-ops when --diagnostic-dir is set (IIUC that's what this PR effectively does). Eventually we can just remove them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

um, on the other hand, maybe we would also want to allow the user to override these directories individually through --cpu-prof-dir and --heap-prof-dir even when --diagnostic-dir is set?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats what I intented with:

  if (cpu_prof && cpu_prof_dir.empty() && !diagnostic_dir.empty()) {
      cpu_prof_dir = diagnostic_dir;
    }

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Jun 9, 2020
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Jun 18, 2020
Open
18 tasks
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 14, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@addaleax addaleax added cli Issues and PRs related to the Node.js command line interface. semver-minor PRs that contain new features and should be released in the next minor version. and removed process Issues and PRs related to the process subsystem. labels Jul 15, 2020
@nodejs-github-bot
Copy link
Collaborator

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

addaleax pushed a commit that referenced this pull request Jul 20, 2020
@addaleax
src: allow setting a dir for all diagnostic output
242bfb6 
Add a flag that allows for the setting of a directory where all
diagnostic output will be written to.
e.g. --redirect-warnings

Refs: #33010 (comment)

PR-URL: #33584
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax
Copy link
Member

Landed in 242bfb6

@addaleax addaleax closed this Jul 20, 2020
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
@cjihrig
src: allow setting a dir for all diagnostic output
de065a9 
Add a flag that allows for the setting of a directory where all
diagnostic output will be written to.
e.g. --redirect-warnings

Refs: #33010 (comment)

PR-URL: #33584
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 27, 2020
@MylesBorins
src: allow setting a dir for all diagnostic output
5c94358 
Add a flag that allows for the setting of a directory where all
diagnostic output will be written to.
e.g. --redirect-warnings

Refs: #33010 (comment)

PR-URL: #33584
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
ruyadorno added a commit that referenced this pull request Jul 28, 2020
@ruyadorno
2020-07-29, Version 14.7.0 (Current)
b04e1d7 
Notable changes:

dgram:
  * (SEMVER-MINOR) add IPv6 scope id suffix to received udp6 dgrams (Pekka Nikander) #14500
doc:
  * add AshCripps to collaborators (AshCripps) #34494
  * add HarshithaKP to collaborators (Harshitha K P) #34417
  * add rexagod to collaborators (Pranshu Srivastava) #34457
  * add release key for Richard Lau (Richard Lau) #34397
events:
  * (SEMVER-MINOR) expand NodeEventTarget functionality (Anna Henningsen) #34057
src:
  * (SEMVER-MINOR) allow preventing SetPromiseRejectCallback (Shelley Vohr) #34387
  * (SEMVER-MINOR) allow setting a dir for all diagnostic output (AshCripps) #33584
worker:
  * (SEMVER-MINOR) make MessagePort inherit from EventTarget (Anna Henningsen) #34057

PR-URL: TODO
@ruyadorno ruyadorno mentioned this pull request Jul 28, 2020
Merged
ruyadorno added a commit that referenced this pull request Jul 28, 2020
@ruyadorno
2020-07-29, Version 14.7.0 (Current)
44a6478 
Notable changes:

dgram:
  * (SEMVER-MINOR) add IPv6 scope id suffix to received udp6 dgrams (Pekka Nikander) #14500
doc:
  * add AshCripps to collaborators (AshCripps) #34494
  * add HarshithaKP to collaborators (Harshitha K P) #34417
  * add rexagod to collaborators (Pranshu Srivastava) #34457
  * add release key for Richard Lau (Richard Lau) #34397
events:
  * (SEMVER-MINOR) expand NodeEventTarget functionality (Anna Henningsen) #34057
src:
  * (SEMVER-MINOR) allow preventing SetPromiseRejectCallback (Shelley Vohr) #34387
  * (SEMVER-MINOR) allow setting a dir for all diagnostic output (AshCripps) #33584
worker:
  * (SEMVER-MINOR) make MessagePort inherit from EventTarget (Anna Henningsen) #34057
zlib:
  * switch to lazy init for zlib streams (Andrey Pechkurov) #34048

PR-URL: #34542
ruyadorno added a commit that referenced this pull request Jul 29, 2020
@ruyadorno
2020-07-29, Version 14.7.0 (Current)
6171265 
Notable changes:

deps:
  * upgrade npm to 6.14.7 (claudiahdz) #34468
dgram:
  * (SEMVER-MINOR) add IPv6 scope id suffix to received udp6 dgrams (Pekka Nikander) #14500
doc:
  * add AshCripps to collaborators (AshCripps) #34494
  * add HarshithaKP to collaborators (Harshitha K P) #34417
  * add rexagod to collaborators (Pranshu Srivastava) #34457
  * add release key for Richard Lau (Richard Lau) #34397
events:
  * (SEMVER-MINOR) expand NodeEventTarget functionality (Anna Henningsen) #34057
src:
  * (SEMVER-MINOR) allow preventing SetPromiseRejectCallback (Shelley Vohr) #34387
  * (SEMVER-MINOR) allow setting a dir for all diagnostic output (AshCripps) #33584
worker:
  * (SEMVER-MINOR) make MessagePort inherit from EventTarget (Anna Henningsen) #34057
zlib:
  * switch to lazy init for zlib streams (Andrey Pechkurov) #34048

PR-URL: #34542
MylesBorins pushed a commit that referenced this pull request Jul 29, 2020
@ruyadorno @MylesBorins
2020-07-29, Version 14.7.0 (Current)
168b22b 
Notable changes:

deps:
  * upgrade npm to 6.14.7 (claudiahdz) #34468
dgram:
  * (SEMVER-MINOR) add IPv6 scope id suffix to received udp6 dgrams (Pekka Nikander) #14500
doc:
  * add AshCripps to collaborators (AshCripps) #34494
  * add HarshithaKP to collaborators (Harshitha K P) #34417
  * add rexagod to collaborators (Pranshu Srivastava) #34457
  * add release key for Richard Lau (Richard Lau) #34397
events:
  * (SEMVER-MINOR) expand NodeEventTarget functionality (Anna Henningsen) #34057
src:
  * (SEMVER-MINOR) allow preventing SetPromiseRejectCallback (Shelley Vohr) #34387
  * (SEMVER-MINOR) allow setting a dir for all diagnostic output (AshCripps) #33584
worker:
  * (SEMVER-MINOR) make MessagePort inherit from EventTarget (Anna Henningsen) #34057
zlib:
  * switch to lazy init for zlib streams (Andrey Pechkurov) #34048

PR-URL: #34542
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@addaleax
src: allow setting a dir for all diagnostic output
403deb7 
Add a flag that allows for the setting of a directory where all
diagnostic output will be written to.
e.g. --redirect-warnings

Refs: #33010 (comment)

PR-URL: #33584
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
@mhdawson mhdawson mentioned this pull request Jan 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@richardlau richardlau richardlau approved these changes

@sam-github sam-github sam-github approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@BethGriggs BethGriggs BethGriggs 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. c++ Issues and PRs that require attention from people who are familiar with C++. cli Issues and PRs related to the Node.js command line interface. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@AshCripps @richardlau @nodejs-github-bot @addaleax @sam-github @jasnell @joyeecheung @BethGriggs