build: add node_module_version to config.gypi by saper · Pull Request #7808 · nodejs/node · GitHub

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>

Missing ;

LGTM with style nit.

cc @nodejs/lts

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

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

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.

@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).

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.

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

Should I prepare pull requests for 0.12 and LTS?

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

Nit fixed, thank you.

ping @bnoordhuis ... does this LGTY?

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.

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.

@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)

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.

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

@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>

@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

Closing given that v0.10 is no longer actively supported

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

build: add node_module_version to config.gypi by saper · Pull Request #7808 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: add node_module_version to config.gypi - #7808

build: add node_module_version to config.gypi#7808saper wants to merge 2 commits intonodejs:v0.10-stagingnodejs/node:v0.10-stagingfrom saper:node_module_version/v0.10saper/node:node_module_version/v0.10Copy head branch name to clipboard

Conversation

saper commented Jul 20, 2016

Uh oh!

bnoordhuis Jul 20, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jul 20, 2016

Uh oh!

Fishrock123 commented Jul 20, 2016

Uh oh!

bnoordhuis commented Jul 20, 2016

Uh oh!

saper commented Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rvagg commented Jul 20, 2016

Uh oh!

jasnell commented Aug 1, 2016

Uh oh!

jasnell commented Aug 4, 2016

Uh oh!

saper commented Aug 4, 2016

Uh oh!

bnoordhuis commented Aug 5, 2016

Uh oh!

saper commented Aug 8, 2016

Uh oh!

jasnell commented Aug 18, 2016

Uh oh!

bnoordhuis commented Aug 18, 2016

Uh oh!

jasnell commented Aug 18, 2016

Uh oh!

saper commented Aug 18, 2016

Uh oh!

jasnell Aug 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Aug 18, 2016

Choose a reason for hiding this comment

Uh oh!

saper Aug 19, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell Aug 19, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell Aug 18, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Aug 19, 2016

Choose a reason for hiding this comment

Uh oh!

saper Aug 19, 2016

Choose a reason for hiding this comment

Uh oh!

saper commented Aug 18, 2016

Uh oh!

jasnell commented Oct 26, 2016

Uh oh!

jasnell commented Jan 6, 2017

Uh oh!

saper commented Jan 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

build: add node_module_version to config.gypi#7808
saper wants to merge 2 commits into
nodejs:v0.10-stagingfrom
saper:node_module_version/v0.10

saper commented Jul 20, 2016 •
edited
Loading

jasnell Aug 18, 2016 •
edited
Loading