Add `target_modules_version` variable into generated config.gypi file #855

pmed · 2016-01-07T13:18:16Z

It may be handy to have multiple native addon binaries for several Node.js versions (i.e. 0.12, 4.X, 5.X). Node.js has a value for the native modules version as process.versions.modules.

A particular C++ addon can be loaded in JavaScript depending on Node.js version like this:

  // loads 'addon_win32_x64_m47.node' for Node.js 5.0 on Windows x64
  // loads 'addon_linux_x64_m46.node' for Node.js 4.X on Linux x64
  var native = require('addon' + process.platform + '_'
              + process.arch + '_m' + process.versions.modules + '.node');

The target binary file name should be set with product_name directive in binding.gyp file in a such way:

  {
    'targets': [
      {
        'product_name': 'addon_<(target_platform)_<(target_arch)_m<(target_modules_version)'
        ... # rest of GYP directives
      }
    ]
  }

where target_platform is a configuration variable with value from Node.js process.platform property.

But the process.versions.modules value can't be used for target_modules_version since node-gyp can build targets for different Node.js versions specified with a --target command-line option.

To resolve this issue, a function getModulesVersion() was added on configure step to get a value of NODE_MODULE_VERSION defined in node_version.h file in a supplied directory with Node.js development files.

bnoordhuis · 2016-01-07T15:00:33Z

lib/configure.js

+
+    for (var i = 0; i < sources.length; ++i) {
+      try {
+        var source = fs.readFileSync(sources[i], { encoding: 'utf8'})


The try/catch should only be around this statement. Style nit: space before }.

bnoordhuis · 2016-01-07T15:04:04Z

Can you add a couple of regression tests? You'll probably need to adjust the logic a little so you can test it without trying to read from disk (or make it use fixtures somehow.)

pmed · 2016-01-08T12:02:47Z

Reworked getModulesVersion() to read a particular file, depending on the target Node.js version.

No tests were added yet, because I don't know how to make appropriate fixtures for them.

saper · 2016-01-08T16:16:22Z

Ideally this should be provided by node itself in it's config.gypi file.

It may be handy to have multiple native addon binaries for several Node.js versions (i.e. 0.12, 4.X, 5.X). Node.js has a value for the native modules version as `process.versions.modules`. A particular C++ addon can be loaded in JavaScript depending on Node.js version like this: // loads 'addon_win32_x64_m47.node' for Node.js 5.0 on Windows x64 // loads 'addon_linux_x64_m46.node' for Node.js 4.X on Linux x64 var native = require(addon' + process.platform + '_' + process.arch + '_m' + process.versions.modules + '.node'); The target binary file name should be set with `product_name` directive in binding.gyp file in a such way: { 'targets': [ { 'product_name': 'addon_<(target_platform)_<(target_arch)_m<(target_modules_version)' ... # rest of GYP directives } ] } where `target_platform` is a configuration variable with value from Node.js `process.platform` property. But the `process.versions.modules` value can't be used for `target_modules_version` since node-gyp can build targets for different Node.js versions specified with a `--target` command-line option. To resolve this issue, a function getModulesVersion() was added on `configure` step to get a value of `NODE_MODULE_VERSION` defined in `node_version.h` file in a supplied directory with Node.js development files.

Use value of release.semver to determinate file name and location with NODE_MODULE_VERSION define, and to read this file depending on target Node.js version. Tested with Node.js versions: 0.8.4, 0.10.25, 0.11.4, 0.11.5, 0.12.0, iojs-1.0.0, iojs-2.0.0, iojs-3.0.0, 4.X, 5.X Added verbose log messages for the target_arch, target_platform, and target_modules_version variables.

Rename getModulesVersion to nodeModulesVersion function and move it out of configure() function for testing purpose. Expose nodeModulesVersion() function in `configure.test` to call it tests. Run tests for the current Node release and for all installed targets.

pmed · 2016-06-20T11:27:44Z

Renamed function to nodeModulesVersion() added tests for it.

pmed · 2016-07-16T08:54:11Z

@bnoordhuis could you review recent changes please?

bnoordhuis · 2016-07-16T09:06:45Z

lib/configure.js

+
+    // set the target modules version
+    variables.target_modules_version = nodeModulesVersion(nodeDir)
+    log.verbose('build/' + configFilename, 'modules version: ' + variables.target_modules_version)


Can you wrap lines at 80 columns? I realize there are long lines elsewhere but I'd like to avoid introducing new ones.

bnoordhuis · 2016-07-16T09:26:58Z

This needs more tests, there are quite a few cases that aren't covered yet. For example, the use case of node-gyp --nodedir /path/to/master configure (i.e., no version number in the path) should be tested because I'm quite sure that's broken now.

If you make the fs.readFileSync call pluggable, e.g. by making it an optional argument to nodeModulesVersion(), you can unit test it more easily.

pmed · 2016-07-16T09:33:05Z

@bnoordhuis thank you very much! I will think on more tests.

saper · 2016-07-17T01:58:04Z

lib/configure.js

+      'target arch: ' + variables.target_arch)
+
+    // set the target_platform variable
+    variables.target_platform = process.platform


Maybe we should allow manual override here as well? What about cross-platform builds?

bnoordhuis · 2016-07-17T07:35:17Z

needs more tests, there are quite a few cases that aren't covered yet

Something that just came to mind: building for electron / nw.js / node-webkit.

Also, it's perhaps worth pointing out that node.js v6.3.0 introduced the node_module_version gyp variable, at least partially obviating this PR.

saper · 2016-07-17T20:05:11Z

@bnoordhuis do you think that node_module_version will be backported to the older release trains?

bnoordhuis · 2016-07-18T16:57:30Z

Maybe to v4 but not to v0.10 and v0.12. Those are near their end of life anyway, though.

MylesBorins · 2016-07-18T17:34:40Z

@bnoordhuis should we open an issue on @nodejs/lts?

bnoordhuis · 2016-07-18T20:11:18Z

@thealphanerd Sure, go ahead.

saper · 2016-07-20T07:07:39Z

Having this in some final releases of 0.10 and 0.12 would be ultra cool.

Actually this patch could become a separate npm module which kicks in as a fallback if module version is not available via the gypi files.

bnoordhuis · 2016-07-20T07:17:22Z

The node_modules_version variable was introduced as a set of bigger changes that make it possible to build node as a shared library. That is not going to get back-ported but if you want to take a stab at back-porting just the variable, you're welcome.

Caveat emptor though, v0.10 and v0.12 are in maintenance mode. Normally only security fixes and critical fixes are accepted but since node_modules_version is useful for building add-ons across versions, I think we can make an exception.

Enable targetting of a different node version than the currently running one when building binary modules. Based on nodejs/node@410296c37 PR-URL: nodejs#7808 Ref: nodejs/node-gyp#855

Enable targetting of a different node version than the currently running one when building binary modules. Based on nodejs/node@410296c37 PR-URL: nodejs#8027 Ref: nodejs#7808 Ref: nodejs/node-gyp#855

Enable targetting of a different node version than the currently running one when building binary modules. Based on nodejs/node@410296c37 PR-URL: nodejs#8171 Ref: nodejs#8027 Ref: nodejs#7808 Ref: nodejs/node-gyp#855

Enable targetting of a different node version than the currently running one when building binary modules. Based on 410296c37 PR-URL: #8171 Ref: #8027 Ref: #7808 Ref: nodejs/node-gyp#855 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

bnoordhuis · 2016-10-07T09:03:10Z

@pmed Do you think this PR is still necessary?

pmed · 2016-10-07T09:54:19Z

As I understand, the variable node_module_version has been added into config.gypi in Node versions 4.x and 6.x

I think there is no more need to do the same thing in node-gyp, so let's close this PR.

bnoordhuis · 2016-10-07T10:31:42Z

Okay thanks, I'll close.

Enable targetting of a different node version than the currently running one when building binary modules. Based on 410296c37 PR-URL: #8171 Ref: #8027 Ref: #7808 Ref: nodejs/node-gyp#855 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Enable targetting of a different node version than the currently running one when building binary modules. Based on nodejs/node@410296c37 PR-URL: nodejs#8027 Ref: nodejs#7808 Ref: nodejs/node-gyp#855

bnoordhuis reviewed Jan 7, 2016
View reviewed changes

pmed added 3 commits June 20, 2016 14:12

pmed force-pushed the target-modules-version branch from 44f04b2 to 1844911 Compare June 20, 2016 11:25

bnoordhuis reviewed Jul 16, 2016
View reviewed changes

Fix code style

a6b6f9e

saper reviewed Jul 17, 2016
View reviewed changes

MylesBorins mentioned this pull request Jul 18, 2016

Should we backport node_module_version to v4.x? nodejs/Release#124

Closed

saper mentioned this pull request Aug 8, 2016

build: add node_module_version to config.gypi nodejs/node#8027

Closed

saper mentioned this pull request Aug 18, 2016

build: add node_module_version to config.gypi nodejs/node#8171

Closed

bnoordhuis closed this Oct 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `target_modules_version` variable into generated config.gypi file #855

Add `target_modules_version` variable into generated config.gypi file #855

pmed commented Jan 7, 2016

bnoordhuis Jan 7, 2016

bnoordhuis commented Jan 7, 2016

pmed commented Jan 8, 2016

saper commented Jan 8, 2016

pmed commented Jun 20, 2016

pmed commented Jul 16, 2016

bnoordhuis Jul 16, 2016

bnoordhuis commented Jul 16, 2016

pmed commented Jul 16, 2016

saper Jul 17, 2016

bnoordhuis commented Jul 17, 2016

saper commented Jul 17, 2016

bnoordhuis commented Jul 18, 2016

MylesBorins commented Jul 18, 2016

bnoordhuis commented Jul 18, 2016

saper commented Jul 20, 2016

bnoordhuis commented Jul 20, 2016

bnoordhuis commented Oct 7, 2016

pmed commented Oct 7, 2016

bnoordhuis commented Oct 7, 2016

Add target_modules_version variable into generated config.gypi file #855

Add target_modules_version variable into generated config.gypi file #855

Conversation

pmed commented Jan 7, 2016

bnoordhuis Jan 7, 2016

Choose a reason for hiding this comment

bnoordhuis commented Jan 7, 2016

pmed commented Jan 8, 2016

saper commented Jan 8, 2016

pmed commented Jun 20, 2016

pmed commented Jul 16, 2016

bnoordhuis Jul 16, 2016

Choose a reason for hiding this comment

bnoordhuis commented Jul 16, 2016

pmed commented Jul 16, 2016

saper Jul 17, 2016

Choose a reason for hiding this comment

bnoordhuis commented Jul 17, 2016

saper commented Jul 17, 2016

bnoordhuis commented Jul 18, 2016

MylesBorins commented Jul 18, 2016

bnoordhuis commented Jul 18, 2016

saper commented Jul 20, 2016

bnoordhuis commented Jul 20, 2016

bnoordhuis commented Oct 7, 2016

pmed commented Oct 7, 2016

bnoordhuis commented Oct 7, 2016

Add `target_modules_version` variable into generated config.gypi file #855

Add `target_modules_version` variable into generated config.gypi file #855