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

deps: update inspector_protocol to c149e90e9ff5bf7 #37574

Closed
wants to merge 2 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Mar 2, 2021

Hopefully this should fix the linux-coverage GitHub Action.

To update it, I used the following commands:

rm -r tools/inspector_protocol
cp -r deps/v8/third_party/inspector_protocol tools/inspector_protocol
git add tools/inspector_protocol
git checkout -- tools/inspector_protocol/jinja2
git checkout -- tools/inspector_protocol/markupsafe
git commit
git cherry-pick 4662f67e3849d1e3d5127cc5ebc469d1febca57f

Then I had to update src/inspector/node_inspector.gypi to reflect the changes in tools/inspector_protocol/inspector_protocol.gni.

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Mar 2, 2021
@aduh95 aduh95 added inspector Issues and PRs related to the V8 inspector protocol needs-ci PRs that need a full CI run. labels Mar 2, 2021
@targos
Copy link
Member

targos commented Mar 2, 2021

See #27770

Last time I updated inspector_protocol, it was by copying the files that V8 keeps in https://github.com/nodejs/node/tree/master/deps/v8/third_party/inspector_protocol

@targos
Copy link
Member

targos commented Mar 2, 2021

Also see https://github.com/nodejs/node/commits/master/tools/inspector_protocol for the fixes that we float on top of it.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM (and assuming Jinga 2 is now a dependency)

@aduh95 aduh95 force-pushed the update-inspector-protocol branch from 7165d33 to 1d993ec Compare March 3, 2021 10:01
@targos
Copy link
Member

targos commented Mar 3, 2021

In the current code on master, jinja2 is included with inspector_protocol. that's why we don't need to install it externally

@aduh95 aduh95 added the wip Issues and PRs that are still a work in progress. label Mar 3, 2021
@aduh95 aduh95 force-pushed the update-inspector-protocol branch from 30f4a66 to fa98a8b Compare March 4, 2021 09:48
@aduh95
Copy link
Contributor Author

aduh95 commented Mar 4, 2021

It seems there are a lot of breaking changes in the update, I'm starting to wonder if I haven't pick a challenge too big for my skills. inspector_protocol is now using the Chrome DevTools Protocol Library which is a quite different implementation, and it would require a lot of refactoring work.
@targos wdyt? do you think it's worth keeping up with the upstream version?

@aduh95 aduh95 added stalled Issues and PRs that are stalled. and removed wip Issues and PRs that are still a work in progress. labels Mar 4, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2021

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@aduh95 aduh95 closed this May 2, 2021
@aduh95 aduh95 deleted the update-inspector-protocol branch May 2, 2021 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol needs-ci PRs that need a full CI run. stalled Issues and PRs that are stalled. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants