-
-
Notifications
You must be signed in to change notification settings - Fork 177
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
upgrade to NAN v2 #195
upgrade to NAN v2 #195
Conversation
I had a look at the the diff and it all looks good (which should come as no greater surprise, since it compiles). Minor details are that
This error appears to come from a double free of a |
Great work @rvagg! I have little time now, but I would like to get some time in the C++ land for Nan 2 (given it's going to break some of my stuff too). |
Cool. Having a look to see if I can get some more information on why it crashes. |
I tracked down a couple of bugs, one of them was this when cleaning up hanging iterators: v8::Local<v8::Value> argv[] = {
Nan::New<v8::FunctionTemplate>()->GetFunction() // empty callback
};
Now, this is what's left on io.js v2:
Something about smalloc, a double-free of a buffer perhaps? I have no idea. Anyway, on io.js v3 it works now, compile & test passes fine. I haven't tested anything below v2 yet. |
OK, a couple more minor changes and this compiles on 0.10, 0.12 and io.js 2 & 3. But I'm getting segfaults on all but io.js v3. This is the 0.10 form of the error (the others all come from smalloc):
I got not ideas at this stage, happy for others to have a look too! |
2c291ec
to
459ca81
Compare
I find it interesting that |
All fixed, @kkoopa pointed out that
|
Good! I was SOL finding what was causing these problems. |
9535f10
to
bdb6bd2
Compare
package.json updated for [email protected], this builds and tests pass using compiled and tested on 0.10, 0.12, 1.x, 2.x and 3.x (pre, supposed to be released today) Imma leave this to someone else to merge and release this, @ralphtheninja? |
@rvagg Np. I need to check with @mafintosh as well, since he builds all binaries :) |
@ralphtheninja i'm ready to do the builds so lets merge this |
i cannot get this build on ubuntu (travis fails as well) on iojs 3.0.0. does anyone know what the issue might be? here is my output
|
It uses the wrong NAN version for some reason. I checked out the branch and it built just fine on 14.04. Nevermind snappy, it was the actual libsnappy. |
@kkoopa oh yea it works for me now as well. i probably messed something up before. |
@mafintosh this looks similar to a problem I had with @kkoopa the snappy used here is not node-snappy, it's upstream snappy compiled for this package, the two projects are unrelated |
@rvagg i wasn't using the right branch on ubuntu hehe so thats why i didn't work. i wonder why it doesn't work on travis though :/ |
@rvagg actually its consistently failing for me on ubuntu using iojs 2.4.0 now when compiling
|
@rvagg iojs 3.0.0 works though... |
ah, I think this is because I didn't catch up to the
I think it might be as easy as replacing every |
@rvagg i'll try to fix it |
75edd30
to
89df0bd
Compare
@rvagg if you want to use prebuild to compile another time use the I've fixed this now for all previous versions of node/iojs and it compiles on iojs 3.0.0 on linux/mac for me as well now. travis still complains with some very weird errors when testing on 3.0.0 though. Unless someone feels strongly about not merging this I'm gonna merge + release this later today. |
There remains an unidentified memory leak. nodejs/nan#389 |
Sorry, I've been out in the bush with limited access to internet. Should have better connection now. @mafintosh +1 for merge |
can we publish this? |
this should be a patch release, right? |
yea. lets publish it and i'll do some builds |
@juliangruber by "yea" i mean that it should be a patch release. |
@mafintosh great! published as 1.4.1 |
I haven't been involved in the NAN v2 work and it's a major departure from <= v1. Overall the approaches are quite nice because it makes it feel much more like working with V8 than some hacked macro soup (well done @kkoopa et. al.). This is my first attempt at actually using NAN v2 and I'm trying this because I'm pushing to get io.js v3.0 out. We have release candidates dropping at https://iojs.org/download/rc/ but getting NAN v2 into npm is the holdup because it's going to break every native addon in npm and the delta between NAN v1 and v2 is so big that it's going to be major trauma for all and I'm trying to help find an approach to ease the pain (likely it'll be about documentation and a tutorial of sorts).
Anyway, I've finally managed to get it to compile, I haven't checked that it works on anything other than io.js v2 and io.js v3-rc.3 and I have segfaults in iterators still so this absolutely isn't ready to merge. If someone has time to help that would be greatly appreciated, I feel like I'm wasting valuable time on C++ rabbit holes that could be better used trying to get io.js v3 out!