This repository was archived by the owner on Mar 4, 2020. It is now read-only.
Repository navigation
This repository was archived by the owner on Mar 4, 2020. It is now read-only.
GN: Build targets node_lib and inspector have inconsistent defines #91
Description
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
Metadata
Metadata
Assignees
Labels
No labels
Activity
hashseed commentedon Jan 30, 2019
@nornagon
nornagon commentedon Feb 4, 2019
I think your fix pulls too much into the common config; e.g.
libsdon't need to be innode_lib_configbecause they're automatically propagated by GN, andcflagsshouldn't be propagated to dependent targets (usually). Sincenode_lib_configis listed as a public config, it should only include configuration that must be present in targets that depend on node. Internal-only#defines such asBUILDING_V8_SHAREDetc. 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, usedvisibilityto restrict that configuration to only internal targets, and kept the public-facing#defines innode_lib_config(which remains exposed to dependents viapublic_configs).hashseed commentedon Feb 4, 2019
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.
hashseed commentedon Feb 5, 2019
Yeah. You only really need
NODE_WANT_INTERNALS=1.