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

node v0.11.* build #39

Closed
navaru opened this issue Feb 26, 2014 · 68 comments
Closed

node v0.11.* build #39

navaru opened this issue Feb 26, 2014 · 68 comments

Comments

@navaru
Copy link

navaru commented Feb 26, 2014

Any plans for supporting node v0.11.*?

@barrysteyn
Copy link
Owner

Hi

Yup, I will try do that maybe over this weekend.

On 26 February 2014 07:53, Eugen Tudorancea [email protected]:

Any plans for supporting node v0.11.*?

Reply to this email directly or view it on GitHubhttps://github.com//issues/39
.

@navaru
Copy link
Author

navaru commented Feb 26, 2014

Thanks, I appreciate it

I don't know C++ and can't help you, tho' I so bcrypt and BSON (which also need to build) use nan maybe it helps

@thehydroimpulse
Copy link

Definitely loving to hear this. I know several native Node modules are waiting till 0.12 (whenever that'll land) to update, since the C++ API is constantly changing with new V8 versions.

@barrysteyn
Copy link
Owner

Yeah, that was my thinking when I decided to skip support for 0.11 - I
mean, the changes are really breaking.

The way to do things is to use macros, but as Eugen pointed out, the nan
project provides a lot of these. I am hesitant to use Nan due to the
changes that I will probably have to make when 0.12 comes out, but then
again, if I am to support 0.11 with my own macros (the other alternative),
then I am in the same boat.

I will see how long it takes me this weekend. I am really glad you are
finding the module useful.

On 28 February 2014 00:40, Daniel [email protected] wrote:

Definitely loving to hear this. I know several native Node modules are
waiting till 0.12 (whenever that'll land) to update, since the C++ API is
constantly changing with new V8 versions.

Reply to this email directly or view it on GitHubhttps://github.com//issues/39#issuecomment-36331142
.

@barrysteyn
Copy link
Owner

Hi All

Just some feedback: I have started with this task. I have elected not to use the nan project, but instead I am going to make my own macros. I consider this slog work: Read the new v8 api, make a macro that fit will accomodate both version! Even though it is slog work, it is still time intensive. With this in mind, I decided to put priority on something that people have been begging me to do for ages - a windows build. I am not a windows fan (actually, I have not used the OS since 1999), but I have been getting so much requests, that I do consider it more important. Therefore, I am giving this issue a rest until next weekend when I think I will have the Windows build under my belt.

As an aside: If you guys have any additional feedback, I would love to hear it. I really get a kick out of hearing what people use my software for.

All the best
Barry

@rehno-lindeque
Copy link

Hi I would certainly like to see 0.11 support soon :) Node 0.12 is apparently pretty close https://twitter.com/nodejs/status/462245114593570816

@barrysteyn
Copy link
Owner

Yeah, I plan on doing this come the end of August (at the latest).

On 29 June 2014 11:03, Rehno Lindeque [email protected] wrote:

Hi I would certainly like to see 0.11 support soon :) Node 0.12 is
apparently pretty close
https://twitter.com/nodejs/status/462245114593570816


Reply to this email directly or view it on GitHub
#39 (comment)
.

@rehno-lindeque
Copy link

Right, thanks 👍

@vhain
Copy link

vhain commented Oct 11, 2014

My PR(pdxmholmes/cpuid-node#3) to make build successful with node 0.11.x for cpuid-node(https://github.com/brainling/cpuid-node), which is dependent for node-scrypt, was merged.

Now it is only node-scrypt to be fixed for node 0.11.x!

@barrysteyn
Copy link
Owner

Wow, thanks. I'll look into this. I'm also in the process of working on a
native node implementation of Scrypt. This should solve most problems, but
will probably only be ready on about two months or so.
On Oct 11, 2014 12:13 PM, "vhain" [email protected] wrote:

My PR(pdxmholmes/cpuid-node#3
pdxmholmes/cpuid-node#3) to make build
successful with node 0.11.x for cpuid-node(
https://github.com/brainling/cpuid-node), which is dependent for
node-scrypt, was merged.

Now it is only node-scrypt to be fixed for node 0.11.x!


Reply to this email directly or view it on GitHub
#39 (comment)
.

@underbluewaters
Copy link

Any news on this? I'm trying to decide whether to use another library.

Sorry I know these messages are annoying, but I have no c++ chops to contribute.

@tellnes
Copy link

tellnes commented Feb 5, 2015

You should reconsider not using the nan project. nan does not solves all issues, but it solves a lot of them. Also, in the io.js world there is talk about shipping nan with io.js (nodejs/node#725 (comment)).

@barrysteyn
Copy link
Owner

Yeah - I might do that. But I am still trying to go for a native node solution, then none of these addon problems will be present.

@tellnes
Copy link

tellnes commented Feb 8, 2015

nodejs/node#456

@barrysteyn
Copy link
Owner

@tellnes I really like the IO project, and I want to support it. If you can do some investgation or help me, I would be keen to get this going. Does this really only need nan?

@tellnes
Copy link

tellnes commented Feb 9, 2015

I can't answer you if all you need is nan because that is a question about if nan exposes the apis you need to do the binding. But if nan exposes those apis, then that should be all you need.

I think the plan is that if v8 changes its api and it can be fixed for node-scrypt by only using a patched version of the nan module, then io.js will do a minor version bump. If it can not be fixed in nan and you need to patch node-scrypt, then io.js will do a major version bump.

@barrysteyn
Copy link
Owner

Hmmm. Seems reasonable. Tell me something, has io got more excitement at
the moment compared to node?

On Mon, Feb 9, 2015, 8:44 AM Christian Tellnes [email protected]
wrote:

I can't answer you if all you need is nan because that is a question about
if nan exposes the apis you need to do the binding. But if nan exposes
those apis, then that should be all you need.

I think the plan is that if v8 changes its api and it can be fixed for
node-scrypt by only using a patched version of the nan module, then io.js
will do a minor version bump. If it can not be fixed in nan and you need to
patch node-scrypt, then io.js will do a major version bump.


Reply to this email directly or view it on GitHub
#39 (comment)
.

@tellnes
Copy link

tellnes commented Feb 9, 2015

node.js v0.12 was just released. It is far behind io.js. But as of now both are still api compatible. nan does support both io.js and node.js. I think people still believes in the possibility of a merge in the feature, which would be the best for the community. If that is made possible, then you can look at io.js as a bug pull request to node.js.

io.js is how a lot of people wants node.js development to work. Including most of the core contributors expect for those working for Joyent.

@tellnes
Copy link

tellnes commented Feb 9, 2015

Ref if nan is all you need: If nan does not expose the api you need, than you either open a pull request to nan to make it support it or you use the v8 api directly and hope you does not get a new issue like this one if v8 changes its api.

@barrysteyn
Copy link
Owner

Okay, just for all to know: I am starting to actively work on this issue.

@BrandonZacharie
Copy link

+1 for 0.12 support

@dmmalam
Copy link

dmmalam commented Feb 11, 2015

+1

@mike-marcacci
Copy link

Just adding my enthusiastic +1 for node v0.12 / io.js 1.2.x support. This is the last package I need updated before I can make the switch in all of my projects :)

@barrysteyn
Copy link
Owner

Its coming @mike-marcacci I need to go over the entire source code. I got about half way over the weekend.

@mike-marcacci
Copy link

@barrysteyn that is so great to hear! Thanks so much for your weekends

@mariusk
Copy link

mariusk commented Feb 19, 2015

+1 thx

@barrysteyn
Copy link
Owner

Just a quick update for everyone: I have been diving into nan. There are many things I need to use that are not documented, so I am forced to look at the nan source code. Things are coming along quite well: I may finish things by the end of this weekend. If I am not able to finish by the end of this weekend, I should finish by the end of next weekend.

@barrysteyn
Copy link
Owner

Another quick update: It turns out that I underestimated the size of the work needed to accomplish this task (I think that was why I put it off in the first place). Seeing as this is crypto code we are dealing with, I need to be very careful how things are implemented. The good news is that things are coming along. In my last comment, I said I should finish by the end of next weekend, and I still hope to keep that promise, but being honest, I may not be able to do this.

Lets see...

@mike-marcacci
Copy link

Thanks so much for all your work and time @barrysteyn! If you're ever in Southern California... I owe you a beer. If you think there's a way for others to contribute without stepping on your toes or interfering with your vision, let us know!

@tellnes
Copy link

tellnes commented Mar 2, 2015

No problem. I'm just reporting it.

@barrysteyn
Copy link
Owner

@tellnes I have removed engineStrict, replaced it with engine-strict. Thanks.

@tellnes
Copy link

tellnes commented Mar 2, 2015

They are removing support for engineStrict in package.json and is saying you should use npm config instead. In practice that means the the decision is in the hands of the user and not the module author. See npm/npm#7171 (comment) for details.

@barrysteyn
Copy link
Owner

Hmmm - can npm config get this from package.json? If so, I am going to leave my change be. If not, then I will remove it.

@tellnes
Copy link

tellnes commented Mar 2, 2015

Not that I'm aware of, but I'm not an expert on npm.

@dpatti
Copy link

dpatti commented Mar 2, 2015

I forgot about the encoding on the buffer in my original repro, but even with it (I updated my last comment) I am still getting a segfault on 0.12 but not 0.10. I will admit my C++ debugging chops are not what they used to be, so in my naive approach, I have isolated the problem here:

  HashInfo(Handle<Object> config) : hash_ptr(NULL), key_ptr(NULL), keySize(0) { 
    std::cout << "HashInfo()\n";
    v8::Local<v8::String> a = NanNew<String>("_hashEncoding");
    std::cout << "1\n";
    v8::Local<v8::Value> b = config->GetHiddenValue(a);
    std::cout << "2\n";
    // The following line causes a segfault:
    v8::Local<v8::Uint32> c = b->ToUint32();
    std::cout << "3\n";
    int d = c->Value();
    std::cout << "value " << d << "\n";
    // Snip...

If I run this same code with _keyEncoding instead, it works. It looks to me like CreateScryptConfigObject does not initialize _hashEncoding, but I also have no idea how that would only cause a segfault for me.

@barrysteyn
Copy link
Owner

Hi @dpatti

Thanks for doing the investigation. Short story: I know why it is segfaulting, and I have fixed the problem (get the latest from the nan-update branch).

Long story: In order to hide encoding settings from JS land, I use something called SetHiddenValue (it is a V8 function) that hides properties from users, it can only be accessed by the C++ api. In previous versions of V8, if you accessed a hidden value that was not present, it just defaulted to 0, which turns out to be a valid ENUM value for Node's encoding (I think it is ascii). But the latest version of V8 causes a segfault, and so I need to intialize it. It's acting weird though because I did not initialize the HashEncoding (I forgot about it) yet it was working with me and others.

Anwyways, it's good practice to initialize your things, and this fix should sort you out. Please can you test it for me again.

Thanks

@dpatti
Copy link

dpatti commented Mar 2, 2015

My original repro case is working now, and all of our tests are passing. 👍

@dpatti
Copy link

dpatti commented Mar 2, 2015

I am running into another issue. We keep modules checked in to source control after installing with --ignore-scripts. We always do an npm rebuild to compile all dependencies. In the case of the newer scrypt, we have this problem:

$ npm install barrysteyn/node-scrypt#nan-update --ignore-scripts
[email protected] node_modules/scrypt
├── [email protected]
└── [email protected]

$ npm rebuild

> [email protected] install /home/doug/dev/scrypt-install-test/node_modules/scrypt
> node-gyp rebuild

child_process: customFds option is deprecated, use stdio instead.
cpuid.node seems to not have been built. Run npm install.
gyp: Call to 'node sse-discover.js' returned exit status 0. while trying to load binding.gyp

What happens is cpuid prints to stderr, which causes gyp to stop and fail regardless of the exit code. And honestly, I don't know how to fix this. I don't know of another module that used a native dependency during its own build process. I'm sure it could be fixed in npm by reversing the order of the rebuild to do dependencies before dependents, but I have no idea how long that would take. Do you have any insights on what could be done to mitigate this within scrypt?

@gavinhungry
Copy link

Cloned the nan-update branch, built successfully and all tests (216) passed with node v0.12.0 (Linux). Thanks for all your work!

@alemhnan
Copy link

alemhnan commented Mar 2, 2015

I just tried to use the nan-update branch on heroku and it worked without strange problems. Although locally I do have experienced some of the problems mentioned above in this thread. In heroku I explicit included cpuid, not sure if that was needed.
I tried both node versions (0.10.36 and 0.12.0), locally and on heroku as well.

Keep up the good work!!

@barrysteyn
Copy link
Owner

@dpatti This seems to be a standard problem with cpuid. I am going to change the way the module detected SSE2 availability thereby getting rid of cpuid, but that is not going to happen for this issue.

@barrysteyn
Copy link
Owner

@timshadel Hi Tim - please can you test the latest nan-update branch for me and tell me if the performance is still okay for you?

@timshadel
Copy link

The performance looks just fine to me. It is slower, but only slightly, so it's well within the timeframes we need to hit.

@barrysteyn
Copy link
Owner

@timshadel I have an idea why it is slower (that's why I asked you to test it). Let me apply some fixes and see if it makes it faster. I will probably move my dev to a new branch for this. I will try finish this week with the fixes

@timshadel
Copy link

Interestingly, when I use the nan-update branch on node 0.10.35 the performance is essentially equal to the current version of scrypt (0.10 only), but when I run it on 0.12.0, the performance drops slightly. So it may not be scrypt#nan-update at all, but something else in my setup.

@barrysteyn
Copy link
Owner

@timshadel Hmmmm, then I don't think my fix will work. It could be a node 0.12 thing. By how much does the performance drop by?

@timshadel
Copy link

OK. After isolating the scrypt verification, I collected timing data on [email protected], [email protected], and [email protected]. I did 3 runs. For each run I generated 300 hashes, and then looped over them and timed how long each successful verify call took under each environment. I then used a two-sided t-test against the pairs of environments to assess if the timing samples were statistically the same. Most of the time they came out statistically the same. On the times they didn't their raw statistics (medians, avg, range, etc) were very close (< 2ms off). So those times most likely were disrupted by OS/processor/background stuff, in my opinion.

Whatever the slight slowdown was I saw in my tests, it wasn't due to scrypt. Its raw performance seems very stable and usually indistinguishable from the former version.

@barrysteyn
Copy link
Owner

@timshadel Excellent. This is good to hear.

@barrysteyn
Copy link
Owner

This issue is now officially closed in 223f039

@thehydroimpulse
Copy link

@barrysteyn Does this bring support for 0.12 only or also io.js?

@barrysteyn
Copy link
Owner

Should work for io.js as well (although I have not tested it). Looking at the above comments, you can see other people have tested it and it works fine. But I aim to support io.js - infact, I am much more keen on it than I am of node. In short: Yes, it will support io

@taoeffect
Copy link

Hey @barrysteyn (and everyone else helping with this project), I just want to say that I really appreciate the work you guys are doing on this. It's an important project, so thank you for the recent activity, fixes, etc.! 👍

@c-geek
Copy link

c-geek commented Mar 4, 2015

@barrysteyn All tests passed for me on nan-update too. Thank you very much for your work, you've come along with the right time!

@taoeffect You again!

@barrysteyn
Copy link
Owner

Hi Everyone. Thanks for the support. Knowing that people are using my software makes all the difference. If you want to say thanks, the best way I can think of is to just send me an email ([email protected]) and tell me how you guys are using my software.

@dmmalam
Copy link

dmmalam commented Mar 4, 2015

Thanks for building this.

Dharmesh Malam

On Wed, Mar 4, 2015 at 3:03 PM, Barry Steyn [email protected]
wrote:

Hi Everyone. Thanks for the support. Knowing that people are using my
software makes all the difference. If you want to say thanks, the best way
I can think of is to just send me an email ([email protected]) and
tell me how you guys are using my software.


Reply to this email directly or view it on GitHub
#39 (comment)
.

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

No branches or pull requests