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

Windows Compilation Fixes #221

Merged
merged 5 commits into from
Aug 18, 2017
Merged

Conversation

hone
Copy link
Contributor

@hone hone commented Aug 11, 2017

This fixes linking neon on Windows to account for rust-lang/rust#38850.

It also replaces the gyp <(target_arch) var, so rustc can find the node.lib directory.

-l is only intended to communicate the names of libraries

See rust-lang/rust#38850 for more details.
@ctaggart
Copy link

@hone, need help getting this to work on AppVeyor?

@dherman
Copy link
Collaborator

dherman commented Aug 16, 2017

@ctaggart Thanks for that offer! I just tested this on my Windows machine and it's still failing so I don't think it's specific to AppVeyor. We're just dealing with some finicky issues satisfying the linker in Windows with the right incantations. (If you have suggestions about that let me know!) I'm gonna dig into this and see what I can figure out.

@dherman
Copy link
Collaborator

dherman commented Aug 17, 2017

OK, as it turns out, it was only failing on my machine because I had made a mistake with my environment. Now it's building fine.

@ctaggart So this is indeed an AppVeyor mystery. Any help you can offer would be awesome!

@ctaggart
Copy link

@dherman What was the mistake in your environment? I opened PR #224 with what I had attempted last night, but it doesn't work yet.

npm 5.0.3 changed the CLI output so '-Dnode_lib_file' returned the path of
the file vs just the file name.
NODEJS_VERSION: "8"
RUST_TOOLCHAIN: stable-x86_64-pc-windows-msvc
- PLATFORM: x86
NODEJS_VERSION: "8"

Choose a reason for hiding this comment

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

This matrix looks good & looks like AppVeyor is green!
🍀 🍀 🍀 🍀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, yeah I wanted to test both LTS + latest and the old configuration was doing a 2x2 matrix where you'd test x64 toolchain on a x86 machine.

@ctaggart ctaggart mentioned this pull request Aug 18, 2017
@dherman
Copy link
Collaborator

dherman commented Aug 18, 2017

@ctaggart I had installed something with chocolatey that I thought was the same as the Windows Build Tools, but it wasn't. So the build was picking up GNU link in the path instead of the Visual Studio linker.

@dherman
Copy link
Collaborator

dherman commented Aug 18, 2017

This is fantastic. @hone thank you so much for your persistence. I'm planning to add some comments and clarifications to the build logic, which has definitely gotten intricate enough to be hard to maintain, but that's on me. :)

@dherman dherman merged commit 0cd434c into neon-bindings:master Aug 18, 2017
@hone hone deleted the windows_compilation branch August 18, 2017 22:37
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.

None yet

3 participants