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: do not depend on v8's inspector_protocol generator #21975

Closed

Conversation

aslushnikov
Copy link
Contributor

@aslushnikov aslushnikov commented Jul 25, 2018

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
Copy link
Contributor Author

@ak239 can you guys please take a look?

@addaleax
Copy link
Member

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

/cc @nodejs/v8-inspector

@addaleax
Copy link
Member

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

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

@eugeneo eugeneo left a comment

Choose a reason for hiding this comment

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

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

@aslushnikov
Copy link
Contributor Author

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
Copy link
Contributor

refack commented Jul 25, 2018

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

@refack 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
@addaleax
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor

refack commented Jul 25, 2018

Let's separate two concerns.

  1. The codegen tool
  2. 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
Copy link
Contributor

eugeneo commented Jul 25, 2018

@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
Copy link
Contributor Author

cc @hashseed

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

@Trott
Copy link
Member

Trott commented Jul 25, 2018

@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
Copy link
Contributor

refack commented Jul 25, 2018

@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
refack previously requested changes Jul 25, 2018
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

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

@aslushnikov 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
Copy link
Contributor Author

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

@eugeneo
Copy link
Contributor

eugeneo commented Jul 26, 2018

@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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

eugeneo commented Jul 26, 2018

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

@ofrobots
Copy link
Contributor

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

@alexkozy
Copy link
Member

alexkozy commented Aug 5, 2018

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
Copy link
Member

Trott commented Aug 5, 2018

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
Copy link
Contributor

ofrobots commented Aug 9, 2018

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
Copy link
Member

addaleax commented Aug 9, 2018

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
Copy link
Member

alexkozy commented Aug 10, 2018

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

@Trott
Copy link
Member

Trott commented Aug 10, 2018

@alexkozy
Copy link
Member

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

@alexkozy alexkozy dismissed refack’s stale review August 11, 2018 00:54

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

@alexkozy
Copy link
Member

Landed in e039524

alexkozy pushed a commit that referenced this pull request Aug 11, 2018
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 <[email protected]>
Reviewed-By: Aleksei Koziatinskii <[email protected]>
@alexkozy alexkozy closed this Aug 11, 2018
targos pushed a commit that referenced this pull request Aug 11, 2018
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 <[email protected]>
Reviewed-By: Aleksei Koziatinskii <[email protected]>
hashseed pushed a commit to v8/node that referenced this pull request Aug 17, 2018
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 <[email protected]>
Reviewed-By: Aleksei Koziatinskii <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Aug 20, 2018
Dependencies that influence directly what goes into the `node` binary
should be in `deps/`.

Refs: nodejs#21975 (comment)
@refack refack mentioned this pull request Sep 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. inspector Issues and PRs related to the V8 inspector protocol tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants