GN: Build targets node_lib and inspector have inconsistent defines
Opened this issue · 4 comments
The build target node_lib in BUILD.gn uses defines that are defined in the target itself and in the config node_lib_config. The target inspector in src/inspector/BUILD.gn shares some of these defines, but not all.
That can cause the node::Environment object as defined in env.h to have different layout in src/inspector/tracing_agent.cc than in the rest of Node.js. This could cause worker tests to fail.
See https://gist.github.com/hashseed/8ee8fe7a5c491cff13c62292ae298fcc and https://github.com/hashseed/gn-node
The right way would be to move all defines into node_lib_config and use that both in the node_lib and the inspector targets.
Here is the example fix in my fork of the GN files: hashseed/gn-node@d4cf240
I think your fix pulls too much into the common config; e.g. libs don't need to be in node_lib_config because they're automatically propagated by GN, and cflags shouldn't be propagated to dependent targets (usually). Since node_lib_config is listed as a public config, it should only include configuration that must be present in targets that depend on node. Internal-only #defines such as BUILDING_V8_SHARED etc. should not be exposed to consumers of the target.
I wasn't sure about NODE_{ARCH,PLATFORM,TAG,V8_OPTIONS,RELEASE_URLBASE,OPENSSL_SYSTEM_CERT_PATH}. Are they considered part of node's public C++ API?
Here's my take: #93. I split out just the #defines into :node_internal_config, used visibility to restrict that configuration to only internal targets, and kept the public-facing #defines in node_lib_config (which remains exposed to dependents via public_configs).
That's a good point. So far I have focused mostly on getting things to build and pass tests (which they now do) and not so much on clean configs.
Yeah. You only really need NODE_WANT_INTERNALS=1.