Prefix build targets with /t: on Windows #1164
Conversation
Currently, on non-Windows platforms, it is possible to have a multi-target native module and specify building just some of the targets using `node-gyp build my_target`. On Windows, however, specifying the target(s) to build requires the [`/t:` or `/target:` flag](https://msdn.microsoft.com/en-us/library/ms164311.aspx). Without this change you will get the error `MSB1008` because MSBuild things you are trying to specify multiple solutions.
|
I will admit to not exactly understanding what this does. Is this for a binding.gyp that produces multiple .node files? In any case, it would be nice to have a regression test. |
|
Yes, for example take the binding file below: {
"targets": [{
"target_name": "my_module",
"sources": [
"src/my_module.cpp",
],
"include_dirs": [
"src",
"<!(node -e \"require('nan')\")"
]
}, {
"target_name": "tests",
"sources": [
"src/tests/my_tests.cpp"
],
"include_dirs": [
"src",
"<!(node -e \"require('nan')\")"
]
}]
}I have a native module similar to this that has some tests written in C++ for native-only classes. There is no reason for this test module to build when installed from npm so I have in my package.json I will see if I can add a test for this. |
|
I've added a test for specifying the module. If you'd like to add a case that tests a multi-target gyp file just let me know. |
test/test-addon.js
Outdated
| test('build specific addon', function (t) { | ||
| t.plan(4) | ||
|
|
||
| var cmd = [nodeGyp, 'clean', 'configure', '-C', addonPath, '--loglevel=verbose'] |
There was a problem hiding this comment.
Oops, cruft from before moving it into the exec function. Removed.
There was a problem hiding this comment.
LGTM, but...
If you'd like to add a case that tests a multi-target gyp file just let me know.
...would be nice. CI: https://ci.nodejs.org/job/nodegyp-test-pull-request/5/
|
@bnoordhuis If I'm reading that output correct, it looks like the tests failed to download the node headers for 4.8.1? That doesn't seem like the kind of thing my changes would cause. Do these tests ever flap, or am I not reading this correctly? |
|
Probably a hiccup. New run: https://ci.nodejs.org/job/nodegyp-test-pull-request/7/ |
|
Ugh, looks like Here's if you run both tests in the same go with $ tape test/test-*addon.js
TAP version 13
# build simple addon
# /Users/nwolfe/work/ext/node-gyp/bin/node-gyp.js clean rebuild -C /Users/nwolfe/work/ext/node-gyp/test/node_modules/hello_world --loglevel=verbose
ok 1 should be equal
ok 2 should end in ok
ok 3 should be equal
ok 4 should be equal
# build specific addon
# /Users/nwolfe/work/ext/node-gyp/bin/node-gyp.js clean configure -C /Users/nwolfe/work/ext/node-gyp/test/node_modules/hello_world --loglevel=verbose
ok 5 clean build
# /Users/nwolfe/work/ext/node-gyp/bin/node-gyp.js build -C /Users/nwolfe/work/ext/node-gyp/test/node_modules/hello_world --loglevel=verbose hello
ok 6 should be equal
ok 7 should end in ok
ok 8 should be equal
{ world: [Function] }
not ok 9 should not have loaded unbuilt module
---
operator: fail
at: maybeClose (internal/child_process.js:854:16)
...
1..9
# tests 9
# pass 8
# fail 1And this is running $ tape test/test-addon.js
TAP version 13
# build simple addon
# /Users/nwolfe/work/ext/node-gyp/bin/node-gyp.js clean rebuild -C /Users/nwolfe/work/ext/node-gyp/test/node_modules/hello_world --loglevel=verbose
ok 1 should be equal
ok 2 should end in ok
ok 3 should be equal
ok 4 should be equal
1..4
# tests 4
# pass 4
# ok
$ tape test/test-specific-addon.js
TAP version 13
# build specific addon
# /Users/nwolfe/work/ext/node-gyp/bin/node-gyp.js clean configure -C /Users/nwolfe/work/ext/node-gyp/test/node_modules/hello_world --loglevel=verbose
ok 1 clean build
# /Users/nwolfe/work/ext/node-gyp/bin/node-gyp.js build -C /Users/nwolfe/work/ext/node-gyp/test/node_modules/hello_world --loglevel=verbose hello
ok 2 should be equal
ok 3 should end in ok
ok 4 should be equal
ok 5 should not build world
1..5
# tests 5
# pass 5
# ok |
|
That's issue #1123, it's on my todo list for when I have a spare afternoon. Would some judicious |
|
@NatalieWolfe Are you still working on this? As a workaround you could make the test do |
|
@NatalieWolfe I had a much simpler idea: split your If you run binding.gyp{
"targets": [{
"target_name": "my_module",
"sources": [
"hello.cc",
],
}]
}test.gyp{
"targets": [{
"include_dirs" : [
"<!(node -e \"require('nan')\")"
],
"dependencies": [
"./binding.gyp:my_module"
],
"target_name": "tests",
"sources": [
"tests/hello-test.cc"
],
}]
} |
Currently, on non-Windows platforms, it is possible to have a multi-target native module and specify building just some of the targets using `node-gyp build my_target`. On Windows, however, specifying the targets to build requires the `/t:` or `/target:` flag. Without this change you will get an `MSB1008` error because MSBuild thinks you are trying to specify multiple solutions. PR-URL: #1164 Reviewed-By: Ben Noordhuis <[email protected]>
|
Landed in 4379c47. I left out the tests because of conflicts. @NatalieWolfe Can you open a new pull request if you want to see those merged? Cheers. |
..until fixing command line processing in Windows broken by a change making multi-target invokation possible in Windows, just like in other OSes, nodejs/node-gyp#1164 The change did not account for the special case of passing a define on command line.
nodejs/node-gyp#1164 interprets additional node-gyp build parameters as target names, which breaks the current way CL configs are passed to msbuild in install.js. Move the detection logic into configuration phase and pass CL configs as gyp variables. Fixes nodejs#321, fixes nodejs#377
Currently, on non-Windows platforms, it is possible to have a multi-target native module and specify building just some of the targets using
node-gyp build my_target. On Windows, however, specifying the target(s) to build requires the/t:or/target:flag. Without this change you will get the errorMSB1008because MSBuild things you are trying to specify multiple solutions.