Update to nan2 and node 3+ APIs #622

zbjornson · 2015-09-10T03:44:40Z

This is not ready to merge.

This builds and all tests (mocha and browser) pass with iojs 3.3.0, but it's unstable and crashes node randomly. I'm at the limits of my C++ knowledge to debug this, although I have some suspicion it's an iojs bug based on the call stack.

@rvagg?

Replaces #580.

rvagg · 2015-09-10T03:57:19Z

src/Canvas.cc

@@ -63,19 +63,19 @@ Canvas::Initialize(Handle<Object> target) {
 */

 NAN_METHOD(Canvas::New) {
-  NanScope();
+  Nan::HandleScope scope;


can be removed, HandleScopes are implicit for methods

zbjornson · 2015-09-10T04:03:33Z

Huh.

This builds on Windows. That excludes JPEGStream, which is causing failures for 1.8+. Build logs for 0.8 - 0.12 are useless, but might be the same problem.

rvagg · 2015-09-10T04:03:39Z

src/CanvasGradient.cc

-    return NanThrowTypeError("offset required");
-  if (!args[1]->IsString())
-    return NanThrowTypeError("color string required");
+  Nan::HandleScope scope;


zbjornson · 2015-09-10T04:08:49Z

@rvagg gimme a sec and I'll clean those up :)

rvagg · 2015-09-10T04:11:25Z

Looks really good @zbjornson, I think where your crashiness comes from is in the use of Nan::NewBuffer() which assumes that it's allowed to take ownership of the data you're passing it whereas Nan::CopyBuffer() take the data and makes a copy of it just for the new Buffer. You need to figure out whether the data you have is being managed elsewhere or whether you're really allowed to hand it off the Buffer, you'll get crashes if it gets cleaned up while the new Buffer thinks it has ownership of it. I ran into this problem myself with leveldown ...

zbjornson · 2015-09-10T04:33:56Z

@rvagg Changing Nan::NewBuffer to Nan::CopyBuffer fixed it indeed, thanks for looking at that so fast. I left one instance of Nan::NewBuffer here, where the closure wasn't destroyed. Maybe that should get changed as well to be safe? I don't see a test that covers that.

Gonna be a bit for travis to catch up but I assume build issues will remain. Actually looks like it's from running node-gyp 2.0.2 instead of 3.x.x. https://travis-ci.org/Automattic/node-canvas/jobs/79592537#L3031 @LinusU perhaps your realm to fix that? :)

rvagg · 2015-09-10T06:42:44Z

The choice of NewBuffer vs CopyBuffer is an important one because doing it wrong could lead to either segfaults or memory leaks. Follow the trail (and read docs?) to figure out who owns what data and what the upstream APIs are doing with it (speaking about cairo here but also in node-canvas itself).

zbjornson · 2015-09-10T23:14:34Z

@rvagg Got most of the builds working. I'm not sure what to do about these: https://travis-ci.org/Automattic/node-canvas/jobs/79758937#L3036 for example

rvagg · 2015-09-11T01:12:08Z

I'm not even sure how to properly test 0.8 any more, I haven't tested it for node-canvas since ... actually I don't know if I ever have!

Anyway, you should be able to remove all of the Handle references from src/* and replace them with Local, even in the function signatures. You should only see HandleScope when you grep Handle src/*.cc src/*.h

zbjornson · 2015-09-11T05:24:06Z

Closer.

0.8 still fails. That's pointing to NODE_MODULE. No idea here.
0.10 through 2.5 work.
3.3.0 💥 explodes... Looks like a bad Travis build config? (This is the version I'm developing in and it builds locally.)

rvagg · 2015-09-11T05:52:21Z

for 0.8, in init.cc, change:

extern "C" void
init (Handle<Object> target) {

to

NAN_MODULE_INIT(init) {

rvagg · 2015-09-11T05:52:54Z

for 3.0+, see this thread to update .travis.yml serialport/node-serialport#566 (comment)

zbjornson · 2015-09-11T07:20:50Z

Wooo builds! Thanks for your help.

I have Class::Initialize(Nan::ADDON_REGISTER_FUNCTION_ARGS_TYPE target) everywhere now, following the change to NAN_MODULE_INIT you suggested. I assume that's okay.

FYI I also fixed the libpng download URL while troubleshooting the build. (replaces #489, fixes #466, overlaps with #602).

LinusU · 2015-09-14T12:15:15Z

Oh crap, I forgot to cut an actual release for that 😭

I'll fix that

kkoopa · 2015-09-14T12:39:59Z

Massive! I'm so happy I didn't have to do this!

benzmuircroft · 2015-09-14T13:00:41Z

I've been following and reading along on all this nan2 upgrade trying to understand whats going on. I see that node-canvas still wants no higher than node version 3. I saw one of you write something about v8::Handle and v8::Local .. wondering if I can help do anything to get node-canvas to work with node 4.0.0 ?

kkoopa · 2015-09-14T13:05:03Z

What works on 3 works on 4.

On Monday 14 September 2015 06:00:49 Benz Muircroft wrote:

I've been following and reading along on all this nan2 upgrade trying to
understand whats going on. I see that node-canvas still wants no higher
than node version 3. I saw one of you write something about v8::Handle and
v8::Local .. wondering if I can help do anything to get node-canvas to work
with node 4.0.0 ?

Reply to this email directly or view it on GitHub:
#622 (comment)

benzmuircroft · 2015-09-14T13:08:21Z

@kkoopa

npm WARN engine [email protected]: wanted: {"node":">=0.8.0 <3"} (current: {"node":"4.0.0","npm":"2.14.2"})

> [email protected] install /home/engine/node_modules/canvas
> node-gyp rebuild

make: Entering directory `/home/engine/node_modules/canvas/build'
  SOLINK_MODULE(target) Release/obj.target/canvas-postbuild.node
  COPY Release/canvas-postbuild.node
  CXX(target) Release/obj.target/canvas/src/Canvas.o
In file included from ../src/Canvas.h:22:0,
             from ../src/Canvas.cc:7:
../node_modules/nan/nan.h:324:27: error: redefinition of ‘template<class T> v8::Local<T>  Nan::imp::NanEnsureHandleOrPersistent(const v8::Local<T>&)’
   NAN_INLINE v8::Local<T> NanEnsureHandleOrPersistent(const v8::Local<T> &val) {
                       ^
../node_modules/nan/nan.h:319:17: error: ‘template<class T> v8::Handle<T> Nan::imp::NanEnsureHandleOrPersistent(v8::Handle<T>&)’ previously declared here
   v8::Handle<T> NanEnsureHandleOrPersistent(const v8::Handle<T> &val) {
             ^
../node_modules/nan/nan.h:344:27: error: redefinition of ‘template<class T> v8::Local<T> Nan::imp::NanEnsureLocal(v8::Handle<T>&)’
   NAN_INLINE v8::Local<T> NanEnsureLocal(const v8::Handle<T> &val) {
                       ^
../node_modules/nan/nan.h:334:27: error: ‘template<class T> v8::Local<T> Nan::imp::NanEnsureLocal(const v8::Local<T>&)’ previously declared here
   NAN_INLINE v8::Local<T> NanEnsureLocal(const v8::Local<T> &val) {
                       ^
../node_modules/nan/nan.h:757:13: error: ‘node::smalloc’ has not been declared
     , node::smalloc::FreeCallback callback
         ^
../node_modules/nan/nan.h:757:35: error: expected ‘,’ or ‘...’ before ‘callback’
     , node::smalloc::FreeCallback callback
                               ^
../node_modules/nan/nan.h: In function ‘v8::Local<v8::Object> NanNewBufferHandle(char*, size_t, int)’:
../node_modules/nan/nan.h:761:50: error: ‘callback’ was not declared in this scope
         v8::Isolate::GetCurrent(), data, length, callback, hint);
                                              ^
../node_modules/nan/nan.h:761:60: error: ‘hint’ was not declared in this scope
         v8::Isolate::GetCurrent(), data, length, callback, hint);
                                                        ^
../node_modules/nan/nan.h: In function ‘v8::Local<v8::Object> NanNewBufferHandle(const char*, uint32_t)’:
../node_modules/nan/nan.h:768:67: error: call of overloaded ‘New(v8::Isolate*, const char*&, uint32_t&)’ is ambiguous
      return node::Buffer::New(v8::Isolate::GetCurrent(), data, size);
                                                               ^
../node_modules/nan/nan.h:768:67: note: candidates are:
In file included from ../node_modules/nan/nan.h:25:0,
                 from ../src/Canvas.h:22,
                 from ../src/Canvas.cc:7:
/root/.node-gyp/4.0.0/include/node/node_buffer.h:31:40: note: v8::MaybeLocal<v8::Object> node::Buffer::New(v8::Isolate*, v8::Local<v8::String>, node::encoding) <near match>
 NODE_EXTERN v8::MaybeLocal<v8::Object> New(v8::Isolate* isolate,
                                    ^
/root/.node-gyp/4.0.0/include/node/node_buffer.h:31:40: note:   no known conversion for argument 3 from ‘uint32_t {aka unsigned int}’ to ‘node::encoding’
/root/.node-gyp/4.0.0/include/node/node_buffer.h:43:40: note: v8::MaybeLocal<v8::Object> node::Buffer::New(v8::Isolate*, char*, size_t) <near match>
 NODE_EXTERN v8::MaybeLocal<v8::Object> New(v8::Isolate* isolate,
                                    ^
/root/.node-gyp/4.0.0/include/node/node_buffer.h:43:40: note:   no known conversion for argument 2 from ‘const char*’ to ‘char*’
In file included from ../src/Canvas.h:22:0,
                 from ../src/Canvas.cc:7:
../node_modules/nan/nan.h: In function ‘v8::Local<v8::Object> NanNewBufferHandle(uint32_t)’:
../node_modules/nan/nan.h:772:61: error: could not convert ‘node::Buffer::New(v8::Isolate::GetCurrent(), ((size_t)size))’ from ‘v8::MaybeLocal<v8::Object>’ to ‘v8::Local<v8::Object>’
     return node::Buffer::New(v8::Isolate::GetCurrent(), size);
                                                         ^
../node_modules/nan/nan.h: In function ‘v8::Local<v8::Object> NanBufferUse(char*, uint32_t)’:
../node_modules/nan/nan.h:779:12: error: ‘Use’ is not a member of ‘node::Buffer’
     return node::Buffer::Use(v8::Isolate::GetCurrent(), data, size);
            ^
../node_modules/nan/nan.h: In function ‘v8::Local<v8::Object> NanNewBufferHandle(const char*, uint32_t)’:
../node_modules/nan/nan.h:769:3: warning: control reaches end of non-void function [-Wreturn-type]
   }
   ^
make: *** [Release/obj.target/canvas/src/Canvas.o] Error 1
make: Leaving directory `/home/engine/node_modules/canvas/build'
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/usr/local/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:270:23)
gyp ERR! stack     at emitTwo (events.js:87:13)
gyp ERR! stack     at ChildProcess.emit (events.js:172:7)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)
gyp ERR! System Linux 2.6.32-042stab108.8
gyp ERR! command "/usr/local/bin/node" "/usr/local/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /home/engine/node_modules/canvas
gyp ERR! node -v v4.0.0
gyp ERR! node-gyp -v v3.0.1
gyp ERR! not ok 
npm ERR! Linux 2.6.32-042stab108.8
npm ERR! argv "/usr/local/bin/node" "/usr/local/bin/npm" "install" "canvas"
npm ERR! node v4.0.0
npm ERR! npm  v2.14.2
npm ERR! code ELIFECYCLE

npm ERR! [email protected] install: `node-gyp rebuild`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] install script 'node-gyp rebuild'.
npm ERR! This is most likely a problem with the canvas package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     node-gyp rebuild
npm ERR! You can get their info via:
npm ERR!     npm owner ls canvas
npm ERR! There is likely additional logging output above.
npm ERR! Linux 2.6.32-042stab108.8
npm ERR! argv "/usr/local/bin/node" "/usr/local/bin/npm" "install" "canvas"
npm ERR! node v4.0.0
npm ERR! npm  v2.14.2
npm ERR! path npm-debug.log.59c1ae6c9c96c64d90c3d09cae73d181
npm ERR! code ENOENT
npm ERR! errno -2
npm ERR! syscall open

npm ERR! enoent ENOENT: no such file or directory, open 'npm-debug.log.59c1ae6c9c96c64d90c3d09cae73d181'
npm ERR! enoent This is most likely not a problem with npm itself
npm ERR! enoent and is related to npm not being able to find a file.
npm ERR! enoent 

npm ERR! Please include the following file with any support request:
npm ERR!     /home/engine/node_modules/npm-debug.log

kkoopa · 2015-09-14T13:16:56Z

It won't work until this PR has been merged.

targos · 2015-09-14T13:28:07Z

I have another issue. I am testing locally and it works fine with

  "optionalDependencies": {
    "canvas": "zbjornson/node-canvas#nan2"
  },

But on Travis, it is different (only for v4, 0.12 is fine):

/home/travis/.node-gyp/4.0.0/include/node/v8.h:336:1: error: expected unqualified-id before ‘using’
/home/travis/.node-gyp/4.0.0/include/node/v8.h:469:1: error: expected unqualified-id before ‘using’
/home/travis/.node-gyp/4.0.0/include/node/v8.h:852:1: error: expected unqualified-id before ‘using’
In file included from ../node_modules/nan/nan.h:182:0,
                 from ../src/Canvas.h:22,
                 from ../src/Canvas.cc:7:
../node_modules/nan/nan_maybe_43_inl.h:13:1: error: expected unqualified-id before ‘using’
../node_modules/nan/nan_maybe_43_inl.h:16:1: error: expected unqualified-id before ‘using’
../node_modules/nan/nan_maybe_43_inl.h:19:12: error: ‘Maybe’ does not name a type
../node_modules/nan/nan_maybe_43_inl.h:24:12: error: ‘Maybe’ does not name a type
../node_modules/nan/nan_maybe_43_inl.h:31:1: error: ‘MaybeLocal’ does not name a type
../node_modules/nan/nan_maybe_43_inl.h:36:1: error: ‘MaybeLocal’ does not name a type
../node_modules/nan/nan_maybe_43_inl.h:41:1: error: ‘Maybe’ does not name a type
../node_modules/nan/nan_maybe_43_inl.h:46:1: error: ‘MaybeLocal’ does not name a type

...

kkoopa · 2015-09-14T14:30:08Z

Needs g++-4.8.

LinusU · 2015-09-14T15:00:56Z

@targos It was released, but not pushed to the main repo :S

Okay, here is what happened. I tagged the release, pushed it to npm, updated History.md and then pushed it to my own fork.

I have now pushed it to the main repo 👍

targos · 2015-09-14T15:03:24Z

@kkoopa It's supposed to be here:
This is my .travis.yml file

language: node_js
node_js:
  - "0.12"
  - "4"
  - "stable"
sudo: false
addons:
  apt:
    sources:
      - ubuntu-toolchain-r-test
    packages:
      - libcairo2-dev
      - libjpeg8-dev
      - libpango1.0-dev
      - libgif-dev
      - g++-4.8

targos · 2015-09-14T15:05:11Z

@LinusU cool, thanks 😃

kkoopa · 2015-09-14T15:54:33Z

You have to set CXX=g++-4.8 too. Just installing the package is not enough.

targos · 2015-09-14T17:28:26Z

Oh right, it is fine now. Thanks a lot !

LinusU · 2015-09-14T18:44:52Z

.travis.yml

  - sudo chown -R $USER /usr/local
  - sh install
+  - npm explore npm -g -- npm install node-gyp@latest
+after_script:
+  - make benchmark


Could you add back the newline at the end of the file?

zbjornson · 2015-09-14T18:56:10Z

@LinusU newline added (but GH isn't recognize the change in this convo). And no, nothing should break.

Thanks @rvagg for the explanation of HandleScope's behavior.

LinusU · 2015-09-14T18:57:27Z

.travis.yml

 before_install:
  - '[ "${TRAVIS_NODE_VERSION}" != "0.8" ] || npm install -g [email protected]'
+  - if [[ $TRAVIS_OS_NAME == "linux" ]]; then export CXX=g++-4.8; fi


Perhaps this line or the one above should be updated so that they follow the same style?

'[ "${TRAVIS_OS_NAME}" != "linux" ] || export CXX=g++-4.8'

LinusU · 2015-09-14T19:01:29Z

This is very good work, hopefully we can get this out of the door within the hour. Do you mind rebasing on master? I pushed some commits and it seems that you are two after.

zbjornson · 2015-09-14T20:46:22Z

Rebased and changed .travis.yml. (I went with the if because I think it's easier to read than "not or".)

I removed my change to install.sh where I updated the URI for libpng. My fix wasn't correct (typo in URL) and when I fixed it, it broke. So, no libpng. Save that for #602.

Thanks!

LinusU · 2015-09-14T20:52:45Z

Cool cool cool! I'll test locally now and merge if everything looks good. Great job 🚀 🎢

Update to nan2 and node 3+ APIs

LinusU · 2015-09-14T21:01:56Z

Published as 1.2.9, thanks @zbjornson and @rvagg for a awesome effort!

benzmuircroft · 2015-09-14T22:14:35Z

excellent!

lehni · 2015-09-14T22:26:08Z

Thanks everybody! Impressive work, in no time.

markthethomas · 2015-09-15T01:23:48Z

Awesome to see this merged! 👍

Latest node-canvas support nodejs v4.0.0; see Automattic/node-canvas#622

rvagg reviewed Sep 10, 2015
View reviewed changes

zowers mentioned this pull request Sep 10, 2015

Can't install on 4.0.0 #621

Closed

Rayman mentioned this pull request Sep 10, 2015

Test more node versions in travis RobotWebTools/roslibjs#196

Merged

lehni mentioned this pull request Sep 14, 2015

Consider switching to newer JSDOM paperjs/paper.js#782

Closed

LinusU reviewed Sep 14, 2015
View reviewed changes

Update to NAN 2 and node 3+ APIs

d4d3b9c

LinusU added a commit that referenced this pull request Sep 14, 2015

Merge pull request #622 from zbjornson/nan2

5405e73

Update to nan2 and node 3+ APIs

LinusU merged commit 5405e73 into Automattic:master Sep 14, 2015

markthethomas added a commit to markthethomas/node-identicon that referenced this pull request Sep 15, 2015

Update package.json

cc4ddc3

Latest node-canvas support nodejs v4.0.0; see Automattic/node-canvas#622

markthethomas mentioned this pull request Sep 15, 2015

Update package.json Ajido/node-identicon#9

Closed

mathiasbynens mentioned this pull request Sep 26, 2015

Upgrade NAN to v2 #580

Closed

Update to nan2 and node 3+ APIs #622

Update to nan2 and node 3+ APIs #622

Conversation

zbjornson commented Sep 10, 2015

rvagg Sep 10, 2015

Choose a reason for hiding this comment

zbjornson commented Sep 10, 2015

rvagg Sep 10, 2015

Choose a reason for hiding this comment

zbjornson commented Sep 10, 2015

rvagg commented Sep 10, 2015

zbjornson commented Sep 10, 2015

rvagg commented Sep 10, 2015

zbjornson commented Sep 10, 2015

rvagg commented Sep 11, 2015

zbjornson commented Sep 11, 2015

rvagg commented Sep 11, 2015

rvagg commented Sep 11, 2015

zbjornson commented Sep 11, 2015

LinusU commented Sep 14, 2015

kkoopa commented Sep 14, 2015

benzmuircroft commented Sep 14, 2015

kkoopa commented Sep 14, 2015

benzmuircroft commented Sep 14, 2015

kkoopa commented Sep 14, 2015

targos commented Sep 14, 2015

kkoopa commented Sep 14, 2015

LinusU commented Sep 14, 2015

targos commented Sep 14, 2015

targos commented Sep 14, 2015

kkoopa commented Sep 14, 2015

targos commented Sep 14, 2015

LinusU Sep 14, 2015

Choose a reason for hiding this comment

zbjornson commented Sep 14, 2015

LinusU Sep 14, 2015

Choose a reason for hiding this comment

LinusU commented Sep 14, 2015

zbjornson commented Sep 14, 2015

LinusU commented Sep 14, 2015

LinusU commented Sep 14, 2015

benzmuircroft commented Sep 14, 2015

lehni commented Sep 14, 2015

markthethomas commented Sep 15, 2015