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

report: move diagnostic reports to stable #32242

Merged
merged 6 commits into from Mar 15, 2020
Merged

report: move diagnostic reports to stable #32242

merged 6 commits into from Mar 15, 2020

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 13, 2020

This PR promotes the experimental diagnostic report feature to stable, as outlined in #26293 (comment).

Fixes: #26293

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++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 13, 2020
@richardlau richardlau added the baking-for-lts PRs that need to wait before landing in a LTS release. label Mar 13, 2020
@gireeshpunathil
Copy link
Member

@cjihrig :

One of the item in the referenced issue is Make the --report-on-fatalerror CLI flag experimental , due to a bug in that API. #32207 is in place to address that, just an FYI.

@gireeshpunathil gireeshpunathil mentioned this pull request Mar 13, 2020
Closed
4 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 14, 2020

The CI is fine, except test-memory-usage is failing on ARM 12. #32239 might help?

@cjihrig cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 14, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 15, 2020 •
edited by cjihrig

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

@cjihrig cjihrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 15, 2020
@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 15, 2020

This can land, but it needs another review from @nodejs/tsc.

mcollina
mcollina approved these changes Mar 15, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@cjihrig
src: unconditionally include report feature
a6bd2c2 
This commit removes all #ifdef NODE_REPORT checks in the src
directory.

PR-URL: nodejs#32242
Fixes: nodejs#26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@cjihrig
build: remove node_report option in node.gyp
f1d3d92 
PR-URL: nodejs#32242
Fixes: nodejs#26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@cjihrig
build: make --without-report a no-op
765c375 
This commit makes the configure --without-report flag a no-op.
This commit also updates a test that depends on the report CLI
flags being conditionally present.

PR-URL: nodejs#32242
Fixes: nodejs#26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@cjihrig
test: remove common.skipIfReportDisabled()
4c64e7c 
The report feature won't ever be disabled moving forward, so
checking for its existence in the tests is no longer needed.

PR-URL: nodejs#32242
Fixes: nodejs#26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@cjihrig
doc,lib,src,test: make --experimental-report a nop
9d1a3b6 
This commit makes the --experimental-report CLI flag a no-op.

PR-URL: nodejs#32242
Fixes: nodejs#26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@cjihrig
doc: update stability of report features
a022d38 
This commit updates the stability documentation for the report
feature. All diagnostic report APIs are now listed as stable,
with the exception of report-on-fatalerror, which still has a
few bugs to work out.

PR-URL: nodejs#32242
Fixes: nodejs#26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@cjihrig cjihrig merged commit a022d38 into nodejs:master Mar 15, 2020
@cjihrig cjihrig deleted the report branch March 15, 2020 17:31
@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 15, 2020

Landed in 8617508...a022d38. Thanks for the reviews!

@gireeshpunathil gireeshpunathil mentioned this pull request Mar 16, 2020
Closed
MylesBorins added a commit that referenced this pull request Mar 24, 2020
@MylesBorins
2020-03-25 Version 13.12.0 (Current)
968751d 
Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * update npm to 6.14.3 (Myles Borins)
    #32368
  * update to uvwasi 0.0.6 (Colin Ihrig) [#32309](#32309)
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* node\_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 24, 2020
@MylesBorins
2020-03-25 Version 13.12.0 (Current)
27dbe1f 
Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * update npm to 6.14.3 (Myles Borins)
    #32368
  * update to uvwasi 0.0.6 (Colin Ihrig) [#32309](#32309)
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* node\_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 25, 2020
@MylesBorins
2020-03-26 Version 13.12.0 (Current)
a87ac59 
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * update npm to 6.14.3 (Myles Borins)
    #32368
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 26, 2020
@MylesBorins
2020-03-26 Version 13.12.0 (Current)
65782ff 
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * upgrade npm to 6.14.4 (Ruy Adorno)
    #32495
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 26, 2020
@MylesBorins
2020-03-26 Version 13.12.0 (Current)
f6a339f 
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * upgrade npm to 6.14.4 (Ruy Adorno)
    #32495
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 26, 2020
@MylesBorins
2020-03-26 Version 13.12.0 (Current)
113c04c 
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * upgrade npm to 6.14.4 (Ruy Adorno)
    #32495
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
@targos targos removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@cjihrig @targos
src: unconditionally include report feature
2939a45 
This commit removes all #ifdef NODE_REPORT checks in the src
directory.

PR-URL: nodejs#32242
Fixes: nodejs#26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@cjihrig @targos
build: remove node_report option in node.gyp
e895baf 
PR-URL: nodejs#32242
Fixes: nodejs#26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@cjihrig @targos
build: make --without-report a no-op
bda6d4c 
This commit makes the configure --without-report flag a no-op.
This commit also updates a test that depends on the report CLI
flags being conditionally present.

PR-URL: nodejs#32242
Fixes: nodejs#26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@cjihrig @targos
test: remove common.skipIfReportDisabled()
38ef658 
The report feature won't ever be disabled moving forward, so
checking for its existence in the tests is no longer needed.

PR-URL: nodejs#32242
Fixes: nodejs#26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@cjihrig @targos
doc,lib,src,test: make --experimental-report a nop
45c8c2d 
This commit makes the --experimental-report CLI flag a no-op.

PR-URL: nodejs#32242
Fixes: nodejs#26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@cjihrig @targos
doc: update stability of report features
5587d66 
This commit updates the stability documentation for the report
feature. All diagnostic report APIs are now listed as stable,
with the exception of report-on-fatalerror, which still has a
few bugs to work out.

PR-URL: nodejs#32242
Fixes: nodejs#26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@cjihrig @targos
src: unconditionally include report feature
0b310df 
This commit removes all #ifdef NODE_REPORT checks in the src
directory.

PR-URL: #32242
Fixes: #26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@cjihrig @targos
build: remove node_report option in node.gyp
70eaba1 
PR-URL: #32242
Fixes: #26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@cjihrig @targos
build: make --without-report a no-op
984ae30 
This commit makes the configure --without-report flag a no-op.
This commit also updates a test that depends on the report CLI
flags being conditionally present.

PR-URL: #32242
Fixes: #26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@cjihrig @targos
test: remove common.skipIfReportDisabled()
6a0bc83 
The report feature won't ever be disabled moving forward, so
checking for its existence in the tests is no longer needed.

PR-URL: #32242
Fixes: #26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@cjihrig @targos
doc,lib,src,test: make --experimental-report a nop
90d35ad 
This commit makes the --experimental-report CLI flag a no-op.

PR-URL: #32242
Fixes: #26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@cjihrig @targos
doc: update stability of report features
fd2486e 
This commit updates the stability documentation for the report
feature. All diagnostic report APIs are now listed as stable,
with the exception of report-on-fatalerror, which still has a
few bugs to work out.

PR-URL: #32242
Fixes: #26293
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@targos targos mentioned this pull request May 2, 2020
Merged
codebytere added a commit to electron/electron that referenced this pull request May 28, 2020
@codebytere
Unconditionally include report feature
e7e27e1 
nodejs/node#32242
codebytere added a commit to electron/electron that referenced this pull request Jun 1, 2020
@codebytere
Unconditionally include report feature
bd707b5 
nodejs/node#32242
jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 8, 2020
@codebytere
Unconditionally include report feature
5edb45f 
nodejs/node#32242
jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 9, 2020
@codebytere
Unconditionally include report feature
42a1745 
nodejs/node#32242
codebytere added a commit to electron/electron that referenced this pull request Jun 15, 2020
@codebytere
Unconditionally include report feature
7b7fa90 
nodejs/node#32242
codebytere added a commit to electron/electron that referenced this pull request Jun 17, 2020
@codebytere
Unconditionally include report feature
fa2634d 
nodejs/node#32242
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@devnexen devnexen devnexen approved these changes

@richardlau richardlau richardlau approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil 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++. lib / src Issues and PRs related to general changes in the lib or src directory. 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.

Discussion: Criteria for upgrading diagnostic reporting from experimental to stable
9 participants
@cjihrig @gireeshpunathil @nodejs-github-bot @mcollina @jasnell @devnexen @richardlau @addaleax @targos