build: add node_module_version to config.gypi #7808

saper · 2016-07-20T08:52:51Z

Enable targetting of a different node version than
the currently running one when building binary modules.

This is extracted from 410296c37

Work around spec violations in V8 where it checks that `this == NULL`. GCC 6 started exploiting this particular kind of UB, resulting in runtime crashes. Fixes: nodejs#6724 PR-URL: nodejs#6738 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

bnoordhuis · 2016-07-20T09:18:36Z

test/simple/test-module-version.js

+assert(process.config.variables.hasOwnProperty('node_module_version'));
+
+// ensure that `node_module_version` is an Integer > 0
+assert(!Number.isNaN(parseInt(process.config.variables.node_module_version)))


bnoordhuis · 2016-07-20T09:19:05Z

LGTM with style nit.

cc @nodejs/lts

Fishrock123 · 2016-07-20T09:30:10Z

I'm not sure we'd actually backport this kinda thing to v0.10... @nodejs/lts

bnoordhuis · 2016-07-20T09:53:39Z

I commented on that here. I think a case for an exception can be made here.

saper · 2016-07-20T09:54:47Z

For example at https://github.com/sass/node-sass/releases/tag/v3.8.0 we are building binaries for all known node modules versions. It would be easier to support and cross-compile to older node versions if they need not be installed. Having the same interface in the config.gypi would be really cool, without resorting to workarounds like manual parsing of the node header files. Having it only in the newest version kind of defeats its purpose - it works only for the current and the future versions. For legacy versions, one would need to resort to manual version guessing.

One uses always the latest v0.10, v0.12 etc. headers and config.gypi files anyway, so if this change makes it into, say, v0.10.47 and v0.12.16, I don't need to bother about older 0.10 and 0.12 versions, since the binaries will work there, too.

rvagg · 2016-07-20T11:59:15Z

@saper you could hardwire values for 0.10 and 0.12 though, right? those are fixed numbers and not going to change (same with every other major but I take the point about it being easier into the future).

jasnell · 2016-08-01T18:08:55Z

I've no objections to backporting this to v0.10 with the caveat that anything that depends on process.config is inherently flaky.

This PR LGTM.

jasnell · 2016-08-04T20:14:08Z

@nodejs/lts ... any additional thoughts? I'm inclined to land unless there are objections.

saper · 2016-08-04T21:26:30Z

Should I prepare pull requests for 0.12 and LTS?

bnoordhuis · 2016-08-05T08:47:00Z

@saper Yes, go ahead. One small style nit: can you capitalize the comments in the test?

saper · 2016-08-08T23:03:59Z

Nit fixed, thank you.

jasnell · 2016-08-18T18:34:59Z

ping @bnoordhuis ... does this LGTY?

bnoordhuis · 2016-08-18T20:23:45Z

The test could use assert.equal() when comparing the numbers. LGTM either way though.

To who lands this: s/Provide/provide/ in the title of the commit and maybe drop the quotes so it fits in 50 columns.

jasnell · 2016-08-18T22:07:05Z

Oh, right, didn't notice that this was against v0.10 directly. Either we can open a new PR against v0.10-staging or we can roll the dice with GitHubs new PR-retargeting feature.

saper · 2016-08-18T22:17:45Z

@jasnell just tried that and I think it might have worked (I got 5 commits after retarget so I have pushed a cherry-picked commit)

jasnell · 2016-08-18T22:19:19Z

test/simple/test-module-version.js

@@ -0,0 +1,10 @@
+'use strict';
+require('../common');
+var assert = require('assert');


~~can you use const here please :-)~~

ugh.. ha! nevermind... v0.10.... silly me

Do you think we might actually want to add const in the current trunk?

node/test/parallel/test-module-version.js

Line 3 in 8ff3d61

var assert = require('assert');

We should be using const in master, definitely, but by itself that's not enough of a reason to update the test case if it's not using const already. Best to leave it unless there are other changes to make.

jasnell · 2016-08-18T22:19:57Z

test/simple/test-module-version.js

+
+// Ensure that `node_module_version` is an Integer
+assert(!Number.isNaN(parseInt(process.config.variables.node_module_version)));
+assert.equal(process.config.variables.node_module_version, 11);


assert.strictEqual() here instead of assert.equal()

Apologies, I said assert.equal() because I had a fuzzy notion that strictEqual() doesn't exist in v0.10. I was probably thinking of deepStrictEqual().

No problem, happy to learn more about differences between node engines.

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

saper · 2016-08-18T22:42:47Z

@jasnell voilà!

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell · 2016-10-26T20:48:07Z

@nodejs/lts... this appears to be the only still open PR based on v0.10-staging. Given that it's not likely that we'll be cutting a new v0.10, I recommend that we go ahead and close this.

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

jasnell · 2017-01-06T20:06:37Z

Closing given that v0.10 is no longer actively supported

saper · 2017-01-06T20:15:56Z

Thanks, if there will be no 0.10 release this makes no point.

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 20, 2016

This was referenced Jul 20, 2016

Add target_modules_version variable into generated config.gypi file nodejs/node-gyp#855

Closed

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

Closed

bnoordhuis reviewed Jul 20, 2016
View reviewed changes

bnoordhuis added the v0.10 label Jul 20, 2016

saper mentioned this pull request Jul 20, 2016

Officially support Alpine linux sass/node-sass#1589

Closed

saper force-pushed the node_module_version/v0.10 branch from a6be6b5 to 441d8b9 Compare July 20, 2016 10:19

saper force-pushed the node_module_version/v0.10 branch 2 times, most recently from 9ff899f to d108c3b Compare July 20, 2016 12:44

saper force-pushed the node_module_version/v0.10 branch from d108c3b to 4c24cb3 Compare August 8, 2016 23:03

saper mentioned this pull request Aug 8, 2016

build: add node_module_version to config.gypi #8027

Closed

saper force-pushed the node_module_version/v0.10 branch from 4c24cb3 to cb47e37 Compare August 8, 2016 23:51

saper force-pushed the node_module_version/v0.10 branch from cb47e37 to 3f7a52b Compare August 18, 2016 20:52

saper mentioned this pull request Aug 18, 2016

build: add node_module_version to config.gypi #8171

Closed

saper changed the base branch from v0.10 to v0.10-staging August 18, 2016 22:15

saper force-pushed the node_module_version/v0.10 branch from 3f7a52b to 7299eac Compare August 18, 2016 22:16

jasnell reviewed Aug 18, 2016
View reviewed changes

build: add node_module_version to config.gypi

6a47a07

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

saper force-pushed the node_module_version/v0.10 branch from 7299eac to 6a47a07 Compare August 18, 2016 22:39

rvagg force-pushed the v0.10-staging branch from 7f44ecf to 35b49ed Compare September 22, 2016 23:52

mscdex removed the v0.10 label Nov 14, 2016

rvagg mentioned this pull request Dec 20, 2016

v0.12.18 release proposal #10352

Merged

jasnell closed this Jan 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add node_module_version to config.gypi #7808

build: add node_module_version to config.gypi #7808

saper commented Jul 20, 2016

bnoordhuis Jul 20, 2016

bnoordhuis commented Jul 20, 2016

Fishrock123 commented Jul 20, 2016

bnoordhuis commented Jul 20, 2016

saper commented Jul 20, 2016 •

edited

Loading

rvagg commented Jul 20, 2016

jasnell commented Aug 1, 2016

jasnell commented Aug 4, 2016

saper commented Aug 4, 2016

bnoordhuis commented Aug 5, 2016

saper commented Aug 8, 2016

jasnell commented Aug 18, 2016

bnoordhuis commented Aug 18, 2016

jasnell commented Aug 18, 2016

saper commented Aug 18, 2016

jasnell Aug 18, 2016 •

edited

Loading

jasnell Aug 18, 2016

saper Aug 19, 2016

jasnell Aug 19, 2016

jasnell Aug 18, 2016

bnoordhuis Aug 19, 2016

saper Aug 19, 2016

saper commented Aug 18, 2016

jasnell commented Oct 26, 2016

jasnell commented Jan 6, 2017

saper commented Jan 6, 2017

build: add node_module_version to config.gypi #7808

build: add node_module_version to config.gypi #7808

Conversation

saper commented Jul 20, 2016

bnoordhuis Jul 20, 2016

Choose a reason for hiding this comment

bnoordhuis commented Jul 20, 2016

Fishrock123 commented Jul 20, 2016

bnoordhuis commented Jul 20, 2016

saper commented Jul 20, 2016 • edited Loading

rvagg commented Jul 20, 2016

jasnell commented Aug 1, 2016

jasnell commented Aug 4, 2016

saper commented Aug 4, 2016

bnoordhuis commented Aug 5, 2016

saper commented Aug 8, 2016

jasnell commented Aug 18, 2016

bnoordhuis commented Aug 18, 2016

jasnell commented Aug 18, 2016

saper commented Aug 18, 2016

jasnell Aug 18, 2016 • edited Loading

Choose a reason for hiding this comment

jasnell Aug 18, 2016

Choose a reason for hiding this comment

saper Aug 19, 2016

Choose a reason for hiding this comment

jasnell Aug 19, 2016

Choose a reason for hiding this comment

jasnell Aug 18, 2016

Choose a reason for hiding this comment

bnoordhuis Aug 19, 2016

Choose a reason for hiding this comment

saper Aug 19, 2016

Choose a reason for hiding this comment

saper commented Aug 18, 2016

jasnell commented Oct 26, 2016

jasnell commented Jan 6, 2017

saper commented Jan 6, 2017

saper commented Jul 20, 2016 •

edited

Loading

jasnell Aug 18, 2016 •

edited

Loading