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

bootstrap: mksnapshot should show JS error summaries #38174

Closed
wants to merge 6 commits into from
Closed

bootstrap: mksnapshot should show JS error summaries #38174

wants to merge 6 commits into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Apr 9, 2021

This came up while trying to debug some stuff going on in the snapshot, makes debugging a lot easier. IDK if we want to do something other than use abort() here.

@bmeck bmeck requested review from Trott and joyeecheung April 9, 2021 20:33
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 9, 2021
@bmeck bmeck added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels Apr 9, 2021
@bmeck bmeck force-pushed the mksnapshot-show-js-error branch from 8f5e7f7 to 2ffc11a Compare April 9, 2021 20:36
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@bmeck bmeck added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 9, 2021
targos
targos reviewed Apr 10, 2021
View reviewed changes
tools/snapshot/snapshot_builder.cc Outdated Show resolved Hide resolved
tools/snapshot/snapshot_builder.cc Outdated Show resolved Hide resolved
bmeck and others added 2 commits April 12, 2021 13:04
@bmeck @targos
Update tools/snapshot/snapshot_builder.cc
a460fa3 
Co-authored-by: Michaël Zasso <targos@protonmail.com>
@bmeck @targos
Update tools/snapshot/snapshot_builder.cc
9ff198e 
Co-authored-by: Michaël Zasso <targos@protonmail.com>
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@bmeck
Copy link
Member Author

bmeck commented Apr 13, 2021

CI is green except a timeout

bmeck added a commit that referenced this pull request Apr 13, 2021
@bmeck
bootstrap: mksnapshot should show JS error
8d9d823 
PR-URL: #38174
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bmeck
Copy link
Member Author

bmeck commented Apr 13, 2021

Landed in 837f7e2f71ce

@bmeck bmeck closed this Apr 13, 2021
BethGriggs pushed a commit that referenced this pull request Apr 15, 2021
@bmeck @BethGriggs
bootstrap: mksnapshot should show JS error
4183231 
PR-URL: #38174
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung
Copy link
Member

Sorry for missing the ping: just FYI SetIsolateErrorHandlers() can install error handlers that print uncaught exceptions and it's used by the ordinary Node.js instances - I don't quite remember why it wasn't used by mksnapshot, though. Also there are some routines in node_errors.cc that can be reused to print these errors.

@BethGriggs BethGriggs mentioned this pull request Apr 16, 2021
Merged
@bmeck bmeck deleted the mksnapshot-show-js-error branch February 3, 2022 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos left review comments

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Awaiting requested review from Trott

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. 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. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bmeck @nodejs-github-bot @joyeecheung @jasnell @addaleax @targos