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

Get neon building on windows #122

Closed
wants to merge 19 commits into from

Conversation

maxbrunsfeld
Copy link
Contributor

@maxbrunsfeld maxbrunsfeld commented Oct 24, 2016

Summary of changes

  • The explicit destructor call in NeonSys_Scope_Exit was causing a linker error with MSBuild 14.0.

    neon.obj : error LNK2001: unresolved external symbol "__declspec(dllimport) private: static void   __cdecl v8::HandleScope::operator delete(void *,unsigned __int64)" (__imp_??3HandleScope@v8@@CAXPEAX_K@Z) [C:\Users\Max\neon\crates\neon-sys\build\neon.vcxproj]
    C:\Users\Max\neon\crates\neon-sys\build\Release\neon.node : fatal error LNK1120: 1 unresolved externals [C:\Users\Max\neon\crates\neon-sys\build\neon.vcxproj]
    

    It looks like when MSVC compiles these types of explicit destructor calls, it emits a call to operator delete, even when delete isn't being used in the source. This causes the above linker error because v8::HandleScope::operator delete is private. Switching to the static method syntax seems to work around this problem.

  • The extern "system" calling convention specifier was preventing neon-sys from linking correctly in 32-bit windows builds. According to Rust's foreign calling convention docs:

    ... on win32 with a x86 architecture, this [extern "system"] means that the abi used would be stdcall.

    I believe that since the functions defined in neon.cc are marked as extern "C", they can always be linked into rust via the "C" calling convention, regardless of target.

  • In the neon-sys build script, we run npm as npm.cmd, and look for the neon.obj file in a different folder.

  • On windows, native node modules that use neon need to link against node.lib, similarly to how C++ modules built with node-gyp do so. To do this, neon-sys needs to capture the location of node.lib at build time, and modules that use neon must contain a build script that supplies this location to the linker.

    Since this build.rs file must be included in every module that uses neon, we've added code to generate this file as part of neon new: Support windows neon-cli#32.

  • I've added an appveyor.yml that tests neon on both 64 and 32-bit architectures. As you can see on our fork, the 64 bit build passes. Unfortunately, the 32 bit build fails because it is using the currently-published version of neon-cli which does not handle building 32-bit modules on a 64-bit system. Once Support windows neon-cli#32 is merged and a new neon-cli is published, this build will pass.

@maxbrunsfeld maxbrunsfeld force-pushed the windows-build branch 2 times, most recently from f1a95d9 to 6a4bc75 Compare October 24, 2016 22:54
For some reason, node-gyp never exits if we run `npm` commands without
the `--silent` option. Passing `--no-progress --no-color` and/or
capturing the output of stderr and stdout doesn't help.

This will still make the npm commands fail in case something goes wrong,
and won't suppress the node-gyp output when running cargo in verbose
mode.
...because they were preventing `neon_sys` functions from being included
into the `neon_sys` object file on Windows.

Signed-off-by: Max Brunsfeld <maxbrunsfeld@github.com>
@as-cii as-cii force-pushed the windows-build branch 4 times, most recently from 9564ef4 to 10273d4 Compare October 26, 2016 14:35
Antonio Scandurra added 2 commits October 26, 2016 16:37
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>
Nathan Sobo added 3 commits October 26, 2016 13:33
This fixes link errors when compiling against 32-bit Node on Windows.

Signed-off-by: Max Brunsfeld <maxbrunsfeld@github.com>
Signed-off-by: Max Brunsfeld <maxbrunsfeld@github.com>
Signed-off-by: Max Brunsfeld <maxbrunsfeld@github.com>
Antonio Scandurra added 2 commits October 27, 2016 11:02
Apparently, on Windows, cargo capitalizes all the environment variables
passed from the outside. Node-gyp, however, reads them in a
case-sensitive way, thus forcing us to downcase them before running it.
Antonio Scandurra added 2 commits October 27, 2016 19:44
Signed-off-by: Max Brunsfeld <maxbrunsfeld@github.com>
...because on newer Electron versions node-gyp stores the library as
`iojs.lib` and not as `node.lib`.
@dherman dherman self-assigned this Dec 19, 2016
@dherman
Copy link
Collaborator

dherman commented Dec 23, 2016

@maxbrunsfeld Would you be up for joining the Slack and chatting some time? I want to merge this amazing work but it probably needs a little coordination.

@dherman
Copy link
Collaborator

dherman commented Dec 23, 2016

@maxbrunsfeld I guess my main question right now is whether this PR obsoletes #123?

@dherman
Copy link
Collaborator

dherman commented Dec 23, 2016

OK I spent a bunch of time understanding this PR, and I think I will split it into 7 separate PRs, in this order:

Consistent calling convention

  • Use C calling convention

Windows compilation issues

  • Use statically-dispatched destructor call to work around MSVC bug
  • Disable WholeProgramOptimization along with LinkTimeCodeGeneration

Windows path issues

  • Fix object file path on windows
  • Fix npm path on windows

Windows npm issues (depends on: Windows path issues)

  • Run npm with --silent to prevent it from stalling on Windows

Windows linker issues (depends on: Windows npm issues, neon-bindings/neon-cli#32)

  • Resolve and pass node.lib to the linker on Windows
  • Remove build dependency on regex crate

Windows node-gyp issues

  • Prevent cargo build from hanging on Windows
  • Temporarily build node bindings only against ia32 for testing
  • Base node-gyp --arch flag on Cargo target architecture
  • Downcase npm environment variables to ensure they are read by node-gyp
  • Parameterize node_lib_file in addition to node_root_dir

AppVeyor

  • Add appveyor.yml
  • Test on both x86 and x64 platforms on appveyor
  • Use Visual Studio 15.0 on Appveyor
  • Install the correct node.js for the current platform on appveyor
  • Build on x64 on AppVeyor but compile for x86 as well
  • Test 32bit and 64bit compilation on a x86 platform on AppVeyor

I intend to preserve the authors, but splitting the work up in these chunks will help me to understand how it all works as well as to ensure CI keeps passing as we go along.

@dherman
Copy link
Collaborator

dherman commented Jan 31, 2017

OK, all the commits in this PR have landed, so I'm going ahead and closing.

Thank you to @as-cii, @nathansobo, @maxbrunsfeld, and @EdShaw, so much! I never would've figured this out on my own.

@dherman dherman closed this Jan 31, 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.

2 participants