inspector: do not depend on v8's inspector_protocol generator

[aslushnikov]

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]

@ak239 can you guys please take a look?

[addaleax]

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

/cc @nodejs/v8-inspector

[addaleax]

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]

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]

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]

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]

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

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]

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

cc @hashseed

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

[Trott]

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

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

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

[eugeneo]

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

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]

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]

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

[ofrobots]

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

[alexkozy]

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]

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

[addaleax]

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]

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

[Trott]

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

[alexkozy]

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]

Landed in e039524