Skip to content

build: add node_module_version to config.gypi - #10355

Closed
bnoordhuis wants to merge 1 commit into
nodejs:v0.12-stagingfrom
bnoordhuis:pr8027
Closed

build: add node_module_version to config.gypi#10355
bnoordhuis wants to merge 1 commit into
nodejs:v0.12-stagingfrom
bnoordhuis:pr8027

Conversation

@bnoordhuis

@bnoordhuisbnoordhuis commented Dec 20, 2016

Copy link
Copy Markdown
Member

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
@nodejs-github-botnodejs-github-bot added build Issues and PRs related to build files or the CI. v0.12 labels Dec 20, 2016
@gibfahngibfahn mentioned this pull request Dec 20, 2016
@@ -0,0 +1,10 @@
'use strict';
require('../common');
var assert = require('assert');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: const

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Tests in v0.12 overwhelmingly use var. let and const have bugs or at least not the right semantics.

assert(process.config.variables.hasOwnProperty('node_module_version'));

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you mean to use Number.isInteger? The comment above suggests that.

@thefourtheye

Copy link
Copy Markdown
Contributor

cc @saper

@rvagg

Copy link
Copy Markdown
Member

-1, I think I already said that on the original issue, I don't see a solid case for backporting this

@saper

Copy link
Copy Markdown

Why? A whole idea of this change is to get it supported in as many branches as possible. We even produce and ship binaries for 0.10...

@jasnell

Copy link
Copy Markdown
Member

I'm -0 on landing this. There's likely no harm in doing so but given that support for v0.12 is ending in two days and there will no longer be any official support for it, there's not a lot of justification. That said, there's no harm in continuing to accept PRs if members of the community really do want to keep supporting it on their own.

@gibfahn

gibfahn commented Dec 30, 2016

Copy link
Copy Markdown
Member

@saper for which modules do you still ship node 0.10 binaries? How long are you planning to maintain 0.10 and 0.12 now they're out of support? Is it a case of there being no reason to drop support (yet)?


@jasnell so you mean we could land commits on v0.12-staging but never actually do a release? I think people might conflate actively updated with supported.

@mscdex

Copy link
Copy Markdown
Contributor

Closing this for now since v0.12 is no longer.

@mscdexmscdex closed this Jan 17, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@bnoordhuis@thefourtheye@rvagg@saper@jasnell@gibfahn@mscdex@nodejs-github-bot