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

make node-leveldb ready for node 0.8.x #38

Closed
wants to merge 10 commits into from
Closed

Conversation

pconstr
Copy link

@pconstr pconstr commented Jul 22, 2012

@gflarity
Copy link

I've submitted a pull request to pconstr that fixes this. Afterwards it npm installs!

don't delete the build dir, it's needed
@justmoon
Copy link
Collaborator

The title is misleading, node-leveldb works fine for me on Node.js 0.8.3. I think the main advantage of node-gyp is Windows support?

Speaking of which, I'm maintaining a Windows branch of leveldb - using this with minimal changes it should be possible to make this module compile on Windows. I might have a go at it if I have some time and there is interest.

@gflarity
Copy link

Long story short, it does not work under OS X if you install the node.js binaries instead of compiling them yourself. Gyp magically makes these problems go away.

@justmoon
Copy link
Collaborator

Ok, I've done some testing. This pull request currently disables Snappy, which is a regression imho. So I've created a new pull here: #39.

Would be awesome if you guys could test that one and see if it works for you. I'll leave this one open until I get some feedback on the new one. (Technically, we could merge this one first. Then mine would be simply a pull to update the deps and reenable Snappy.)

@gflarity
Copy link

I agree removing Snappy is less than ideal and if you have a fix might as well just do it all together. Testing...

@pconstr
Copy link
Author

pconstr commented Jul 24, 2012

good point, let's not drop Snappy

@justmoon
Copy link
Collaborator

People seem to be in agreement on keeping Snappy, closing in favor of #39.

@justmoon justmoon closed this Jul 24, 2012
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.

None yet

3 participants