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

feat: build with config.gypi from node headers #2497

Merged
merged 1 commit into from
Nov 5, 2021

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Sep 14, 2021

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

Close #2490.

This PR makes the generated config.gypi file inherit from $nodedir/include/node/config.gypi instead of runtime's process.config object when --nodedir or --dist-url is passed. There is also a --force-process-config flag added to force switching back to the old behavior. For the reason behind this change, please check #2490.

Affects of change

This change should have not affect to Node.js users, since they usually don't use either --nodedir or --dist-url. And even when they explicitly do so, since the process.config is generated from the $nodedir/include/node/config.gypi, the build configurations will be exactly the same.

For users building modules for Electron, this is a breaking change because old versions of Electron are shipping a malformed config.gypi in its headers distribution, and users have to pass --force-process-config to node-gyp to correctly build modules.

For NW.js users I believe nothing is changed as they are using a custom fork of node-gyp to build modules.

--nodedir and --dist-url

Note that $nodedir/include/node/config.gypi is only used when --nodedir or --dist-url are passed.

There are 3 kinds of node-gyp use cases:

  1. Building modules for official Node.js binaries with official headers.
  2. Building modules for third party Node.js binaries with official headers.
  3. Building modules for custom Node.js binaries or Node.js-embedded apps with custom headers.

For 1, using either $nodedir/include/node/config.gypi or process.config is fine since they are the same thing.

For 2, using $nodedir/include/node/config.gypi can cause problems, because some third party Node.js distributions are using different build configurations from the official headers.

For example, some Node.js distributions provided in Linux and BSD distributions are using shared libuv and openssl library, and building with $nodedir/include/node/config.gypi would cause problems for them because the $nodedir/include/node/config.gypi file in official headers distribution assumes bundled libuv and openssl.

This is actually a malformed use case because there is no guarantee that official headers can build with custom process.config settings, and it has been causing problems like sass/node-sass#2775. Third party Node.js distributions should build modules with their own headers instead of the official headers, there is no way for Node.js and node-gyp to provide an official build configuration that can work for all kinds of third party Node.js variants. But for now I think we should just make things compatible.

For 3, users have to build modules with $nodedir/include/node/config.gypi instead of process.config, because only the former includes the correct build configurations of the target to build for. Electron has been patching Node.js to ignore the wrong build configurations used in node-gyp, and NW.js is just forking node-gyp.

So making node-gyp use $nodedir/include/node/config.gypi when --nodedir or --dist-url is passed is a natural choice, because passing the flags means building with custom headers, and it implies that the build configurations of the custom target should be used.

And when --nodedir or --dist-url is not passed, node-gyp should just keep the old behavior so it won't break the use case 2.

Why this is needed

Without this change node-gyp always builds modules with the build configurations of the running Node instance, and custom Node distributions like Electron and NW.js have been forking either Node.js or node-gyp to be able to build modules.

By making node-gyp use the config.gypi in node headers, embedders will be able to get rid of their patches, and the ecosystem will have less variants.

/cc @rvagg @nodejs/embedders

@zcbenz zcbenz force-pushed the read-config-gypi branch 7 times, most recently from 8384907 to d1b2c9c Compare September 14, 2021 11:04
@cclauss
Copy link
Contributor

cclauss commented Oct 5, 2021

Given that node-sass is deprecated, can we take it out of the equation?

@cclauss cclauss added the Node Sass --> Dart Sass https://github.com/sass/node-sass/issues/2952 label Oct 5, 2021
@zcbenz
Copy link
Contributor Author

zcbenz commented Oct 6, 2021

Yeah node-sass is deprecated but the problem is still there. For example this FreeBSD bug: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=220983, the stock Node.js binary provided in FreeBSD basically can not work with any C++ native modules.

For the FreeBSD bug, the eventual solution would require them to provide their own node headers distributions (same with what Electron and NW.js have been doing), and node-gyp needs this change to fully support that use case, otherwise there will be more downstream patches on Node.js and node-gyp.

@rvagg rvagg mentioned this pull request Oct 8, 2021
@targos targos removed their request for review October 11, 2021 09:34
@targos

This comment has been minimized.

@gengjiawen

This comment has been minimized.

@gengjiawen
Copy link
Member

I think the pointer stands and we have flags to control. LGTM.

@rvagg Second opinion ?

lib/create-config-gypi.js Outdated Show resolved Hide resolved
lib/create-config-gypi.js Outdated Show resolved Hide resolved
// 1. string comments
config = config.replace(/#.*/g, '')
// 2. join multiline strings
config = config.replace(/'$\s+'/mg, '')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have any examples of this in config.gypi? I'm looking but can't see any, maybe this is safe to include but it's a bit of an uncomfortable regex that would be nice to see tested against an example (even if just manually)

Copy link
Contributor Author

@zcbenz zcbenz Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test was added for this in test/test-create-config-gypi.js:

  const str = "# Some comments\n{'variables': {'multiline': 'A'\n'B'}}"
  const config = parseConfigGypi(str)
  t.deepEqual(config, { variables: { multiline: 'AB' } })

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I suppose we can give it a go! it at least seems to preserve existing behaviour, only adding something new

@richardlau
Copy link
Member

This might fix #2534.

@zcbenz
Copy link
Contributor Author

zcbenz commented Nov 1, 2021

This might fix #2534.

This PR won't fix #2534 directly since the target build config is only used when --nodedir or --dist-url is passed.

@rvagg rvagg merged commit a27dc08 into nodejs:master Nov 5, 2021
RaisinTen added a commit to RaisinTen/electron that referenced this pull request Apr 5, 2023
In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
RaisinTen added a commit to RaisinTen/electron that referenced this pull request Apr 10, 2023
In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
codebytere pushed a commit to electron/electron that referenced this pull request Apr 11, 2023
fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
trop bot added a commit to electron/electron that referenced this pull request Apr 11, 2023
In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

Co-authored-by: Darshan Sen <raisinten@gmail.com>
trop bot added a commit to electron/electron that referenced this pull request Apr 11, 2023
In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

Co-authored-by: Darshan Sen <raisinten@gmail.com>
VerteDinde pushed a commit to electron/electron that referenced this pull request Apr 11, 2023
fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Darshan Sen <raisinten@gmail.com>
codebytere pushed a commit to electron/electron that referenced this pull request Apr 11, 2023
fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Darshan Sen <raisinten@gmail.com>
RaisinTen added a commit to RaisinTen/electron that referenced this pull request Apr 12, 2023
fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
RaisinTen added a commit to RaisinTen/electron that referenced this pull request Apr 12, 2023
fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
codebytere pushed a commit to electron/electron that referenced this pull request Apr 12, 2023
fix: recommended `node-gyp` version in `node.h` error (#37829)

fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
codebytere pushed a commit to electron/electron that referenced this pull request Apr 12, 2023
fix: recommended `node-gyp` version in `node.h` error (#37829)

fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Node Sass --> Dart Sass https://github.com/sass/node-sass/issues/2952
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The generated config.gypi file should inherit from $nodedir/config.gypi instead of process.config
6 participants