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 2.0.0 #566

Closed
wants to merge 10 commits into from
Closed

Upgrade to Nan 2.0.0 #566

wants to merge 10 commits into from

Conversation

ghostoy
Copy link

@ghostoy ghostoy commented Aug 22, 2015

Upgrade to Nan 2.0.0 to support latest io.js 3.x

@ghostoy ghostoy force-pushed the nan1to2 branch 3 times, most recently from 38ede95 to 913d18f Compare August 22, 2015 13:19
@ghostoy
Copy link
Author

ghostoy commented Aug 22, 2015

Sorry. I don't know how to make Travis-CI work correctly. But it's built and tested ok on my Windows, Linux and Mac.

@zzz6519003
Copy link

🍺 the commits can be squashed XD

@rwaldron
Copy link
Contributor

rwaldron commented Sep 7, 2015

The failure is io.js 3.0.0 specific—maybe @rvagg can take a look and offer some insight.

@rvagg
Copy link
Contributor

rvagg commented Sep 7, 2015

this is a compiler incompatibility, Travis runs on Ubuntu 12.04 and therefore has a GCC that doesn't support the newer C++ functionality being embraced by V8, so you need to upgrade it (tbh I don't know about clang and I see you have compiler: clang in there but I'm guessing it's similar). What other projects have done to make this work is add the following:

addons:
  apt:
    sources:
    - ubuntu-toolchain-r-test
    packages:
    - g++-4.8

Then under install add the following at the top of the list:

  - if [[ $TRAVIS_OS_NAME == "linux" ]]; then export CXX=g++-4.8; fi
  - $CXX --version

(the "linux" case there is helpful if you're adding the experimental osx support travis have made available).

On your builds you should see a gcc version of 4.8. Probably remove the clang bit while you're there.

@rwaldron
Copy link
Contributor

rwaldron commented Sep 8, 2015

Thanks @rvagg

@@ -63,7 +63,7 @@
// - Jim Cline
// - Jeff Kohn
// - Todd Heckel
// - Ullrich Poll�hne
// - Ullrich Poll�hne
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@ChALkeR
Copy link
Contributor

ChALkeR commented Sep 10, 2015

Adding to the list: nodejs/node#2798.

@ghostoy
Copy link
Author

ghostoy commented Sep 11, 2015

Disabled 32bit build and dropped 0.8 because node-pre-gyp doesn't support 0.8 anymore.

@ghostoy
Copy link
Author

ghostoy commented Sep 13, 2015

Bring back x86 builds on Travis-CI.


OpenBaton* baton = new OpenBaton();
memset(baton, 0, sizeof(OpenBaton));
strcpy(baton->path, *path);
baton->baudRate = options->Get(NanNew<v8::String>("baudRate"))->ToInt32()->Int32Value();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove all ->Get?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not removed. It's replace with Nan::Get, which is said to be compatible with all V8 versions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The double ToLocalChecked seems redundant and expensive, are you sure its needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ToLocalChecked is introduced by the new Maybe types, which fails at the point of null value. And there is another counterpart ToLocal which doesn't do the check. I think the current convention is assuming options->Get always return non-NULL value. So ToLocalChecked is the safest replacement.

@imyller
Copy link
Contributor

imyller commented Sep 18, 2015

👍 Travis CI passed. Good to go?

@mgcrea
Copy link

mgcrea commented Sep 21, 2015

Looking forward to have this merged to use with latest node stable.

@fivdi
Copy link
Contributor

fivdi commented Sep 26, 2015

I performed some tests with this PR on the BeagleBone Black with two different test setups.

  1. Debian Wheezy, gcc/g++ 4.6, Node.js v0.10.38
    Result: installed successfully and a simple test ran successfully

  2. Debian Jessie, gcc/g++ 4.9, Node.js v4.1.1
    Result: installed successfully and a simple test ran successfully

baton->hupcl = options->Get(NanNew<v8::String>("hupcl"))->ToBoolean()->BooleanValue();

v8::Local<v8::Object> platformOptions = options->Get(NanNew<v8::String>("platformOptions"))->ToObject();
baton->baudRate = Nan::Get(options, Nan::New<v8::String>("baudRate").ToLocalChecked()).ToLocalChecked()->ToInt32()->Int32Value();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ghostoy talking about this double tolocalchecked. Are we sure it needs to be checked twice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not being, the "baudRate" v8::String is being checked first and then the v8::Value that results from the Nan::Get() has to be converted too.

See https://nodesource.com/blog/cpp-addons-for-nodejs-v4 for some details on MaybeLocal types and why this garbage is needed (in short: blame Haskell).

@voodootikigod
Copy link
Collaborator

This is missing the infrastructure update/buildout for Windows. That needs to be done in the appveyor.yml file. Will see if I can handle the update.

@jacobrosenthal
Copy link
Contributor

Squashed, rebased and commited as 866e415

@jacobrosenthal jacobrosenthal mentioned this pull request Oct 13, 2015
@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.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

10 participants