deps: move inspector_protocol to deps

[addaleax]

Dependencies that influence directly what goes into the node binary
should be in deps/.

Refs: #21975 (comment)

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[nodejs-github-bot]

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/600/pipeline

[addaleax]

Regarding the fixup commit here … it seems interesting that Travis CI caught something that our own test-commit-linuxone didn’t …? 🤔

[refack]

This is not code this is a code generation tool.
The code this tool uses is in https://github.com/nodejs/node/tree/master/src/inspector
Ref: https://chromium.googlesource.com/deps/inspector_protocol/
Ref: #21975 (review)

[refack]

IMO this is equivalent to

• GYP - https://github.com/nodejs/node/tree/master/tools/gyp
• ICU tools - https://github.com/nodejs/node/tree/master/tools/icu
• MSVS helper - https://github.com/nodejs/node/tree/master/tools/msvs

(jinja2 and markupsafe were part of the original change, but I understand they are more obvious and were kept out of this PR).

Just to be clear I'm -1 on this change, and request that is not dismissed (unless overridden by a TSC vote).

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

@addaleax Since you brought it up, I found your comment #21975 (comment) confusing and even a little bit disrespectful. If you had critique on my comment you should brought it up then and there.

To all those to approved, I'd like to remind you of the policy:
https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#code-reviews:

All pull requests must be reviewed and accepted by a Collaborator with sufficient expertise who is able to take full responsibility for the change.

[refack]

As a point of reference, these are the generated files that are actually compiled into the binary:

node/node.gyp

Lines 1049 to 1055 in 9bcb744

	'node_inspector_generated_sources': [
	'<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/Forward.h',
	'<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/Protocol.cpp',
	'<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/Protocol.h',
	'<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/NodeTracing.cpp',
	'<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/NodeTracing.h',
	],

node/node.gyp

Lines 493 to 510 in 9bcb744

	'sources': [
	'src/inspector_agent.cc',
	'src/inspector_io.cc',
	'src/inspector_js_api.cc',
	'src/inspector_socket.cc',
	'src/inspector_socket_server.cc',
	'src/inspector/main_thread_interface.cc',
	'src/inspector/node_string.cc',
	'src/inspector/tracing_agent.cc',
	'src/inspector_agent.h',
	'src/inspector_io.h',
	'src/inspector_socket.h',
	'src/inspector_socket_server.h',
	'src/inspector/main_thread_interface.h',
	'src/inspector/node_string.h',
	'src/inspector/tracing_agent.h',
	'<@(node_inspector_generated_sources)'
	],

[refack]

Regarding the fixup commit here … it seems interesting that Travis CI caught something that our own test-commit-linuxone didn’t …? 🤔

Seems like we have jinja2 installed globally on some CI workers.

[joyeecheung]

@refack The code generated by inspector_protocol are primarily based on the templates that are also under that directory, and the generation is driven by python/gyp...so personally I think it is closer to deps in the deps-tools spectrum?

(Also on a side note, do we need to bump the V8 embedder suffix if we update inspector_protocol? If we do, then I believe it brings this closer to the deps end of the spectrum)

[refack]

@refack The code generated by inspector_protocol are primarily based on the templates that are also under that directory, and the generation is driven by python/gyp...so personally I think it is closer to deps in the deps-tools spectrum?

(Also on a side note, do we need to bump the V8 embedder suffix if we update inspector_protocol? If we do, then I believe it brings this closer to the deps end of the spectrum)

As mentioned in #21975 this copy of the tool is node's own. It is meant to be independent of the one in https://github.com/nodejs/node/tree/master/deps/v8/third_party/inspector_protocol. So no need to change v8_embedder_string whatsoever. In fact if you look at the neighbors in the V8 tree - https://github.com/nodejs/node/tree/master/deps/v8/third_party they are all more obvious tools.
When I considered this as dependency it brought up the major problem of us simultaneously using two versions of this dependence. The authors of #21975 convinced me that this is just a tool, and it's the code that we write that is the most significant bits, and hence it's reasonable to have two versions in the same code base.

[refack]

BTW if the name is confusing. This actually has very little to do with the debugging inspector. We use it to generate the code used to communicate with V8 to get VM tracing information.

[joyeecheung]

BTW if the name is confusing. This actually has very little to do with the debugging inspector. We use it to generate the code used to communicate with V8 to get VM tracing information.

Ah, thanks, didn't know that!

As mentioned in #21975 this copy of the tool is node's own. It is meant to be independent of the one in https://github.com/nodejs/node/tree/master/deps/v8/third_party/inspector_protocol. So no need to change v8_embedder_string whatsoever.

If there is a bug in the code generated by tools/inspector_protocol that is fixed in v8/third_party/inspector_protocol, and we release two versions of Node with each version (because they have compatible APIs), wouldn't that result in two versions of Node with the same version of V8 but different behavior of the trace events? Would that be acceptable for the consumers of trace events?

[kfarnung]

@refack The generated code isn't used to communicate with V8, it's used to define an object model that represents the CrDP interface. It's up to the implementer of the generated classes (in this case node) to interact with the system and perform the requested action. It's there to act as a bridge between the debugger requests (e.g. DevTools, VS Code, etc.) and the application code that can service the requests.

The name inspector_protocol seems fine to me, the code takes in a protocol description and generates all of the plumbing required to take in a JSON-formatted request, forward it to application-defined handlers, and turn the output back into a JSON-formatted response. Since the common API surface is just JSON-formatted UTF-16 strings, there should be little to no risk of breaking API changes. Each instance of the generated code stands completely on its own.

[refack]

Thank you @kfarnung for the explanation.

Given that #21975 established that node needs it's own version, back to the point of this PR, is inspector_protocol a tool, or is it a dependency library?
I agree that it is a question of semantics, but in this case IMHO it's important to make this distinction. If we decide that it's a dependency library, then we must accept that we embed two (different) versions of this library. I think that is a situation that we should avoid, and refactor out.

[eugeneo]

inspector_protocol is a tool - it is a Python codegenerator and a collection of templates.

[addaleax]

Okay everyone, I’m closing this … this isn’t that important, plus I talked to @refack and he has some ideas around an altogether better approach for the particular issue that the “extra” dependencies solve.