-
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
Initial gyp.js support #962
Changes from all commits
8db9e57
5eb0972
eb2c709
1e8a221
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 |
---|---|---|
|
@@ -106,6 +106,37 @@ you can require the `.node` file with Node and run your tests! | |
__Note:__ To create a _Debug_ build of the bindings file, pass the `--debug` (or | ||
`-d`) switch when running either the `configure`, `build` or `rebuild` command. | ||
|
||
#### Usage without Python | ||
|
||
There is a [`gyp.js`](https://github.com/indutny/gyp.js/) project, a GYP | ||
implementation in JavaScript. It generates [`Ninja`](https://ninja-build.org/) | ||
build files and requires no Python installation. | ||
|
||
In this case you will need to install [`Ninja`](https://ninja-build.org/) build | ||
tool and a C/C++ compiler toolchain. | ||
|
||
To generate projects files with `gyp.js` instead of `gyp` and to build them with | ||
`ninja`, please supply a command-line option `--gypjs` in `node-gyp` command. | ||
|
||
``` bash | ||
$ node-gyp configure --gypjs | ||
``` | ||
|
||
It is also possible to set `npm_config_gypjs` environment variable to turn on | ||
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. I would suggest all-capitals env var name: 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. Environment variables named with prefix 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. Ah, ok. My bad then! 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. Though, they are case sensitive. 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. no need for this after #1185 |
||
the `gyp.js` usage. | ||
|
||
``` bash | ||
$ npm_config_gypjs=1 node-gyp build | ||
``` | ||
|
||
Path to an existing `ninja` installation can be set with a `--ninja` command-line | ||
option or in a `NINJA` environment variable. | ||
|
||
``` bash | ||
$ node-gyp configure --gypjs --ninja=/my/path/to/ninja | ||
$ npm_config_gypjs=1 NINJA=/my/path/to/ninja node-gyp build | ||
``` | ||
|
||
|
||
The "binding.gyp" file | ||
---------------------- | ||
|
@@ -181,6 +212,8 @@ Command Options | |
| `--python=$path` | Set path to the python (2) binary | ||
| `--msvs_version=$version` | Set Visual Studio version (win) | ||
| `--solution=$solution` | Set Visual Studio Solution version (win) | ||
| `--gypjs` | Use gyp.js instead of gyp | ||
| `--ninja=$ninja` | Override ninja command (with --gypjs) | ||
|
||
|
||
License | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,7 +92,7 @@ | |
'-luuid.lib', | ||
'-lodbc32.lib', | ||
'-lDelayImp.lib', | ||
'-l"<(node_root_dir)/$(ConfigurationName)/<(node_lib_file)"' | ||
'-l"<(node_lib_file)"' | ||
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. Why? 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. $(ConfigurationName) is a MSBuild macro unknown for Ninja. So I changed it to See comments in PR #964. Actually that request should be merged before this one. 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. For the record #964 landed |
||
], | ||
'msvs_disabled_warnings': [ | ||
# warning C4251: 'node::ObjectWrap::handle_' : class 'v8::Persistent<T>' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,12 +11,12 @@ var fs = require('graceful-fs') | |
, glob = require('glob') | ||
, log = require('npmlog') | ||
, which = require('which') | ||
, mkdirp = require('mkdirp') | ||
, exec = require('child_process').exec | ||
, processRelease = require('./process-release') | ||
, win = process.platform == 'win32' | ||
|
||
exports.usage = 'Invokes `' + (win ? 'msbuild' : 'make') + '` and builds the module' | ||
exports.usage = 'Invokes `' + (process.env.npm_config_gypjs ? 'ninja' : | ||
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. At this point it may be useful to just have several ifs here, IMO. 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. Now node-gyp is for node >=4 can declare a const and use ` templates |
||
(win ? 'msbuild' : 'make')) + '` and builds the module' | ||
|
||
function build (gyp, argv, callback) { | ||
var platformMake = 'make' | ||
|
@@ -36,8 +36,10 @@ function build (gyp, argv, callback) { | |
, config | ||
, arch | ||
, nodeDir | ||
, copyDevLib | ||
|
||
if (gyp.opts.gypjs) { | ||
command = gyp.opts.ninja || process.env.NINJA || 'ninja' | ||
} | ||
loadConfigGypi() | ||
|
||
/** | ||
|
@@ -60,7 +62,6 @@ function build (gyp, argv, callback) { | |
buildType = config.target_defaults.default_configuration | ||
arch = config.variables.target_arch | ||
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. Why? 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. There is no need to cope node.lib into node_dir/[Debug | Release] directory. See PR #964 |
||
nodeDir = config.variables.nodedir | ||
copyDevLib = config.variables.copy_dev_lib == 'true' | ||
|
||
if ('debug' in gyp.opts) { | ||
buildType = gyp.opts.debug ? 'Debug' : 'Release' | ||
|
@@ -73,7 +74,7 @@ function build (gyp, argv, callback) { | |
log.verbose('architecture', arch) | ||
log.verbose('node dev dir', nodeDir) | ||
|
||
if (win) { | ||
if (win && !gyp.opts.gypjs) { | ||
findSolutionFile() | ||
} else { | ||
doWhich() | ||
|
@@ -105,7 +106,7 @@ function build (gyp, argv, callback) { | |
// First make sure we have the build command in the PATH | ||
which(command, function (err, execPath) { | ||
if (err) { | ||
if (win && /not found/.test(err.message)) { | ||
if (win && !gyp.opts.gypjs && /not found/.test(err.message)) { | ||
// On windows and no 'msbuild' found. Let's guess where it is | ||
findMsbuild() | ||
} else { | ||
|
@@ -115,7 +116,7 @@ function build (gyp, argv, callback) { | |
return | ||
} | ||
log.verbose('`which` succeeded for `' + command + '`', execPath) | ||
copyNodeLib() | ||
doBuild() | ||
}) | ||
} | ||
|
||
|
@@ -173,36 +174,12 @@ function build (gyp, argv, callback) { | |
return | ||
} | ||
command = msbuildPath | ||
copyNodeLib() | ||
doBuild() | ||
}) | ||
})() | ||
}) | ||
} | ||
|
||
/** | ||
* Copies the node.lib file for the current target architecture into the | ||
* current proper dev dir location. | ||
*/ | ||
|
||
function copyNodeLib () { | ||
if (!win || !copyDevLib) return doBuild() | ||
|
||
var buildDir = path.resolve(nodeDir, buildType) | ||
, archNodeLibPath = path.resolve(nodeDir, arch, release.name + '.lib') | ||
, buildNodeLibPath = path.resolve(buildDir, release.name + '.lib') | ||
|
||
mkdirp(buildDir, function (err, isNew) { | ||
if (err) return callback(err) | ||
log.verbose('"' + buildType + '" dir needed to be created?', isNew) | ||
var rs = fs.createReadStream(archNodeLibPath) | ||
, ws = fs.createWriteStream(buildNodeLibPath) | ||
log.verbose('copying "' + release.name + '.lib" for ' + arch, buildNodeLibPath) | ||
rs.pipe(ws) | ||
rs.on('error', callback) | ||
ws.on('error', callback) | ||
rs.on('end', doBuild) | ||
}) | ||
} | ||
|
||
/** | ||
* Actually spawn the process and compile the module. | ||
|
@@ -212,57 +189,72 @@ function build (gyp, argv, callback) { | |
|
||
// Enable Verbose build | ||
var verbose = log.levels[log.level] <= log.levels.verbose | ||
if (!win && verbose) { | ||
argv.push('V=1') | ||
} | ||
if (win && !verbose) { | ||
argv.push('/clp:Verbosity=minimal') | ||
} | ||
if (!gyp.opts.gypjs) { | ||
if (!win && verbose) { | ||
argv.push('V=1') | ||
} | ||
if (win && !verbose) { | ||
argv.push('/clp:Verbosity=minimal') | ||
} | ||
|
||
if (win) { | ||
// Turn off the Microsoft logo on Windows | ||
argv.push('/nologo') | ||
} | ||
if (win) { | ||
// Turn off the Microsoft logo on Windows | ||
argv.push('/nologo') | ||
} | ||
|
||
// Specify the build type, Release by default | ||
if (win) { | ||
var p = arch === 'x64' ? 'x64' : 'Win32' | ||
argv.push('/p:Configuration=' + buildType + ';Platform=' + p) | ||
if (jobs) { | ||
var j = parseInt(jobs, 10) | ||
if (!isNaN(j) && j > 0) { | ||
argv.push('/m:' + j) | ||
} else if (jobs.toUpperCase() === 'MAX') { | ||
argv.push('/m:' + require('os').cpus().length) | ||
// Specify the build type, Release by default | ||
if (win) { | ||
var p = arch === 'x64' ? 'x64' : 'Win32' | ||
argv.push('/p:Configuration=' + buildType + ';Platform=' + p) | ||
if (jobs) { | ||
var j = parseInt(jobs, 10) | ||
if (!isNaN(j) && j > 0) { | ||
argv.push('/m:' + j) | ||
} else if (jobs.toUpperCase() === 'MAX') { | ||
argv.push('/m:' + require('os').cpus().length) | ||
} | ||
} | ||
} else { | ||
argv.push('BUILDTYPE=' + buildType) | ||
// Invoke the Makefile in the 'build' dir. | ||
argv.push('-C') | ||
argv.push('build') | ||
if (jobs) { | ||
var j = parseInt(jobs, 10) | ||
if (!isNaN(j) && j > 0) { | ||
argv.push('--jobs') | ||
argv.push(j) | ||
} else if (jobs.toUpperCase() === 'MAX') { | ||
argv.push('--jobs') | ||
argv.push(require('os').cpus().length) | ||
} | ||
} | ||
} | ||
|
||
if (win) { | ||
// did the user specify their own .sln file? | ||
var hasSln = argv.some(function (arg) { | ||
return path.extname(arg) == '.sln' | ||
}) | ||
if (!hasSln) { | ||
argv.unshift(gyp.opts.solution || guessedSolution) | ||
} | ||
} | ||
} else { | ||
argv.push('BUILDTYPE=' + buildType) | ||
// Invoke the Makefile in the 'build' dir. | ||
argv.push('-C') | ||
argv.push('build') | ||
// build with ninja | ||
if (verbose) { | ||
argv.push('-v') | ||
} | ||
// Specify the build type, Release by default | ||
argv.push('-C', path.join('build', buildType)) | ||
if (jobs) { | ||
var j = parseInt(jobs, 10) | ||
var j = jobs.toUpperCase() === 'MAX'? require('os').cpus().length : parseInt(jobs, 10) | ||
if (!isNaN(j) && j > 0) { | ||
argv.push('--jobs') | ||
argv.push(j) | ||
} else if (jobs.toUpperCase() === 'MAX') { | ||
argv.push('--jobs') | ||
argv.push(require('os').cpus().length) | ||
argv.push('-j' + j) | ||
} | ||
} | ||
} | ||
|
||
if (win) { | ||
// did the user specify their own .sln file? | ||
var hasSln = argv.some(function (arg) { | ||
return path.extname(arg) == '.sln' | ||
}) | ||
if (!hasSln) { | ||
argv.unshift(gyp.opts.solution || guessedSolution) | ||
} | ||
} | ||
|
||
var proc = gyp.spawn(command, argv) | ||
proc.on('exit', onExit) | ||
} | ||
|
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.
@pmed this is no longer the case. I would say "For fast and incremental builds
ninja
build too should be installed, otherwise a default JavaScript shim will be used to build your addon".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.
Though,
node-gyp
will probably need to be modified to use it.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.
Or alternatively
npm install -g ninja.js
may be recommended, if binary is not available.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.
This shouldn't be a blocker for this PR, though.
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.
There should be a fallback to
ninja.js
indoWhich()
checks, when ninja was not found.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.
What about using ninja.js directly instead?
El 2/8/2016 11:22, "Pavel Medvedev" [email protected] escribió:
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.
Current status of
ninja.js
is "work-in-progress", and I've never tried it. Maybe later, in next PR after @indutny will releaseninja.js
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.
Let's do it in next PR indeed.
ninja.js
is kind of working, but we don't want to delay this thing.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.
BTW I don't know how to spawn
ninja.js
as a command-line utility in cross-platfrom way.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.
Don't worry too much about it. I'll send a PR once this will land.