inspector: do not depend on v8's inspector_protocol generator by aslushnikov · Pull Request #21975 · nodejs/node · GitHub

inspector_protocol is designed in a way that every instance of the generated code needs to own its copy of the inspector_protocol harness. For example, chromium code has 2 instances, one in v8 and one in chromium itself.

The reason is that otherwise incremental changes to the inspector_protocol need to happen in the interdependent projects simultaneously, which is infeasible due to the roll process.

This patch fixes the issue introduced by the Tracing agent's misuse of the v8's private protocol generator harness and establishes a direct dependency to the harness for the Node's agents.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

@ak239 can you guys please take a look?

@aslushnikov Can you update the commit message to follow the guidelines?

/cc @nodejs/v8-inspector

Also, since this brings in multiple new directories in deps/ – does this need to come with updates for tools/license-builder.sh?

+1, but please address the comments raised by @addaleax

Can you update the commit message to follow the guidelines?

@addaleax I think it's all good now. Please let me know if I'm missing something.

Also, since this brings in multiple new directories in deps/ – does this need to come with updates for tools/license-builder.sh?

I ran the tool, but it just removed seemingly unrelated licenses from the LICENSE file. I wonder if it works as expected.

@eugeneo @ak239 thanks for the review

Hello @aslushnikov and thank you for the contribution!
I think this might not be the optimal way to achieve this. Since V8 carries its own version of inspector_protocol IMHO we need to keep using that.
As for CL 1149321 AFAICT it will not affect node since we trigger inspector_protocol via GYP and have all it's files as dependencies.

node/node.gyp

Lines 1055 to 1079 in 83474ae

    
           'node_protocol_files': [ 
        
             '<(protocol_path)/lib/Allocator_h.template', 
        
             '<(protocol_path)/lib/Array_h.template', 
        
             '<(protocol_path)/lib/Collections_h.template', 
        
             '<(protocol_path)/lib/DispatcherBase_cpp.template', 
        
             '<(protocol_path)/lib/DispatcherBase_h.template', 
        
             '<(protocol_path)/lib/ErrorSupport_cpp.template', 
        
             '<(protocol_path)/lib/ErrorSupport_h.template', 
        
             '<(protocol_path)/lib/Forward_h.template', 
        
             '<(protocol_path)/lib/FrontendChannel_h.template', 
        
             '<(protocol_path)/lib/Maybe_h.template', 
        
             '<(protocol_path)/lib/Object_cpp.template', 
        
             '<(protocol_path)/lib/Object_h.template', 
        
             '<(protocol_path)/lib/Parser_cpp.template', 
        
             '<(protocol_path)/lib/Parser_h.template', 
        
             '<(protocol_path)/lib/Protocol_cpp.template', 
        
             '<(protocol_path)/lib/ValueConversions_h.template', 
        
             '<(protocol_path)/lib/Values_cpp.template', 
        
             '<(protocol_path)/lib/Values_h.template', 
        
             '<(protocol_path)/templates/Exported_h.template', 
        
             '<(protocol_path)/templates/Imported_h.template', 
        
             '<(protocol_path)/templates/TypeBuilder_cpp.template', 
        
             '<(protocol_path)/templates/TypeBuilder_h.template', 
        
             '<(protocol_path)/CodeGenerator.py', 
        
           ]

I think you can solve the fail on the V8 buildbot by updating the GN that is used to build node, and make sure all the templates are dependencies (or force a rerun on every build).

/CC @nodejs/build-files @nodejs/v8-update

Also, since this brings in multiple new directories in deps/ – does this need to come with updates for tools/license-builder.sh?

I ran the tool, but it just removed seemingly unrelated licenses from the LICENSE file. I wonder if it works as expected.

@nodejs/collaborators I’m not sure who’d be best pinged for this question, so, sorry to bother all of you.

@refack,
I believe that we need separate copy of inspector_protocol. We can consider example Chromium as another example of V8 embedder. We introduced separate copy there a long time ago and it allows us to make changes inside inspector_protocol much easier.
Imagine that we would like to make some inspector_protocol change. We first landed it into inspector_protocol repository. As next step we can separately rolled it for all protocol users with all required changes. Without separate copy we need to roll change into V8 in a manner that would not break node.js usage so most likely we need 3 steps every time: roll to V8 some intermediate protocol state, roll V8 to node, migrate node to new version, implement final solution for protocol, roll to V8, roll V8 to node. With separate copy we can just land new feature to inspector_protocol and then roll it to V8 and node by two patches without non trivial intermediate step.

Let's separate two concerns.

The codegen tool
The templates

There should be only one instance of the tool in the code base.
As for the templates, if the changes are incompatible, won't they break the code?

@refack divergent templates will not break the code. Node Inspector calls into V8 inspector through the stable API and does not directly interact with the code generated with those templates.

cc @hashseed

there's also more context about this in v8#76.

@aslushnikov So, currently the license-builder.sh file is slightly broken in that it includes entries for three things that don't ship with our source code and therefore don't need to be included. But you can ignore those errors. I've opened a PR to fix it. (#21979)

What needs to happen in this PR is an entry needs to be added for the dependency or dependencies added here, and then the script should be run and the license file updated as part of the PR.

Hope that helps!

@refack divergent templates will not break the code. Node Inspector calls into V8 inspector through the stable API and does not directly interact with the code generated with those templates.

@eugeneo So what does break and why?

This should not land as a dep it should be in tools.

@refack I updated the description; hopefully it now does a better job explaining what's going on.

@refack I believe this PR is trying to address Node build breaking when V8 moves to a newer version of the inspector_protocol generator. There is no reason to require them to use same generator and templates.

@aslushnikov I am unable to build it locally even with a clean build:
make[1]: *** No rule to make target '../deps/v8/src/debug/mirrors.js', needed by '/usr/local/google/home/eostroukhov/Development/node/out/Release/obj/gen/libraries.cc'. Stop.

What needs to happen in this PR is an entry needs to be added for the dependency or dependencies added here, and then the script should be run and the license file updated as part of the PR.

@Trott thanks, I see what's going on. Once your PR lands, I'll rebaseline atop; I included your changes here meanwhile.

This should not land as a dep it should be in tools.

@refack done.

@aslushnikov I am unable to build it locally even with a clean build:
make[1]: *** No rule to make target '../deps/v8/src/debug/mirrors.js', needed by '/usr/local/google/home/eostroukhov/Development/node/out/Release/obj/gen/libraries.cc'. Stop.

@eugeneo that's right; I was compiling against v8/node instead of nodejs/node. I updated the PR to include an older version of the generators that is currently used in the node.js.

Haven't reviewed yet, but I'm generally +1 on this.

I've recently been bitten by this when a new inspector_protocol rolled into V8. I scrambled to update V8/node to make sure our CI for Node.js didn't break. Decoupling Node.js from V8 in this regard makes sense.

Consider this the same as with ICU.

The fix works for me, I can now build locally and "make tests' passes! 👍

@refack, your concern about deps/ vs. tools/ directory has been addressed. Can you dismiss your request for changes?

This PR is blocking us from rolling new inspector_protocol version to V8.
@refack could you please take a look and dismiss your request for changes or describe what additional changes are required?

The other license-builder PR has landed so this one can be rebased. We'll need a CI run once that happens.

Currently, node.js depends on [inspector_protocol](https://www.google.com/url?q=https://chromium.googlesource.com/deps/inspector_protocol/) indirectly through the dependency on v8. This is a dependency violation that will make it hard to roll V8 into Node if V8 gets a newer inspector protocol version with incompatible API. In fact, this surfaced on one of our bots when we tried to roll new inspector_protocol into V8: [upstream patch](https://chromium-review.googlesource.com/c/v8/v8/+/1149321). This patch adds inspector protocol and its required dependencies to node deps: - jinja2 - markupsafe

I agree, @refack’s change request seems to have been addressed.

(That being said, I was surprised by it, I can’t find an explanation for it in this thread and, quite frankly, I disagree with it – dependencies that influence directly what goes into the node binary should be in deps/, imho. I didn’t want to make any fuzz about it and I’m fine with opening a PR to move it back myself after this lands.)

CI once again, failures on existing one looks unrelated: https://ci.nodejs.org/job/node-test-pull-request/16326/

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16329/

It looks like latest CI finished successfully. I will merge this PR later today.

Based on development process, I can see that requested changes have been made and can dismiss request changes in this case.

Landed in e039524

Currently, node.js depends on inspector_protocol indirectly through the dependency on v8. This is a dependency violation that will make it hard to roll V8 into Node if V8 gets a newer inspector protocol version with incompatible API. In fact, this surfaced on one of our bots when we tried to roll new inspector_protocol into V8. This patch adds inspector protocol and its required dependencies to node deps: - jinja2 - markupsafe PR-URL: #21975 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Aleksei Koziatinskii <ak239spb@gmail.com>

Currently, node.js depends on inspector_protocol indirectly through the dependency on v8. This is a dependency violation that will make it hard to roll V8 into Node if V8 gets a newer inspector protocol version with incompatible API. In fact, this surfaced on one of our bots when we tried to roll new inspector_protocol into V8. This patch adds inspector protocol and its required dependencies to node deps: - jinja2 - markupsafe PR-URL: nodejs#21975 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Aleksei Koziatinskii <ak239spb@gmail.com>

Dependencies that influence directly what goes into the `node` binary should be in `deps/`. Refs: nodejs#21975 (comment)

Currently, node.js depends on inspector_protocol indirectly through the dependency on v8. This is a dependency violation that will make it hard to roll V8 into Node if V8 gets a newer inspector protocol version with incompatible API. In fact, this surfaced on one of our bots when we tried to roll new inspector_protocol into V8. This patch adds inspector protocol and its required dependencies to node deps: - jinja2 - markupsafe PR-URL: nodejs/node#21975 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Aleksei Koziatinskii <ak239spb@gmail.com>

addaleax added the inspector Issues and PRs related to the V8 inspector protocol label Jul 25, 2018

eugeneo approved these changes Jul 25, 2018

View reviewed changes

alexkozy approved these changes Jul 25, 2018

View reviewed changes

refack added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. tools Issues and PRs related to the tools directory. labels Jul 25, 2018

refack previously requested changes Jul 25, 2018

View reviewed changes

aslushnikov changed the title inspector: bring inspector_protocol as a direct dependency inspector: do not depend on v8's inspector_protocol generator Jul 25, 2018

aslushnikov force-pushed the node-fix-deps-violation branch from d98e288 to 1ece530 Compare August 6, 2018 20:27

alexkozy closed this Aug 11, 2018

rvagg mentioned this pull request Aug 13, 2018

Release proposal: v10.9.0 #22295

Merged

aslushnikov mentioned this pull request Aug 13, 2018

inspector: bring in inspector_protocol as a direct dependency v8/node#76

Closed

4 tasks

refack mentioned this pull request Aug 17, 2018

tools: remove gyp from tools directory #22343

Closed

2 tasks

addaleax added a commit to addaleax/node that referenced this pull request Aug 20, 2018

deps: move inspector_protocol to deps

b661b5f

Dependencies that influence directly what goes into the `node` binary should be in `deps/`. Refs: nodejs#21975 (comment)

addaleax mentioned this pull request Aug 20, 2018

deps: move inspector_protocol to deps #22411

Closed

2 tasks

refack mentioned this pull request Sep 4, 2018

deps: update V8 to 6.9 #21983

Closed

ravishivt mentioned this pull request Sep 28, 2018

Inspect - sources aren't visible in Node v10.9.0 #23152

Closed

kapouer mentioned this pull request Sep 30, 2018

multiple copies of markupsafe, jinja2, inspector_protocol #23171

Closed

inspector: do not depend on v8's inspector_protocol generator by aslushnikov · Pull Request #21975 · nodejs/node · GitHub

Uh oh!

Uh oh!

inspector: do not depend on v8's inspector_protocol generator - #21975

inspector: do not depend on v8's inspector_protocol generator#21975aslushnikov wants to merge 1 commit intonodejs:masternodejs/node:masterfrom aslushnikov:node-fix-deps-violationaslushnikov/node:node-fix-deps-violationCopy head branch name to clipboard

Conversation

aslushnikov commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

aslushnikov commented Jul 25, 2018

Uh oh!

addaleax commented Jul 25, 2018

Uh oh!

addaleax commented Jul 25, 2018

Uh oh!

eugeneo left a comment

Choose a reason for hiding this comment

Uh oh!

aslushnikov commented Jul 25, 2018

Uh oh!

refack commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Jul 25, 2018

Uh oh!

alexkozy commented Jul 25, 2018

Uh oh!

refack commented Jul 25, 2018

Uh oh!

eugeneo commented Jul 25, 2018

Uh oh!

aslushnikov commented Jul 25, 2018

Uh oh!

Trott commented Jul 25, 2018

Uh oh!

refack commented Jul 25, 2018

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

aslushnikov commented Jul 25, 2018

Uh oh!

eugeneo commented Jul 26, 2018

Uh oh!

aslushnikov commented Jul 26, 2018

Uh oh!

hashseed commented Jul 26, 2018

Uh oh!

eugeneo commented Jul 26, 2018

Uh oh!

ofrobots commented Jul 28, 2018

Uh oh!

alexkozy commented Aug 5, 2018

Uh oh!

Trott commented Aug 5, 2018

Uh oh!

addaleax commented Aug 9, 2018

Uh oh!

alexkozy commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Aug 10, 2018

Uh oh!

alexkozy commented Aug 10, 2018

Uh oh!

alexkozy commented Aug 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

inspector: do not depend on v8's inspector_protocol generator#21975
aslushnikov wants to merge 1 commit into
nodejs:masterfrom
aslushnikov:node-fix-deps-violation

aslushnikov commented Jul 25, 2018 •
edited
Loading

refack commented Jul 25, 2018 •
edited
Loading

alexkozy commented Aug 10, 2018 •
edited
Loading