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

[v14.x backport] deps,debugger: move node-inspect into core by Trott · Pull Request #38858 · nodejs/node · GitHub

/ node Public

[v14.x backport] deps,debugger: move node-inspect into core - #38858

Closed
Trott wants to merge 12 commits into
nodejs:v14.x-stagingfrom
Trott:v14.x-backport-33161
ConversationCommits (12)ChecksFiles changed
Closed

[v14.x backport] deps,debugger: move node-inspect into core#38858
Trott wants to merge 12 commits into
nodejs:v14.x-stagingfrom
Trott:v14.x-backport-33161

Conversation

Trott commented May 30, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Original pull request: #38161

Trott commented May 30, 2021

Copy link
Copy Markdown
Member Author

@targos

github-actions 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. v14.x labels May 30, 2021
Trott mentioned this pull request May 30, 2021
Merged

Trott commented May 30, 2021

Copy link
Copy Markdown
Member Author

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.

bl-ue approved these changes May 30, 2021
View reviewed changes

bl-ue left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

RSLGTM

Trott force-pushed the v14.x-backport-33161 branch from be8bef8 to 6f3662c Compare May 30, 2021 19:04

This comment has been minimized.

Sign in to view

targos commented May 30, 2021

Copy link
Copy Markdown
Member

I should have said that I already tried to fix the conflict and run tests, but got failures.

Trott commented May 30, 2021

Copy link
Copy Markdown
Member Author

I should have said that I already tried to fix the conflict and run tests, but got failures.

Will Node.js 14 get V8 8.5 at some point?

targos commented May 30, 2021

Copy link
Copy Markdown
Member

I should have said that I already tried to fix the conflict and run tests, but got failures.

Will Node.js 14 get V8 8.5 at some point?

No, that likely won't happen.

targos force-pushed the v14.x-backport-33161 branch from 5f1ce4c to 0ea6546 Compare June 6, 2021 09:21

targos commented Jun 6, 2021

Copy link
Copy Markdown
Member

test-inspector-cli-address
11:22:00 ---

@targos I suspect that would be fixed by #38431. Want me to cherry-pick the commit from that PR over to this one? (Or feel free to do it yourself if you want--I think you should have write-access to the branch on this PR.) Or would you want to cherry-pick that separately rather than in this PR?

Thanks. I added that commit.

nodejs-github-bot commented Jun 6, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jun 6, 2021 •
edited by Trott
Loading

Copy link
Copy Markdown
Collaborator

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

richardlau commented Jul 15, 2021

Copy link
Copy Markdown
Member

This needs a rebase.

Trott commented Jul 15, 2021

Copy link
Copy Markdown
Member Author

This needs a rebase.

Merge conflicts resolved. Thanks!

nodejs-github-bot commented Jul 15, 2021

Copy link
Copy Markdown
Collaborator

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

richardlau commented Jul 15, 2021

Copy link
Copy Markdown
Member

Lots of test failures 😞

Trott commented Jul 15, 2021

Copy link
Copy Markdown
Member Author

Lots of test failures 😞

Guess I should have rebased (like you suggested) rather than cherry-picked anew atop the v14.x-staging branch. Guess I'll try again....

Trott added 12 commits July 15, 2021 15:09
debugger: move node-inspect to internal library
eff3c97 
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
Backport-PR-URL: nodejs#38858
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>
tools: remove node-inspect from license
d286b75 
PR-URL: nodejs#38161
Backport-PR-URL: nodejs#38858
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>
debugger: remove unused code
7ee45c0 
Remove code that was for when `node-inspect` was called as a standalone
process.

PR-URL: nodejs#38161
Backport-PR-URL: nodejs#38858
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>
debugger: fix inspect restart on Windows
802e4c1 
Fixes: nodejs#37224

PR-URL: nodejs#38161
Backport-PR-URL: nodejs#38858
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>
debugger: accommodate line chunking in Windows
8a6448d 
PR-URL: nodejs#38161
Backport-PR-URL: nodejs#38858
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>
debugger: allow longer time to connect
8ec940d 
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
Backport-PR-URL: nodejs#38858
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>
test: fix test-inspector-cli-address
e5ee810 
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
Backport-PR-URL: nodejs#38858
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>
test,debugger: migrate node-inspect tests to core
767dc25 
Migrate the node-inspect tests to core (where node-inspect code now
lives) and remove node-inspect from deps directory.

PR-URL: nodejs#38161
Backport-PR-URL: nodejs#38858
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>
debugger: replace internal use of deprecated API
9d5ab8f 
Change process.binding() use to internalBinding().

PR-URL: nodejs#38161
Backport-PR-URL: nodejs#38858
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>
test: fix comment typo
5357c18 
PR-URL: nodejs#38161
Backport-PR-URL: nodejs#38858
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>
debugger: fix race condition/deadlock on initialization
d2263ef 
PR-URL: nodejs#38161
Backport-PR-URL: nodejs#38858
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>
test: extend timeout on debugger tests for slower machines
22cbfa8 
PR-URL: nodejs#38161
Backport-PR-URL: nodejs#38858
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 commented Jul 15, 2021

Copy link
Copy Markdown
Member Author

OK, rebased. This should work better. It doesn't have the unsupported (in 14.x) replaceAll().

nodejs-github-bot commented Jul 15, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jul 16, 2021

Copy link
Copy Markdown
Collaborator

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

richardlau commented Jul 16, 2021

Copy link
Copy Markdown
Member

Landed in 848cf46...562b554.

richardlau mentioned this pull request Jul 19, 2021
Merged
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

1 more reviewer
bl-ue bl-ue approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL