Conversation
|
Does this mean that i can get this working on windows? |
|
It builds fine for me. Follow the intructions in up in the PR description, and it should work for you too. |
|
Sorry, been busy so I haven't been able to look at it. And, I'm going away (omg no technology) this weekend, but will look at it when I get back. |
|
There are a few things I don't like about the code changes. I am not sure if it was ever made clear why we have to have the EncodeBinary method versus the current Encode method. There are also some other changes |
|
No need to include the nodeunit change as I just pushed a change which does it in a slightly different way. (Don't like the rebuild for every npm test invocation if not needed) |
|
@seanmonstar Would like the EncodeBinary method to go away. Encode is in the node.h header so I don't see why it can't be used. If you could check on windows that it works without that I think that would be quite swell. |
|
@shtylman replacing |
|
@seanmonstar we shall see what is said about that: nodejs/node-v0.x-archive#3811 Ideally they will export it. Then the patch for windows will be even smaller :) |
|
@shtylman that'd be great, but it would also make the minimum version of bcrypt be nodejs 0.8.6 or whenever it landed. |
|
Looks like that function is now marked as external in the latest master. If you could give that a shot without the EncodeBinary function I think the modified pull request will be good to go. |
|
It's in the 0.8.5 branch, so alter the package.json to have |
|
No need for that. This is for windows only so I don't want to lock down the package.json this tightly for everyone. I realize it is an annoyance for windows testing, but it will at least begin to work :) |
|
There's the updated and rebased PR, using Encode from 0.8.5. |
|
Eh, that's what I get for force pushing in short succession: travis-ci ran |
|
Please squash these commits. There is no need to have a commit which adds EncodeBinary and then one that removes it right away. Really all three of these commits can become one windows compat commit |
|
OK, I converted all the changes into a single commit. |
- Win32 code - updating binding.gyp to work with Windows expects user to have downloaded OpenSSL-Win32 from http://slproweb.com/products/Win32OpenSSL.html and installed in default location, C:/OpenSSL-Win32
windows support Thank you to everyone who persisted and helped make this happen :)
|
Well, I never thought it would happen but the tests all pass on my version of windows. BUT, windows support requires v0.8.5+ of nodeJS. Still, this is great. I'll add some docs and stuff either tonight or tomorrow and get a release out. |
5 participants
Then OpenSSL website suggests that the library be downloaded for Windows from http://slproweb.com/products/Win32OpenSSL.html. This should probably be the recommended method for all Windows users.
This expects that to have been downloaded, and installed in the default location,
C:\OpenSSL-Win32.