deps,debugger: move node-inspect into core by Trott · Pull Request #38161 · nodejs/node

Trott · 2021-04-09T07:05:42Z

Refs: https://github.com/nodejs/node/discussions/36481

I named a lot of files/directories with inspector-cli and I suspect inspect-cli or debugger-cli (or just debugger or inspect) would have all been better?

Still some ESLint checks to re-enable in three files, but assuming tests pass everywhere, I think this is good enough to land and things can be adjusted with subsequent PRs.

Because I was porting over the tests, this found and fixed a small bug with node inspect in Node.js 15.x. Specifically, calling scripts would always include internal scripts, even with scripts(false). (Wouldn't mind reconsidering that API as that first argument is a boolean trap. But OMG not when first landing this thing.)

Also there are some changes that landed in the node-inspect repo but never made it into a release. Those changes will need to be ported over. But that's the next PR. (Or maybe the lint fixes would be the next one.)

Trott · 2021-04-09T07:06:28Z

@nodejs/diagnostics

Trott · 2021-04-09T07:12:30Z

(Easier to review, I think, if you look at each commit separately.)

mcollina

lgtm

hybrist

Thanks so much for working on this!

Trott · 2021-04-11T04:50:00Z

Anyone know what the right thing to do with this failing test on sharedlibs_openssl300 might be?

https://ci.nodejs.org/job/node-test-commit-linux-containered/26443/nodes=ubuntu1804_sharedlibs_openssl300_x64/testReport/junit/(root)/test/inspector_cli_test_inspector_cli_auto_resume/

/home/iojs/build/workspace/node-test-commit-linux-containered/out/Release/node: error while loading shared libraries: libcrypto.so.3: cannot open shared object file: No such file or directory

@danbev

Trott · 2021-04-11T04:52:29Z

Getting a lot of ECONNRESET on Windows and wondering how I might make the tests more robust.

https://ci.nodejs.org/job/node-test-binary-windows-js-suites/9309/#showFailuresLink

@nodejs/platform-windows

richardlau · 2021-04-11T06:44:59Z

Anyone know what the right thing to do with this failing test on sharedlibs_openssl300 might be?
https://ci.nodejs.org/job/node-test-commit-linux-containered/26443/nodes=ubuntu1804_sharedlibs_openssl300_x64/testReport/junit/(root)/test/inspector_cli_test_inspector_cli_auto_resume/
/home/iojs/build/workspace/node-test-commit-linux-containered/out/Release/node: error while loading shared libraries: libcrypto.so.3: cannot open shared object file: No such file or directory

@Trott The test needs to pass on the environment when it spawns a child node process — specifically on Linux LD_LIBRARY_PATH needs to be passed on (other operating systems use different but equivalent variables that would need to be passed on if node is dynamically linked to a non-system shared library).

Trott · 2021-04-11T19:21:49Z

The failed tests in CI on Windows looks (superficially at least) like maybe it's #37224. Might be a significant legitimate bug seen in node-inspect + Node.js 15 + WIndows. Any ideas about how to best fix?

nodejs-github-bot · 2021-04-24T14:43:27Z

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

nodejs-github-bot · 2021-04-24T14:46:48Z

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

nodejs-github-bot · 2021-04-24T22:04:10Z

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

Trott · 2021-04-24T23:49:29Z

CI is green, and I think this is ready to land, but a lot has changed since it's last review, so a review, or a re-review, or a rubber-stamp LGTM would be great. Anyone?

node-inspect developers have agreed to move node-inspect into core rather than vendor it as a dependency. Refs: https://github.com/nodejs/node/discussions/36481 PR-URL: nodejs#38161 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: nodejs#38161 Refs: https://github.com/nodejs/node/discussions/36481 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Remove code that was for when `node-inspect` was called as a standalone process. PR-URL: nodejs#38161 Refs: https://github.com/nodejs/node/discussions/36481 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Fixes: nodejs#37224 PR-URL: nodejs#38161 Refs: https://github.com/nodejs/node/discussions/36481 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: nodejs#38161 Refs: https://github.com/nodejs/node/discussions/36481 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Make five attempts with a timeout of 1 second each rather than 10 attempts with a timeout of 500ms each. This is to allow for slower-connecting devices like Raspberry Pi. PR-URL: nodejs#38161 Refs: https://github.com/nodejs/node/discussions/36481 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>

The test was assuming that the entire string being sought would arrive in a single data chunk, but it can be split across multiple chunks. PR-URL: nodejs#38161 Refs: https://github.com/nodejs/node/discussions/36481 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Migrate the node-inspect tests to core (where node-inspect code now lives) and remove node-inspect from deps directory. PR-URL: nodejs#38161 Refs: https://github.com/nodejs/node/discussions/36481 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Change process.binding() use to internalBinding(). PR-URL: nodejs#38161 Refs: https://github.com/nodejs/node/discussions/36481 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: nodejs#38161 Refs: https://github.com/nodejs/node/discussions/36481 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Trott · 2021-04-25T08:52:31Z

Landed in dfc00ea...2a70f4f

targos · 2021-05-03T13:03:37Z

Is this really a notable change for users? What should I write in the changelog/blog post about it ?

Trott · 2021-05-03T13:08:57Z

Is this really a notable change for users? What should I write in the changelog/blog post about it ?

You can leave it off the notable changes and remove the label.

Trott · 2021-05-03T13:09:28Z

In fact, I just removed the label myself.

targos · 2021-05-30T09:26:42Z

Do you want to backport this to v14.x-staging?

Trott · 2021-05-30T18:57:49Z

Do you want to backport this to v14.x-staging?

No, I don't want to. But yes, I guess I should. #38858

I didn't compile or run tests locally but there was only one merge conflict that was straightforward to resolve, so I think it should work.

nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Apr 9, 2021

Trott changed the title ~~Node inspect~~deps,debugger: move node-inspect into coreApr 9, 2021

Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Apr 9, 2021

Trott requested a review from hybrist April 9, 2021 07:06

mcollina approved these changes Apr 9, 2021
View reviewed changes

Trott force-pushed the node-inspect branch from 7abbfed to dce5476Compare April 9, 2021 13:49

hybrist approved these changes Apr 9, 2021
View reviewed changes

cjihrig approved these changes Apr 9, 2021
View reviewed changes

Trott force-pushed the node-inspect branch 2 times, most recently from 267e87c to 1069e36Compare April 10, 2021 05:13

Qard approved these changes Apr 10, 2021
View reviewed changes

Trott force-pushed the node-inspect branch from d1647d4 to a85d5adCompare April 10, 2021 22:11

This comment has been minimized.
Sign in to view

Trott force-pushed the node-inspect branch from a85d5ad to aef35b5Compare April 11, 2021 00:59

This comment has been minimized.
Sign in to view

Trott force-pushed the node-inspect branch from e76f8bc to fdd596eCompare April 11, 2021 13:53

This comment has been minimized.
Sign in to view

Flarna approved these changes Apr 13, 2021
View reviewed changes

Trott force-pushed the node-inspect branch from 65ee417 to 83ac996Compare April 16, 2021 09:45

This comment has been minimized.
Sign in to view

targos reviewed Apr 25, 2021
View reviewed changes

Comment threadlib/internal/inspector/_inspect.js Outdated

targos approved these changes Apr 25, 2021
View reviewed changes

Comment threadlib/internal/inspector/inspect_client.js Outdated
Comment threadlib/internal/inspector/inspect_repl.js Outdated

Trott added 12 commits April 25, 2021 01:49

targos mentioned this pull request May 3, 2021
v16.1.0 release proposal #38522
Merged

Trott mentioned this pull request May 30, 2021
[v14.x backport] deps,debugger: move node-inspect into core #38858
Closed

richardlau mentioned this pull request Jul 20, 2021
v14.17.4 proposal #39466
Merged

This was referenced Feb 24, 2026
inspector: unwrap internal/debugger/inspect imports Renegade334/node#1
Closed
inspector: unwrap internal/debugger/inspect imports #61974
Merged

Uh oh!

Uh oh!

Conversation

Trott commented Apr 9, 2021

Uh oh!

Trott commented Apr 9, 2021

Uh oh!

Trott commented Apr 9, 2021

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

hybrist left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Trott commented Apr 11, 2021

Uh oh!

Trott commented Apr 11, 2021

Uh oh!

This comment has been minimized.

richardlau commented Apr 11, 2021

Uh oh!

This comment has been minimized.

Trott commented Apr 11, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Apr 24, 2021

Uh oh!

nodejs-github-bot commented Apr 24, 2021

Uh oh!

nodejs-github-bot commented Apr 24, 2021

Uh oh!

Trott commented Apr 24, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Trott commented Apr 25, 2021

Uh oh!

targos commented May 3, 2021

Uh oh!

Trott commented May 3, 2021

Uh oh!

Trott commented May 3, 2021

Uh oh!

targos commented May 30, 2021

Uh oh!

Trott commented May 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Trott commented Apr 11, 2021•
edited
Loading