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

aslushnikov · 2018-07-25T21:00:31Z

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

aslushnikov · 2018-07-25T21:01:07Z

@ak239 can you guys please take a look?

addaleax · 2018-07-25T21:02:41Z

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

/cc @nodejs/v8-inspector

addaleax · 2018-07-25T21:05:04Z

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

eugeneo

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

aslushnikov · 2018-07-25T21:33:36Z

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

refack · 2018-07-25T21:34:31Z

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

addaleax · 2018-07-25T21:38:39Z

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.

alexkozy · 2018-07-25T22:11:27Z

@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.

refack · 2018-07-25T22:48:13Z

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?

eugeneo · 2018-07-25T22:59:32Z

@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.

aslushnikov · 2018-07-25T23:01:47Z

cc @hashseed

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

Trott · 2018-07-25T23:11:07Z

@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 · 2018-07-25T23:44:15Z

@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?

refack

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

aslushnikov · 2018-07-25T23:59:08Z

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

eugeneo · 2018-07-26T00:01:14Z

@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.

aslushnikov · 2018-07-26T01:16:07Z

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.

hashseed · 2018-07-26T05:24:03Z

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.

eugeneo · 2018-07-26T17:56:58Z

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

ofrobots · 2018-07-28T00:49:07Z

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

alexkozy · 2018-08-05T01:02:36Z

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?

Trott · 2018-08-05T02:23:49Z

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

ofrobots · 2018-08-09T20:30:44Z

The development process states:

If any Collaborator objects to a change without giving any additional explanation or context, and the objecting Collaborator fails to respond to explicit requests for explanation or context within a reasonable period of time, the objection may be dismissed. Note that this does not apply to objections that are explained.

While this doesn't 100% apply as @refack did explain his objection. AFAICT the objection has been addressed. A reasonable amount of time (15 days) has elapsed to allow @refack to acknowledge and/or leave further comments. IMO it would be reasonable to dismiss objection at this point.

addaleax · 2018-08-09T20:37:03Z

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.)

alexkozy · 2018-08-10T02:06:29Z

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

Trott · 2018-08-10T04:23:57Z

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

alexkozy · 2018-08-10T20:16:49Z

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.

alexkozy · 2018-08-11T01:06:32Z

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)

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aslushnikov commented Jul 25, 2018 •

edited

Loading

aslushnikov commented Jul 25, 2018

addaleax commented Jul 25, 2018

addaleax commented Jul 25, 2018

eugeneo left a comment

aslushnikov commented Jul 25, 2018

refack commented Jul 25, 2018 •

edited

Loading

addaleax commented Jul 25, 2018

alexkozy commented Jul 25, 2018

refack commented Jul 25, 2018

eugeneo commented Jul 25, 2018

aslushnikov commented Jul 25, 2018

Trott commented Jul 25, 2018

refack commented Jul 25, 2018

refack left a comment

aslushnikov commented Jul 25, 2018

eugeneo commented Jul 26, 2018

aslushnikov commented Jul 26, 2018

hashseed commented Jul 26, 2018

eugeneo commented Jul 26, 2018

ofrobots commented Jul 28, 2018

alexkozy commented Aug 5, 2018

Trott commented Aug 5, 2018

ofrobots commented Aug 9, 2018

addaleax commented Aug 9, 2018

alexkozy commented Aug 10, 2018 •

edited

Loading

Trott commented Aug 10, 2018

alexkozy commented Aug 10, 2018

alexkozy commented Aug 11, 2018

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

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

Conversation

aslushnikov commented Jul 25, 2018 • edited Loading

Checklist

aslushnikov commented Jul 25, 2018

addaleax commented Jul 25, 2018

addaleax commented Jul 25, 2018

eugeneo left a comment

Choose a reason for hiding this comment

aslushnikov commented Jul 25, 2018

refack commented Jul 25, 2018 • edited Loading

addaleax commented Jul 25, 2018

alexkozy commented Jul 25, 2018

refack commented Jul 25, 2018

eugeneo commented Jul 25, 2018

aslushnikov commented Jul 25, 2018

Trott commented Jul 25, 2018

refack commented Jul 25, 2018

refack left a comment

Choose a reason for hiding this comment

aslushnikov commented Jul 25, 2018

eugeneo commented Jul 26, 2018

aslushnikov commented Jul 26, 2018

hashseed commented Jul 26, 2018

eugeneo commented Jul 26, 2018

ofrobots commented Jul 28, 2018

alexkozy commented Aug 5, 2018

Trott commented Aug 5, 2018

ofrobots commented Aug 9, 2018

addaleax commented Aug 9, 2018

alexkozy commented Aug 10, 2018 • edited Loading

Trott commented Aug 10, 2018

alexkozy commented Aug 10, 2018

alexkozy commented Aug 11, 2018

aslushnikov commented Jul 25, 2018 •

edited

Loading

refack commented Jul 25, 2018 •

edited

Loading

alexkozy commented Aug 10, 2018 •

edited

Loading