Allow the use of Isolated V8 methods for node-0.12

[morellan]

I was trying to build the atom/node-spellchecker for nw.js with nw-gyp and keep getting the same error:

▶ nw-gyp build
gyp info it worked if it ends with ok
gyp info using nw-gyp@0.12.4
gyp info using node@0.12.0 | darwin | x64
child_process: customFds option is deprecated, use stdio instead.
gyp info spawn make
gyp info spawn args [ 'BUILDTYPE=Release', '-C', 'build' ]
  CXX(target) Release/obj.target/spellchecker/src/main.o
In file included from ../src/main.cc:1:
../node_modules/nan/nan.h:207:13: error: no type named 'SetCounterFunction' in 'v8::V8'
    v8::V8::SetCounterFunction(cb);
    ~~~~~~~~^
../node_modules/nan/nan.h:212:13: error: no type named 'SetCreateHistogramFunction' in 'v8::V8'
    v8::V8::SetCreateHistogramFunction(cb);
    ~~~~~~~~^
../node_modules/nan/nan.h:217:13: error: no type named 'SetAddHistogramSampleFunction' in 'v8::V8'
    v8::V8::SetAddHistogramSampleFunction(cb);
    ~~~~~~~~^
../node_modules/nan/nan.h:221:12: error: no member named 'IdleNotification' in 'v8::V8'; did you mean 'NanIdleNotification'?
    return v8::V8::IdleNotification(idle_time_in_ms);
           ^~~~~~~~~~~~~~~~~~~~~~~~
           NanIdleNotification
../node_modules/nan/nan.h:220:19: note: 'NanIdleNotification' declared here
  NAN_INLINE bool NanIdleNotification(int idle_time_in_ms) {
                  ^
../node_modules/nan/nan.h:225:5: error: no member named 'LowMemoryNotification' in 'v8::V8'; did you mean 'NanLowMemoryNotification'?
    v8::V8::LowMemoryNotification();
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    NanLowMemoryNotification
../node_modules/nan/nan.h:224:19: note: 'NanLowMemoryNotification' declared here
  NAN_INLINE void NanLowMemoryNotification() {
                  ^
../node_modules/nan/nan.h:229:5: error: no member named 'ContextDisposedNotification' in 'v8::V8'; did you mean 'NanContextDisposedNotification'?
    v8::V8::ContextDisposedNotification();
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    NanContextDisposedNotification
../node_modules/nan/nan.h:228:19: note: 'NanContextDisposedNotification' declared here
  NAN_INLINE void NanContextDisposedNotification() {
                  ^
6 errors generated.
make: *** [Release/obj.target/spellchecker/src/main.o] Error 1
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/nw-gyp/lib/build.js:267:23)
gyp ERR! stack     at ChildProcess.emit (events.js:110:17)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (child_process.js:1067:12)
gyp ERR! System Darwin 14.1.0
gyp ERR! command "node" "/usr/local/bin/nw-gyp" "build"
gyp ERR! cwd /Users/mob/Dev/node-spellchecker
gyp ERR! node -v v0.12.0
gyp ERR! nw-gyp -v v0.12.4
gyp ERR! not ok

After a lot of research I found that node-0.12 uses V8 version 3.28.73 which implement the method with Isolated (https://chromium.googlesource.com/v8/v8.git/+/3.28.73/src/api.cc#6692), and in the code those are available only for NODE_MODULE_VERSION >= IOJS_1_0_MODULE_VERSION.

After this change it worked, this is my first PR so any comments or another way to achieve the same are welcome.

[kkoopa]

While the patch seems to pass, I am still not entirely sure that it does not have any unintended consequences. The problem is that nw.js claims to be something it clearly is not and we have no reliable way of checking for the actual v8 version used. Even if it somehow were possible, I will still not make special case after special case and exceptions left and right. A version is a version and will be treated as such, not a free-form user agent with five levels of hacks. It is completely unmaintainable.

[morellan]

Given that, I don't think it is necessary to change anything in nan, as they said in their documentation

v0.11.6: (Jan 21, 2015, based off of Node v0.11.13, Chromium 38.0.2125.104)

The weird thing is that nw-gyp uses the node version of nw.js but when you ask the node version it uses node -v, maybe it's a problem in nw-gyp itself.

[kkoopa]

Will hold off while waiting for the response of nw.js, once this has been sorted out, new versions of nw.js 11 will most likely start working by themselves. nw.js 8 should still be fine. Past versions of nw.js 11 will never work.

[rogerwang]

@bnoordhuis thanks for CCing.

@morellan it's required to pass -t <version of nw> to nw-gyp so it will receive the correct version and then download the correct headers distributed by NW. If that's the case, I don't think nan should be changed.

[rogerwang]

@kkoopa NW.js integrates Chromium and Node.js (in <= NW 11) or IO.js (in NW 12 and later) and always uses the v8 version in Chromium.

[kkoopa]

The problem is that the version of v8 cannot be detected directly, so we must rely on NODE_MODULE_VERSION and NODE_VERSION to also detect v8 version. Since nw.js may use a different v8 version, that check will fail.

[rogerwang]

@kkoopa you're right. and v8 upgrades from 3.24.35.22 to 3.28.73 in Node.js (from 0.11.13 to 0.11.15).

Currently in NW11 the Node.js is 0.11.13 and v8 is 3.28.71.2 (from Chrome). Since Nan supports Node.js 0.11.15 (with v8 3.28.73), if NW updates its Node.js to 0.11.15 in next NW 0.11.7 everything should be fine is that correct?

[kkoopa]

Yes, that should hopefully work for now, but the problem remains if you need to change v8 versions again in the future.

[rogerwang]

@kkoopa yes. To fix the root cause v8 should add version definition macros in their headers ...

[kkoopa]

Yes, that would indeed be the best recourse. #226 revolves around the same problem.

[bnoordhuis]

That's https://code.google.com/p/v8/issues/detail?id=3075 - I took a shot at it once but IIRC I got stumped integrating with V8's homegrown release script.

[bnoordhuis]

FYI: https://codereview.chromium.org/955963002/

[morellan]

👍 for the public version macro for v8 @bnoordhuis

But in the meantime at least we need to update the documentation on nw-gyp, maybe adding another note @rogerwang

Note: nw.js is packed with Node.js version 0.11.13 and a different version of V8 (3.28.71.2) than the one Node.js 0.11.13 has (3.24.35.22), it might lead to some inconsistent behaviour when building your native modules (see #285).

Should I open a pull request with that?

[rogerwang]

@morellan yes please. thanks.

[morellan]

@rogerwang done!
nwjs/nw-gyp#62

[merqlove]

I'm locked nan to 1.4.3, till fix release. Higher releases still not work under nw.js 0.11.6 with node 0.11.13.

[kkoopa]

nw.js 0.11.6 will never work

[merqlove]

Yeah, on 0.11.5 was same issue :) But for now NW.JS won't start my app...without logs/messages... Magic...

[kkoopa]

You have to wait for a possible 0.11.7 or switch to 0.12.0.

[merqlove]

Now all works fine on 0.11.6 with Nan 1.4.3 👍
@kkoopa I will wait for 0.12.0, Thanks!