-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add target_modules_version
variable into generated config.gypi file
#855
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
module.exports = exports = configure | ||
module.exports.test = { findAccessibleSync: findAccessibleSync, | ||
findPython: findPython } | ||
findPython: findPython, | ||
nodeModulesVersion: nodeModulesVersion } | ||
|
||
/** | ||
* Module dependencies. | ||
|
@@ -127,6 +128,15 @@ function configure (gyp, argv, callback) { | |
|
||
// set the target_arch variable | ||
variables.target_arch = gyp.opts.arch || process.arch || 'ia32' | ||
log.verbose('build/' + configFilename, 'target arch: ' + variables.target_arch) | ||
|
||
// set the target_platform variable | ||
variables.target_platform = process.platform | ||
log.verbose('build/' + configFilename, 'target platform: ' + variables.target_platform) | ||
|
||
// set the target modules version | ||
variables.target_modules_version = nodeModulesVersion(nodeDir) | ||
log.verbose('build/' + configFilename, 'modules version: ' + variables.target_modules_version) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you wrap lines at 80 columns? I realize there are long lines elsewhere but I'd like to avoid introducing new ones. |
||
|
||
// set the node development directory | ||
variables.nodedir = nodeDir | ||
|
@@ -311,6 +321,42 @@ function configure (gyp, argv, callback) { | |
|
||
} | ||
|
||
/** | ||
* Returns value of NODE_MODULE_VERSION define | ||
* from Node source code in specified directory | ||
* for specified Node version | ||
*/ | ||
function nodeModulesVersion(nodeDir) { | ||
var versionStr = path.basename(nodeDir) | ||
|
||
// parse version from the base name of nodeDir | ||
// probably removing optional `releaseName-` prefix | ||
var nodeVersion = semver.valid(versionStr) | ||
|| semver.valid(versionStr.substr(versionStr.indexOf('-') + 1)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
if (!nodeVersion) { | ||
throw new Error('Can\'t extract target version from ' + nodeDir) | ||
} | ||
|
||
// NODE_MODULE_VERSION may be defined in different files | ||
// depending on the target Node.js version | ||
if (semver.gte(nodeVersion, '3.0.0')) { | ||
nodeVersion = 'include/node/node_version.h' | ||
} else if (semver.gt(nodeVersion, '0.11.4')) { | ||
nodeVersion = 'src/node_version.h' | ||
} else { | ||
nodeVersion = 'src/node.h' | ||
} | ||
nodeVersion = path.join(nodeDir, nodeVersion) | ||
|
||
var modulesVersion = fs.readFileSync(nodeVersion, { encoding: 'utf8'}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: space before You should wrap the EDIT: Then again, maybe it's better for the error to bubble up. |
||
.match(/#define\s+NODE_MODULE_VERSION\s+\(?(\S+)\)?/) | ||
if (!modulesVersion) { | ||
throw new Error('#define NODE_MODULE_VERSION not found in ' + nodeVersion) | ||
} | ||
return parseInt(modulesVersion[1]).toString() | ||
} | ||
|
||
/** | ||
* Returns the first file or directory from an array of candidates that is | ||
* readable by the current user, or undefined if none of the candidates are | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
'use strict' | ||
|
||
var test = require('tape') | ||
var path = require('path') | ||
var semver = require('semver') | ||
var processRelease = require('../lib/process-release') | ||
var configure = require('../lib/configure') | ||
|
||
var gyp = require('../lib/node-gyp')() // for gyp.devDir | ||
gyp.parseArgv([]) // to initialize gyp.opts | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style nit: two spaces before |
||
|
||
test('modules version for the current release', function (t) { | ||
t.plan(1) | ||
|
||
var release = processRelease([], gyp, process.version) | ||
var nodeDir = path.join(gyp.devDir, release.versionDir) | ||
var modulesVersion = configure.test.nodeModulesVersion(nodeDir) | ||
t.equal(modulesVersion, process.versions.modules, | ||
nodeDir + ' modulesVersion=' + modulesVersion) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style nit: can you line up the arguments here and below? |
||
}) | ||
|
||
test('node modules version for installed targets', function (t) { | ||
gyp.commands.list([], function(err, versions) { | ||
if (err) return t.fail(err) | ||
t.plan(versions.length) | ||
|
||
versions.forEach(function(version) { | ||
var nodeDir = path.join(gyp.devDir, version) | ||
var modulesVersion = configure.test.nodeModulesVersion(nodeDir) | ||
t.ok(modulesVersion > 0, | ||
nodeDir + ' modulesVersion=' + modulesVersion) | ||
}) | ||
}) | ||
}) |
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.
Maybe we should allow manual override here as well? What about cross-platform builds?