Skip to content

inspector: roll inspector_protocol to match v8's - #56649

Closed
legendecas wants to merge 3 commits into
nodejs:mainfrom
legendecas:inspector/roll-protocol
Closed

inspector: roll inspector_protocol to match v8's#56649
legendecas wants to merge 3 commits into
nodejs:mainfrom
legendecas:inspector/roll-protocol

Conversation

@legendecas

@legendecas legendecas commented Jan 17, 2025

Copy link
Copy Markdown
Member

tools: update inspector_protocol roller

Fix the inspector_protocol/roll.py to fit node source directory
structure.

roll.py reads the deps/v8/thrid_party/inspector_protocol/README.v8
to get the revision of the inspector_protocol that V8 depends on, and
updates the local version to match.

V8's copy of inspector_protocol modifies the namespace of crdtp
library and does not export the symbols. So it can not be used outside
of V8.

inspector: roll inspector_protocol

Roll the inspector_protocol library to match V8's inspector_protocol
revision.

Update the node inspector to use the new crdtp protocol library.

deps: move inspector_protocol to deps

The crdtp library in the inspector_protocol is compiled as a library
and linked to the node executable.

@nodejs-github-bot

nodejs-github-bot commented Jan 17, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/security-wg
  • @nodejs/inspector
  • @nodejs/cpp-reviewers

@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. labels Jan 17, 2025
@legendecas legendecas added the inspector Issues and PRs related to the V8 inspector protocol label Jan 17, 2025
@legendecas
legendecas marked this pull request as ready for review January 17, 2025 23:58
@legendecas
legendecas force-pushed the inspector/roll-protocol branch 3 times, most recently from a9069ea to be85fe3 Compare January 18, 2025 00:19
@codecov

codecov Bot commented Jan 18, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 60.65574% with 72 lines in your changes missing coverage. Please review.

Project coverage is 89.20%. Comparing base (d978610) to head (afcb230).
Report is 363 commits behind head on main.

Files with missing lines Patch % Lines
src/inspector/node_json.cc 48.95% 39 Missing and 10 partials ⚠️
src/inspector/node_string.cc 59.25% 10 Missing and 1 partial ⚠️
src/inspector/tracing_agent.cc 61.53% 4 Missing and 1 partial ⚠️
src/inspector_agent.cc 81.48% 4 Missing and 1 partial ⚠️
src/inspector/network_agent.cc 87.50% 0 Missing and 1 partial ⚠️
src/inspector/worker_agent.cc 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56649      +/-   ##
==========================================
- Coverage   89.21%   89.20%   -0.02%     
==========================================
  Files         662      663       +1     
  Lines      191945   192180     +235     
  Branches    36948    36988      +40     
==========================================
+ Hits       171238   171425     +187     
- Misses      13549    13573      +24     
- Partials     7158     7182      +24     
Files with missing lines Coverage Δ
src/inspector/node_string.h 57.14% <100.00%> (-16.20%) ⬇️
src/inspector/runtime_agent.cc 100.00% <100.00%> (ø)
src/inspector/network_agent.cc 94.89% <87.50%> (+0.21%) ⬆️
src/inspector/worker_agent.cc 90.36% <80.00%> (ø)
src/inspector/tracing_agent.cc 89.47% <61.53%> (-1.44%) ⬇️
src/inspector_agent.cc 79.74% <81.48%> (-0.40%) ⬇️
src/inspector/node_string.cc 60.00% <59.25%> (+4.82%) ⬆️
src/inspector/node_json.cc 48.95% <48.95%> (ø)

... and 52 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 18, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 21, 2025
@nodejs-github-bot

This comment was marked as outdated.

Fix the inspector_protocol/roll.py to fit node source directory
structure.

`roll.py` reads the `deps/v8/thrid_party/inspector_protocol/README.v8`
to get the revision of the inspector_protocol that V8 depends on, and
updates the local version to match.

V8's copy of inspector_protocol modifies the namespace of `crdtp`
library and does not export the symbols. So it can not be used outside
of V8.
@legendecas
legendecas force-pushed the inspector/roll-protocol branch from be85fe3 to 601a566 Compare January 23, 2025 09:48
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 23, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 23, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 23, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 23, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

legendecas and others added 2 commits January 25, 2025 16:02
Roll the inspector_protocol library to match V8's inspector_protocol
revision.

Update the node inspector to use the new `crdtp` protocol library.
The `crdtp` library in the inspector_protocol is compiled as a library
and linked to the node executable.
@legendecas
legendecas force-pushed the inspector/roll-protocol branch from ab3d03c to afcb230 Compare January 25, 2025 16:02
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 25, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 25, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@legendecas

Copy link
Copy Markdown
Member Author

Fixed CI failures on AIX/linuxone. @nodejs/inspector would you mind taking another review? Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dependencies Pull requests that update a dependency file. inspector Issues and PRs related to the V8 inspector protocol needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants