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 support #248

Merged
merged 1 commit into from
Oct 18, 2017
Merged

Windows support #248

merged 1 commit into from
Oct 18, 2017

Conversation

webmakersteve
Copy link
Contributor

Do not merge yet. Still needs conditional adjustments based on architecture and debug/release.

@webmakersteve
Copy link
Contributor Author

Fixes #45

@andrewsensus
Copy link

I was hoping to experiment with this branch, but it isn't building for me. I notice "ls -1" and "find" in binding.gyp and librdkafka.gyp. Are these fulfilled by an implicit dependency (cygwin/msys/some SDK)? Is it desirable to write these in node instead, such as

      "sources": [ "<!@(node -e \"const fs = require('fs'); const path = require('path'); fs.readdirSync('src').filter(n => path.extname(n) === '.cc').forEach(n => console.log('src/' + n))\")", ],

@webmakersteve
Copy link
Contributor Author

That ls -l should be changed to an explicit list of source files rather than relying on a subshell. I'll adjust that.

Have not fully tested anything, and those dependencies were satisfied on my specific machine, although I don't plan on depending on them long term.

@webmakersteve
Copy link
Contributor Author

I just adjusted the sources list to just explicitly call out the source files

@matthax
Copy link
Contributor

matthax commented Sep 28, 2017

Any status on the other changes for the build process? I'm not familiar enough with gyp bindings to know how exactly to manage the build process, but I do see a few commands that will fail on windows, namely the find scripts. You might be able to replace them with dir *.c /s/b and get similar windows behavior.

I'd love to be able to use this project, the Kerberos support is awesome, but our tests need to pass on win, Linux and mac systems.

If there's anything I can do to help move this along I'd be happy to help out

@webmakersteve
Copy link
Contributor Author

Is it still running find for you when you try to install it? That should be isolated behind a condition of != windows.

I just need to test this on linux and mac before I merge it - so that's what I'm waiting for :)

@matthax
Copy link
Contributor

matthax commented Sep 28, 2017

Yeah I get an exception saying the file wasn't found during the build process. I just wiped the repo I cloned so give me a moment to try and re-clone and rebuild and I can get you some more details.

@matthax
Copy link
Contributor

matthax commented Sep 28, 2017

Here's the stracktrace I get when I try to install using npm install git://github.com/Blizzard/node-rdkafka.git#windows

C:\github\node-kafka\node_modules\node-rdkafka>if not defined npm_config_node_gyp (node "C:\Program Files\nodejs\node_modules\npm\bin\node-gyp-bin\\..\..\node_modules\n
ode-gyp\bin\node-gyp.js" rebuild )  else (node "" rebuild )
File not found - *.c
gyp: Call to 'find librdkafka/src -name *.c ! -name rdkafka_sasl*' returned exit status 2 while in C:\github\node-kafka\node_modules\node-rdkafka\deps\librdkafka.gyp. w
hile loading dependencies of binding.gyp while trying to load binding.gyp

edit I get the same error if I clone and build (I also have to execute npm install nan for some reason, might just be a me thing though) using node-gyp rebuild.

edit v2 It appears to be line 217 in librdkafka.gyp:

'sources': [
    '<!@(find librdkafka/src -name *.c ! -name rdkafka_sasl* )'
],

Which appears outside of the source restrictions.

@webmakersteve
Copy link
Contributor Author

@matthax This should now all be guarded behind a conditional. Can you try one more time?

@matthax
Copy link
Contributor

matthax commented Oct 18, 2017 via email

@matthax
Copy link
Contributor

matthax commented Oct 18, 2017

No dice, however, the issue may be related to my machine. I'm getting The build tools for v120 (Platform Toolset = 'v120') cannot be found. To build using the v120 build tools, please install v120 build tools. though I have the correct tools installed. I've tried to resolve it but I'm not having success. I'll give the install another go on my home computers since I've got one with VS2013 and one with VS2015.

I'll let you know what happens in those environments...

@webmakersteve
Copy link
Contributor Author

So one of the things I could NOT get around is that you need to be using the v120 toolset. Node gyp is not really good at selecting the correctly installed version, so I actually had to link MSBuild.exe to the correct one. This won't build on VS2015 because it uses a version of openssl precompiled by VS2013.

@matthax
Copy link
Contributor

matthax commented Oct 18, 2017 via email

@webmakersteve
Copy link
Contributor Author

I'm going to merge it now because getting to this step means it's working as far as I can tell, and there are more gyp changes I need to make.

I'm going to write some docs up on how to get it all working and add them to the README in the next commit.

@webmakersteve webmakersteve merged commit fc255c1 into master Oct 18, 2017
@webmakersteve
Copy link
Contributor Author

# Fun story export PATH="$PATH:/c/Program Files (x86)/MSBuild/12.0/Bin/"
# I wish there was a better way, but can't find one right now

@matthax
Copy link
Contributor

matthax commented Oct 18, 2017

I had previously specified the path for the 14.0 build tools in the path, which was why the link wasn't taking place. I've gotten through the actual configuration to the build process, I get This project references NuGet package(s) that are missing on this computer. Enable NuGet Package Restore to download them. which I think is due to the zlib package missing. I'm going to try to get that squared away and give this another go.

Changing my path is a small price to pay to be able to develop locally...

@webmakersteve webmakersteve deleted the windows branch October 18, 2017 21:34
@webmakersteve
Copy link
Contributor Author

I updated the README with instructions that got me up and running. Hopefully it helps :)

@matthax
Copy link
Contributor

matthax commented Oct 19, 2017

They did! And I got a fair bit farther, I cloned the repo locally to add the zlib package. Went into the win32 directory and ran nuget restore. Ran npm install ../node-rdkafka. The build returns success with 3 warnings, no problem there. I get this output though before the install completes though

win.cc
win_delay_load_hook.cc

c1xx : fatal error C1083: Cannot open source file: 'win.cc': No such file or directory [node_modules\node-rdkafka\deps\librdkafkacpp.vcxproj]

Inside node-rdkafka\deps\librdkafkacpp.vcxproj I do see the following:

<ItemGroup>
    <ClCompile Include="win.cc"/>
    <ClCompile Include="AppData\Roaming\npm\node_modules\node-gyp\src\win_delay_load_hook.cc"/>
  </ItemGroup>

Odd that win.cc doesn't have an absolute path but the other include directive does...
Was this an issue you encountered? Perhaps I'm missing a Windows SDK? I'll keep digging around but if any of this seems familiar let me know.

@webmakersteve
Copy link
Contributor Author

Sorry about that. I think I fixed that issue. I had to have a c++ file in there just to force things to compile but it was in the deps directory, so it was gitignored and I didn't catch it. Just updated the repo. Can you try once more pulling from github?

I wish I could try this stuff more but I don't have a Windows PC to work on currently. If I get some time I'll set up a VM so I can tests this, but hopefully this is the last change.

@matthax
Copy link
Contributor

matthax commented Oct 19, 2017

Okay so we've gotten past the win.cc so big props on that one, I've gotten the following error now:

Creating library node-rdkafka\build\Release\node-librdkafka.lib and object node-rdkafka\build\Release\node-librdkafka.exp
kafka-consumer.obj : error LNK2001: unresolved external symbol "class std::vector<class RdKafka::TopicPartition *,class std::allocator<class RdKafka::TopicPartition *> > __cdecl NodeKafka::Conversion::TopicPartition::FromV8Array(class v8::Local<class v8::Arra
y>)" (?FromV8Array@TopicPartition@Conversion@NodeKafka@@YA?AV?$vector@PEAVTopicPartition@RdKafka@@V?$allocator@PEAVTopicPartition@RdKafka@@@std@@@std@@V?$Local@VArray@v8@@@v8@@@Z) [node-rdkafka\bu
ild\node-librdkafka.vcxproj]
node-rdkafka\build\Release\node-librdkafka.node : fatal error LNK1120: 1 unresolved externals [node-rdkafka\build\node-librdkafka.vcx
proj]

As for not having a Windows machine, it's all good. I appreciate you making the effort.

@webmakersteve
Copy link
Contributor Author

Hmm. That's a new method I added a few commits ago and rebased into the windows PR. I'm going to need to get it compiling on a windows machine and toy around with it to find out why that isnt working.

The type mangling c++ does is making it kind of difficult to know what it wants that signature to be, but that should be available in common.cc as far as I can tell.

@matthax
Copy link
Contributor

matthax commented Oct 19, 2017

You've been able to successfully compile it on Unix though right? I haven't tried the latest set of commits but I had no issues before. Let me know what comes of your investigation and if you need me to retest stuff, I'd be happy to.

@webmakersteve
Copy link
Contributor Author

Correct. It's been compiling fine on Arch linux and Mac OS X. I'll let you know when i figure things out.

@webmakersteve
Copy link
Contributor Author

Turns out windows is a bit more picky with const correctness than GCC 😛

Pushed up a change to master that should fix that issue, and was able to successfully get it to compile on my Windows machine.

@matthax
Copy link
Contributor

matthax commented Oct 19, 2017

You wonderful person! I'm actually going to have to forgive you for being a Mei main because I'm so excited about this.

Compiled successfully on Windows. Going to test on CentOS7 shortly but I doubt I'll have any issues there. Thanks a ton.

edit works on CentOS7 :)

@matthax
Copy link
Contributor

matthax commented Oct 20, 2017

Is there any way we can bundle the nuget dependency for windows with the package? Originally I was going to write a post install script to run when the os was win32 but since the install will still actually fail, so no post install scripts will run.

I was thinking maybe just having an optional dependency on ../node-rdkafka for Windows users, and specify they need to clone the repo and restore, but it might be nice if I could just have a dependency on node-rdkafka listed and trust that the people running windows will get the library too.

I don't know if it would be as simple as not ignoring \deps\librdkafka\win32\packages or if perhaps there is some just prerequisite that nuget.exe is available and part of the build script for Windows is running nuget restore in the win32 directory?

If any of those sound like a potential solution, or you've got other ideas, let me know. I would be happy to loan a hand, make a PR, whatever, to make this easier on you.

Also, super happy with the performance so far :)

@webmakersteve
Copy link
Contributor Author

I don't really know the best way to do this. I know it's very annoying to install on Windows and I wish it was easier, but I don't really want to ship a binary as part of the library.

You could edit the configure script, which currently does nothing when running as windows, to pull those dependencies. Do you want to try that and submit a PR?

@matthax
Copy link
Contributor

matthax commented Oct 20, 2017

Yeah let me take a crack at that and see what I can do.

@matthax
Copy link
Contributor

matthax commented Oct 21, 2017

Got something, let me update the readme and I'll make a PR. Basically all I did was add another build action to the MS portion of the librdkafka.gpy file to restore. Windows folks will have to install nuget, I'll provide that info in the readme. That way no binaries are being shipped and the dependencies are still built on the machine during install. Should take care of #45 :)

@matthax matthax mentioned this pull request Oct 21, 2017
@d-sphinx
Copy link

Hi guys, I'm trying to build node-rdkafka on Windows 7 x64 (all current updates) + VS 2013 Update 5 and have some problems...

2017-10-31 20_36_05- _ c__windows_system32_cmd exe

MSBUILD : error MSB1009: файл проекта не существует. (Project file doesn't exist)
What's going on?

It also strange that npm yields those warnings BEFORE build process:
Warning: Missing input files:
c:_CODE\node-rdkafka-master\deps\librdkafka\win32\outdir\v120\x64\Release\zlib.dll
c:_CODE\node-rdkafka-master\deps\librdkafka\win32\outdir\v120\x64\Release\librdkafkacpp.dll
c:_CODE\node-rdkafka-master\deps\librdkafka\win32\outdir\v120\x64\Release\librdkafka.dll
c:_CODE\node-rdkafka-master\deps\librdkafka\win32\librdkafka.sln

Missing input binary files? Well, they supposed to be compiled! ))

I was able to successfully build a core librdkafka repository, both Win32 and x64 with appropriate OpenSSL packages I downloaded, as stated here: https://github.com/edenhill/librdkafka/blob/master/README.win32
But I can't do node-rdkafka ( Please help!

@webmakersteve
Copy link
Contributor Author

@d-sphinx Please open a new issue for this and don't comment on the PR. PR conversations should be for discussing the code changes :)

As far as your problem it looks like you downloaded this and did not clone the submodule. You need to git clone --recursive to get all the code.

@d-sphinx
Copy link

d-sphinx commented Nov 1, 2017

@webmakersteve OK, I'll open a new issue for that. BTW what is a "PR"? )

@matthax
Copy link
Contributor

matthax commented Nov 1, 2017

@d-sphinx Pull Request

@yunnysunny
Copy link
Contributor

yunnysunny commented Nov 13, 2017

For the current version of node-gyp using some feature of c++11, its minimum version of Visual Studio is VS 2015 ( see here ). I think the project can not been compiled with VS 2013 in node-gyp.

@mdenoun
Copy link

mdenoun commented Dec 19, 2017

I am using in my javascript project package json:
"dependencies": {
"node-rdkafka": "^2.2.1"
}
which compile node-rdkafka on windows when running npm install,

I set the following environment variable to be able to compile and resolve libraries libeay32MT.lib and ssleay32MT.lib related to OpenSSL

set CL=/I C:\OpenSSL-Win32\include
set LINK=/LIBPATH:"C:\OpenSSL-Win32\lib\VC"

I got errors related to resolving external symbol in open ssl like when it reach this line:
"C:\projects\code\kafka-nodejs\kafka-nodejs\node_modules\node-rdkafka\deps\librdkafka\win32\librdkafka.vcxproj" (default target) (3) ->
(Link target) ->

rdkafka_broker.obj : error LNK2019: unresolved external symbol ERR_remove_thread_state referenced in function rd_kafka_broker_thread_main [C:\projects\code\kafka-nodejs\kafka-nodejs\node_modules\node-rdkafka\deps\librdkafka\wi
n32\librdkafka.vcxproj] [C:\projects\code\kafka-nodejs\kafka-nodejs\node_modules\node-rdkafka\deps\librdkafkacpp.vcxproj]

rdkafka_sasl_scram.obj : error LNK2019: unresolved external symbol BIO_set_flags referenced in function rd_base64_decode [C:\projects\code\kafka-nodejs\kafka-nodejs\node_modules\node-rdkafka\deps\librdkafka\win32\librdkafka.vc
xproj] [C:\projects\code\kafka-nodejs\kafka-nodejs\node_modules\node-rdkafka\deps\librdkafkacpp.vcxproj]
...

It sounds related to how the lib (may be because it is MT mutili-threaded or static) are link.

Any suggestion to fix this issue or any direction will be much appreciated thank you

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.

6 participants