-
Notifications
You must be signed in to change notification settings - Fork 286
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
Windows linker issues #165
Conversation
Other platforms support dynamic lookup of symbols. On Windows, however, we always need to provide a .lib file with the node headers when linking the final dynamic library that will be used by Node applications. Signed-off-by: Nathan Sobo <nathan@github.com>
Signed-off-by: Max Brunsfeld <maxbrunsfeld@github.com>
|
||
# This name is arbitrary, but allows us to re-export the location of node.lib | ||
# to dependent packages so they can link on Windows. | ||
links = "neon-sys" |
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.
If this name is arbitrary, and we're hapy with the one chosen, maybe we could re-phrase this comment and add a link to the crates.io docs about this key.
# This name allows us to re-export the location of node.lib
# to dependent packages so they can link on Windows.
# http://doc.crates.io/build-script.html#the-links-manifest-key
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.
I'll tweak the comment but separately I've filed #169 to rename neon-sys
. Thanks for calling this out.
}, | ||
"devDependencies": { | ||
"nan": "^2.3.2", | ||
"node-gyp": "^3.3.1" |
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.
Do we expect node-gyp
to be installed globally?
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.
It's installed with node (technically, npm) so yes. Depending on a specific node-gyp version is an anti-pattern in most cases.
I get bug reports from time to time where people complain node-gyp stopped working for them and then it turns out they've pinned it to a five years old release.
|
||
[lib] | ||
name = "tests" | ||
crate-type = ["dylib"] | ||
|
||
[dev-dependencies] | ||
neon-build = "0.1.11" | ||
|
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.
So the neon-build
crate isn't required in production? Will production builds still succeed?
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.
Good catch. Will fix.
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.
Also filed neon-bindings/neon-cli#46 and committed a fix for the template in the CLI.
(Pulled out from #122.)