Skip to content
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: use target_arch config variable to link against node.lib #964

Closed
wants to merge 2 commits into from

Conversation

pmed
Copy link
Contributor

@pmed pmed commented Jun 20, 2016

PR extracted form #962

on Windows

Using target_arch in addon.gypi to link against Node.js library.
This variable was written into build/config.gypi on the configure stage.

Do not copy node.lib into node_root_dir/Release or node_root_dir/Debug
on Windows, link it from node_root_dir/target_arch directory.

Removed unused copyNodeLib() function
Removed unused copy_dev_lib variable introduced in commit
@84d24189735e19350a93aaf9f6a327bb4c52349e

…ndows

Using `target_arch` in addon.gypi to link against Node.js library.
This variable was written into build/config.gypi on the `configure` stage.

Do not copy node.lib into node_root_dir/Release or node_root_dir/Debug
on Windows, link it from node_root_dir/target_arch directory.

Removed unused copyNodeLib() function
Removed unused `copy_dev_lib` variable introduced in commit
@84d24189735e19350a93aaf9f6a327bb4c52349e
addon.gypi Outdated
@@ -92,7 +92,7 @@
'-luuid.lib',
'-lodbc32.lib',
'-lDelayImp.lib',
'-l"<(node_root_dir)/$(ConfigurationName)/<(node_lib_file)"'
'-l"<(node_root_dir)/<(target_arch)/<(node_lib_file)"'
Copy link
Member

@richardlau richardlau Jun 23, 2016

Choose a reason for hiding this comment

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

This change will break users on Windows who compile Node.js from source and use --nodedir to point at their source tree. When building from source, the node.lib file is written to either the Release or Debug directory depending on the build configuration (i.e. the value of $(ConfigurationName)) used.

Set path to node lib in `$(Configuration)` dir when `--nodedir` option is
supplied, otherwise use value of `target_arch` variable.
@pmed
Copy link
Contributor Author

pmed commented Jun 23, 2016

@richardlau, thanks for pointing out the --nodedir option.

I've changed the node_lib_file variable to use full path to the node lib there. In case when the --nodedir option was set, this full path to the node lib includes $(Configuration) part, otherwise it includes target_arch variable.

@rvagg
Copy link
Member

rvagg commented Jun 27, 2016

I need to defer to @nodejs/platform-windows folks on this, is it safe to compile against a node.lib that's not in the build directory? Is it statically linked so that if it gets deleted in the original location then it won't cause problems?

@pmed does this work on Node.js v0.10 and v0.12? Lots of stuff changed on the v4 bump including file locations.

@pmed
Copy link
Contributor Author

pmed commented Jun 27, 2016

I've checked these changes with Node versions 0.10.46, 0.12.15 and they work, among with --nodedir option and nodejs sources at tags 0.10.45, 0.12.14 on combinations of Debug|Release, x64|ia32 switches.

@rvagg
Copy link
Member

rvagg commented Jul 7, 2016

@pmed don't let us forget about this, we're going to start work on v4 and this would be a good inclusion for testing

@bnoordhuis
Copy link
Member

Ping @nodejs/platform-windows, see @rvagg's question here: #964 (comment)

Is it safe to compile against a node.lib that's not in the build directory? Is it statically linked so that if it gets deleted in the original location then it won't cause problems?

@pmed
Copy link
Contributor Author

pmed commented Oct 7, 2016

It seems there could be a case, when a user would remove nodeir after node-gyp configure --nodedir=some/location and before node-gyp build.

But I don't think this is a issue, because in this case node-gyp configure could be invoked again with new --nodedir option.

@indutny
Copy link
Member

indutny commented Nov 11, 2016

ping @nodejs/platform-windows we need your help to get this landed.

@seishun
Copy link

seishun commented Nov 11, 2016

node.lib is statically linked, it's not a dll. For the other questions, I need some steps to test this because it's been a while since I last used node-gyp.

Copy link
Member

@joaocgreis joaocgreis left a comment

Choose a reason for hiding this comment

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

LGTM

Tested deleting the .lib after compiling, just to be sure. No problem.

The commit messages should obey the line sizes in https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit . Perhaps use build,win:.

@gibfahn
Copy link
Member

gibfahn commented Mar 5, 2017

For the commit messages, maybe something like:

build, win: use target_arch to link with node.lib

Using `target_arch` in addon.gypi to link against Node.js library. This
variable was written into build/config.gypi on the `configure` stage.

Do not copy node.lib into node_root_dir/Release or node_root_dir/Debug
on Windows, link it from node_root_dir/target_arch directory.

and

configure: use full path in node_lib_file GYP var

Set path to node lib in `$(Configuration)` dir when `--nodedir` option
is supplied, otherwise use value of `target_arch` variable.

@joaocgreis
Copy link
Member

@rvagg @bnoordhuis can I land this?

@pmed let me know if you object to me rewording the commit messages to @gibfahn 's suggestion above.

@bnoordhuis
Copy link
Member

@joaocgreis Yes, if you think it's good to go and CI is green.

joaocgreis pushed a commit that referenced this pull request May 15, 2017
Using `target_arch` in addon.gypi to link against Node.js library. This
variable was written into build/config.gypi on the `configure` stage.

Do not copy node.lib into node_root_dir/Release or node_root_dir/Debug
on Windows, link it from node_root_dir/target_arch directory.

PR-URL: #964
Reviewed-By: João Reis <[email protected]>
joaocgreis pushed a commit that referenced this pull request May 15, 2017
Set path to node lib in `$(Configuration)` dir when `--nodedir` option
is supplied, otherwise use value of `target_arch` variable.

PR-URL: #964
Reviewed-By: João Reis <[email protected]>
@joaocgreis joaocgreis closed this May 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants