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

use NAN for Node 0.8->0.11+ compatibility #77

Merged
merged 1 commit into from
Dec 6, 2013
Merged

use NAN for Node 0.8->0.11+ compatibility #77

merged 1 commit into from
Dec 6, 2013

Conversation

rvagg
Copy link
Contributor

@rvagg rvagg commented Dec 6, 2013

Node 0.11 and onwards uses the new V8 which has huge breaking API changes. NAN is designed to make it easy to support V8 and Node across versions 0.8 to 0.11 and onwards without the need for branching macros. NAN is keeping up-to-date as newer versions of Node 0.11 are released and new breaking changes come upstream from V8.

This PR adds NAN support for compiling against the latest Node 0.11; tests all pass. Would be good to get this in npm before the impending 0.12 release.

Let me know if you have any questions.

@lloydwatkin
Copy link
Contributor

Looks great! Thanks very much.

Have now dropped nodejs < 0.8 support from this project and confirmed all the tests pass.

lloydwatkin added a commit that referenced this pull request Dec 6, 2013
use NAN for Node 0.8->0.11+ compatibility
@lloydwatkin lloydwatkin merged commit a8abb06 into xmppo:master Dec 6, 2013
@lloydwatkin
Copy link
Contributor

Whilst test run on OSX for me they are not completing successfully on travis CI. There's also an issue with windows (#78).

Would you mind taking a look?

https://travis-ci.org/node-xmpp/node-expat/builds/15029182

Its an OS permission denied error. If this is an issue with Travis please let me know and I'll raise an issue with them.

Thanks!

@lloydwatkin
Copy link
Contributor

Have reverted this merge for now as its causing me issues on a server (ubuntu-based).

lloydwatkin pushed a commit that referenced this pull request Dec 6, 2013
This reverts commit a8abb06, reversing
changes made to 899a409.

Conflicts:
	package.json
@rvagg
Copy link
Contributor Author

rvagg commented Dec 6, 2013

Oh man. Node 0.11 on Travis is so broken at the moment; I've given up on it for most of my projects.

It's one of the reasons I wrote DNT (Docker Node Tester). Here's output from node-expat through DNT:

Node@master  : PASS wrote output to /tmp/expat-dnt-master.out
[email protected] : PASS wrote output to /tmp/expat-dnt-v0.11.9.out
[email protected]: PASS wrote output to /tmp/expat-dnt-v0.10.21.out
[email protected] : PASS wrote output to /tmp/expat-dnt-v0.11.8.out
[email protected]: PASS wrote output to /tmp/expat-dnt-v0.10.22.out
[email protected] : PASS wrote output to /tmp/expat-dnt-v0.8.26.out
Took 27s to run 6 versions of Node

And here's the setup I've used to get it running for node-expat if you're interested: https://github.com/rvagg/node-expat/compare/nan...dnt

As for the specific Travis error, I haven't actually seen this one before but my guess would be that it's a transient error that may not happen if you try again--although you might see something different. Mostly the errors I see are from npm bugs, not even getting to the testing phase, it's all about setup.

@lloydwatkin
Copy link
Contributor

Have tested again on my linux box and it works just fine. Have reapplied changes in e2f694d

If travis errors again I'll raise an issue with them. Thanks!

Pushed out as 2.1.2

@lloydwatkin
Copy link
Contributor

FYI got travis working using this config #79

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants