-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
build: add node_module_version to config.gypi #10355
Conversation
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
@@ -0,0 +1,10 @@ | |||
'use strict'; | |||
require('../common'); | |||
var assert = require('assert'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: const
There was a problem hiding this comment.
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))); |
There was a problem hiding this comment.
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.
cc @saper |
-1, I think I already said that on the original issue, I don't see a solid case for backporting this |
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... |
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. |
@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 |
Closing this for now since v0.12 is no longer. |
Rebase of #8027.
CI: https://ci.nodejs.org/job/node-test-pull-request/5486/