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

Upgrade to nan v1.8.4 for io.js v2.0.0 compatibility #519

Merged
merged 1 commit into from
May 9, 2015
Merged

Upgrade to nan v1.8.4 for io.js v2.0.0 compatibility #519

merged 1 commit into from
May 9, 2015

Conversation

imyller
Copy link
Contributor

@imyller imyller commented May 5, 2015

Compiling fails with io.js 2.0.0:

In file included from ../src/serialport.cpp:3:
In file included from ../src/serialport.h:5:
In file included from ../node_modules/nan/nan.h:80:
In file included from ../node_modules/nan/nan_new.h:190:
../node_modules/nan/nan_implementation_12_inl.h:181:66: error: too many arguments to function call, expected at most 2, have 4
  return v8::Signature::New(v8::Isolate::GetCurrent(), receiver, argc, argv);
         ~~~~~~~~~~~~~~~~~~                                      ^~~~~~~~~~
/Users/-/.node-gyp/2.0.0/deps/v8/include/v8.h:4188:3: note: 'New' declared here
  static Local<Signature> New(
  ^
1 error generated.

This PR updates nan to version ~1.8.4 allowing installation of node-serialport with latest io.js.

@imyller
Copy link
Contributor Author

imyller commented May 5, 2015

Note: this PR does not address the usage of deprecated nan features causing compiler warning. This just allows installation and compilation with io.js v2.0.0

@imyller
Copy link
Contributor Author

imyller commented May 5, 2015

Referencing nodejs/node#1620

@imyller
Copy link
Contributor Author

imyller commented May 7, 2015

Any way this (or similar) could be merged soon? Currently io.js 2.0.0 is not supported.

@voodootikigod
Copy link
Collaborator

Honestly I am not super worried about io 2.0.0 so until this can be confirmed to not affect node 0.10.x it will not be merged. And that has to be true for all platforms as outlined when io original support:

Given the wide use of this by many people and it being a native module, for right now I am going to let this sit a couple days until it can be verified at a minimum on the following platforms (assuming latest 'stable' release of the framework (node 0.10 and 0.12 && io 1.x.x and 2.x.x):

  • Windows 10 - io.js 1.0.0
  • Windows 10 - io.js 2.0.0
  • Windows 10 - node.js 0.10.x
  • Windows 10 - node.js 0.12.x
  • Windows 8 - io.js 1.0.0
  • Windows 8 - io.js 2.0.0
  • Windows 8 - node.js 0.10.x
  • Windows 8 - node.js 0.12.x
  • Windows 7 - io.js 1.0.0
  • Windows 7 - io.js 2.0.0
  • Windows 7 - node.js 0.10.x
  • Windows 7 - node.js 0.12.x
  • Ubuntu Snappy 15.04 - io.js 1.0.0
  • Ubuntu Snappy 15.04 - io.js 2.0.0
  • Ubuntu Snappy 15.04 - node.js 0.10.0
  • Ubuntu Snappy 15.04 - node.js 0.12.0
  • Ubuntu Linux 14.04 - io.js 1.0.0
  • Ubuntu Linux 14.04 - io.js 2.0.0
  • Ubuntu Linux 14.04 - node.js 0.10.0
  • Ubuntu Linux 14.04 - node.js 0.12.0
  • Ubuntu Linux 15.04 - io.js 1.0.0
  • Ubuntu Linux 15.04 - io.js 2.0.0
  • Ubuntu Linux 15.04 - node.js 0.10.0
  • Ubuntu Linux 15.04 - node.js 0.12.0
  • Mac OS X Yosemite - io.js 1.0.0
  • Mac OS X Yosemite - io.js 2.0.0
  • Mac OS X Yosemite - node.js 0.10.0
  • Mac OS X Yosemite - node.js 0.12.0
  • Arch Linux - io.js 1.0.0
  • Arch Linux - io.js 2.0.0
  • Arch Linux - node.js 0.10.0
  • Arch Linux - node.js 0.12.0
  • Raspbian Linux (Raspberry Pi) - io.js 1.0.0
  • Raspbian Linux (Raspberry Pi) - io.js 2.0.0
  • Raspbian Linux (Raspberry Pi) - node.js 0.10.0
  • Raspbian Linux (Raspberry Pi) - node.js 0.12.0
  • Linux (BBB) - io.js 1.0.0
  • Linux (BBB) - io.js 2.0.0
  • Linux (BBB) - node.js 0.10.0
  • Linux (BBB) - node.js 0.12.0

@reggi
Copy link

reggi commented May 7, 2015

Hey @voodootikigod I'm getting the same error.

Here's the full output:
https://gist.github.com/reggi/b107b1baab19431629b7

@imyller
Copy link
Contributor Author

imyller commented May 7, 2015

It seems that I have to update OpenEmbedded recipes in meta-iojs for 2.0.x before this gets in ;)

I was waiting for serialport before updating packages there.

@reggi
Copy link

reggi commented May 7, 2015

@voodootikigod any reason you left out OSX?

Ideally there was a service that would try to build a module on all of these platforms and report back on which fail :)

@voodootikigod
Copy link
Collaborator

@reggi hahah my bad.

Remember for serial port its more than just build, we need to verify it works with an actual device.

@voodootikigod
Copy link
Collaborator

@imyller I am unsure what that means. I apologize if the response of having to make sure it doesn't break on other systems comes as a surprise, but this library is used in a tremendously broad range of implementation for production level things. I can't therefore willnilly update it and break all the things. I hope you understand.

@imyller
Copy link
Contributor Author

imyller commented May 7, 2015

@voodootikigod I meant that I wasn't going to release packages for OpenEmbedded/Yocto Linux for iojs 2.0.0 until I can confirm that node-serialport with recent nan is actually available.

So I'll release first and hope that node-serialport catches up later.

No problems. I understand the need for due diligence.

@mattpodwysocki
Copy link

@voodootikigod seems to work fine on Windows 10 preview for io.js, node.js so at least you have nothing to worry about when it releases this summer.

@voodootikigod
Copy link
Collaborator

Merging cause why not? YOLO driven development, thanks to io.js

voodootikigod pushed a commit that referenced this pull request May 9, 2015
Upgrade to nan v1.8.4 for io.js v2.0.0 compatibility
@voodootikigod voodootikigod merged commit 89e2a88 into serialport:master May 9, 2015
@sirwolfgang
Copy link

👍

@voodootikigod
Copy link
Collaborator

In Version 1.7.1

@imyller
Copy link
Contributor Author

imyller commented May 9, 2015

Thank you 👍

@rektide
Copy link

rektide commented May 14, 2015

Adding to the gist here, will there be support for Chakra Node? 🙉

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

6 participants