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

inspector: roll inspector_protocol to match v8's #56649

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jan 17, 2025

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.

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.
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 17, 2025

Review requested:

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

@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 2 times, most recently from 3e5e140 to a9069ea Compare January 18, 2025 00:06
legendecas and others added 2 commits January 18, 2025 00:19
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 a9069ea to be85fe3 Compare January 18, 2025 00:19
Copy link

codecov bot commented Jan 18, 2025

Codecov Report

Attention: Patch coverage is 64.36782% with 62 lines in your changes missing coverage. Please review.

Project coverage is 89.20%. Comparing base (2e45656) to head (be85fe3).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/inspector/node_json_protocol.cc 55.00% 28 Missing and 8 partials ⚠️
src/inspector/node_string.cc 60.00% 13 Missing and 1 partial ⚠️
src/inspector/tracing_agent.cc 61.53% 4 Missing and 1 partial ⚠️
src/inspector_agent.cc 80.76% 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      191883   191946      +63     
  Branches    36941    36939       -2     
==========================================
+ Hits       171196   171216      +20     
- Misses      13536    13560      +24     
- Partials     7151     7170      +19     
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.77% <80.76%> (-0.37%) ⬇️
src/inspector/node_string.cc 52.05% <60.00%> (-3.12%) ⬇️
src/inspector/node_json_protocol.cc 55.00% <55.00%> (ø)

... and 27 files with indirect coverage changes

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. request-ci Add this label to start a Jenkins CI on a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants