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

The generated config.gypi file should inherit from $nodedir/config.gypi instead of process.config #2490

Closed
zcbenz opened this issue Aug 30, 2021 · 1 comment · Fixed by #2497

Comments

@zcbenz
Copy link
Contributor

zcbenz commented Aug 30, 2021

Currently the variables field of the generated config.gypi file inherits from process.config:

node-gyp/lib/configure.js

Lines 101 to 103 in 5f1a06c

var config = process.config ? JSON.parse(JSON.stringify(process.config)) : {}
var defaults = config.target_defaults
var variables = config.variables

And the target_defaults field does inherit from process.config for the reason that the build configurations of the running node executable may be different from the target being compiled for:

node-gyp/lib/configure.js

Lines 115 to 122 in 5f1a06c

// don't inherit the "defaults" from node's `process.config` object.
// doing so could cause problems in cases where the `node` executable was
// compiled on a different machine (with different lib/include paths) than
// the machine where the addon is being built to
defaults.cflags = []
defaults.defines = []
defaults.include_dirs = []
defaults.libraries = []

However the variables field also belongs to build configurations of Node.js, and should be treated the same with target_defaults, for example Node.js executable compiled with v8_enable_pointer_compression=1 can not load modules compiled with v8_enable_pointer_compression=0.

This has been causing problems when compiling modules for Electron, because node-gyp is using process.config.variables of the running node executable for compilation, which are different from the build configurations of Electron.

So the generated config.gypi file should not inherit from process.config at all, and it should inherit from the $nodedir/include/node/config.gypi file instead, which is the actual build configuration of the target.

I can start working out a pull request if the above suggested solution sounds good to the maintainers.

/cc @rvagg

@rvagg
Copy link
Member

rvagg commented Sep 2, 2021

@zcbenz I'm inclined to trust you on this and say go ahead and submit a PR and we'll see if we can figure out what the blast radius of such a change might be. Maybe it's just a semver-major to be safe if we can't find evidence of a major impact.

Any input from other @nodejs/gyp heads would be good head (tbh my mental bandwidth on this is a bit limited at the moment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants