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

Initial 0.11 compatibility attempt, using new project: NAN #48

Merged
merged 6 commits into from
Aug 11, 2013

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jul 20, 2013

Node 0.11.4 introduced V8 3.20.2 which has a crazy amount of breaking API changes, maintaining compatibility across versions is a tricky prospect but I've been hacking away at a solution for cross-version compatibility for a little while now; I need it for a few native addon projects.

So say hello to NAN ( Native Abstractions for Node.js): https://github.com/rvagg/nan, which is really just a header file that can be included in a built and provide all the necessary abstractions to get 0.8, 0.10 and 0.11/0.12 compatibility without having to do any version sniffing yourself (in theory!).

This compiles and runs for me on Linux for 0.8. 0.10 and 0.11.4, I haven't tested any other platform and I worry about Windows... Previous versions of 0.11 won't work, there are incompatibilities that can't be tested because they haven't been incrementing NODE_MODULE_VERSION.

Note that if you get this running in 0.11 and run the LevelUP tests against this then you'll get a couple of failures:

  • a setTimeout problem caused by this Node bug that's impacting slow-stream but that's only for testing and that should be fixed in 0.11.5.
  • an sparse Array problem caused by this V8 bug (found thanks to LevelUP + Node 0.11!) in the tests that make big blobs of data using the Array.apply(null, Array(1024 * 100)) trick. Just reduce that 100 to a 90 and it'll run.

Would appreciate anyone who wants to have a play and look over the code. I'm happy to answer any questions anyone has on what I've done and even happier if anyone can find problems in what I've done.

Also relevant:

i.e. there may be an "official" solution to this problem, but I don't have the patience to wait for it—it's not likely to appear until much later in the 0.11 cycle because it'd be no small job, and I had this in progress anyway so I just finished it off.

@mcollina
Copy link
Member

This is impressive. Congrats.

@No9
Copy link
Contributor

No9 commented Jul 20, 2013

@rvagg windows build seems to be looking for 1.10.0 :(
Looks like its going to be a long night.
Will post my findings here

@rvagg
Copy link
Member Author

rvagg commented Jul 21, 2013

actual nan.h was missing from the original PR (it was a symlink), commited it now, upgraded to 0.1.0

@kesla
Copy link
Contributor

kesla commented Aug 4, 2013

@rvagg Wouldn't it make sense to merge this in sooner rather than later? So that all future development is based on this version rather than the old version...

@kesla kesla mentioned this pull request Aug 4, 2013
@juliangruber
Copy link
Member

merging this in now would be totally rad, then I can use level-co!

@kesla
Copy link
Contributor

kesla commented Aug 10, 2013

I've cherry-picked some commits from #50 so that we're having the latest version of nan in this PR.

There's sadly a test failing from time to time, so right now that's keeping us from being able to release this. I'm looking at it right now, and I'll let you know if I'm making any progress :)

@juliangruber
Copy link
Member

👯

@kesla
Copy link
Contributor

kesla commented Aug 10, 2013

Alright, so some progress!

I found that it's actually .get() that is failing, not .batch() - it's the .batch-test that uses .get.
I've narrowed it down to https://github.com/rvagg/node-leveldown/blob/node_0.11_compat/src/async.h#L32 not working as expected, the c-string is not persistent and can be written over when there's other things going on...

Pull request fixing this coming up, but @rvagg I'm counting on you keeping track on me :)

@kesla
Copy link
Contributor

kesla commented Aug 10, 2013

@rvagg Please take a look at the change that I did to get this to work, especially since it's involves a change in semantics to nan - maybe there's another way to fix this that doesn't involve the change to nan?

@kesla
Copy link
Contributor

kesla commented Aug 10, 2013

Hm...

When running the tests on node 0.11.4, not all tests are run... Am I the only one experiencing this? https://cloudup.com/c4tpJ1AgY0f

I've tried running the tests with plain node, like node test/del-test.js with the same result.

@juliangruber
Copy link
Member

maybe tap has problems

@juliangruber
Copy link
Member

except for a test that already failed in node 0.8.x, the tap testsuite passes on 0.11.4 on my mac.

rvagg added a commit that referenced this pull request Aug 11, 2013
Initial 0.11 compatibility attempt, using new project: NAN
@rvagg rvagg merged commit f77b4e4 into master Aug 11, 2013
@rvagg rvagg deleted the node_0.11_compat branch August 11, 2013 04:29
@rvagg
Copy link
Member Author

rvagg commented Aug 11, 2013

@0.7.0 in npm, let the world rejoice, I'll do a corresponding levelup & level now that'll pull them in. new abstract-leveldown is published too.

@rvagg
Copy link
Member Author

rvagg commented Aug 11, 2013

@No9 all good for me on Windows 0.10.5, I haven't bothered testing 0.11 but would love to hear your reports if you're testing 0.11.

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.

5 participants