Skip to content

io.js v3 preparation #376

Closed
Closed
io.js v3 preparation#376
Milestone
2.0

Description

@rvagg
rvagg
opened on Jun 30, 2015

So! We have V8 4.4 in io.js/next now and the next-nightlies are also coming out with both header-only tarballs and a node-gyp that is hacked to understand when it's running on a nightly, next-nightly, rc or release build and will then grab the headers tarball from the right location. i.e. npm install <compiled addon> should work properly on next-nightly builds and RC builds that I'm about to start pushing out for io.js v3. We need to get NAN rolled out to cope with this now that we have a target to work against.

From this build: https://iojs.org/download/next-nightly/v2.3.1-next-nightly201506308f6f4280c6/

$ npm i buffertools
npm http request GET https://registry.npmjs.org/buffertools
npm http 304 https://registry.npmjs.org/buffertools
npm WARN engine buffertools@2.1.2: wanted: {"node":">=0.3.0"} (current: {"node":"2.3.1-next-nightly201506308f6f4280c6","npm":"2.11.3"})

> buffertools@2.1.2 install /tmp/node_modules/buffertools
> node-gyp rebuild

gyp http GET https://iojs.org/download/next-nightly/v2.3.1-next-nightly201506308f6f4280c6/iojs-v2.3.1-next-nightly201506308f6f4280c6-headers.tar.gz
gyp http 200 https://iojs.org/download/next-nightly/v2.3.1-next-nightly201506308f6f4280c6/iojs-v2.3.1-next-nightly201506308f6f4280c6-headers.tar.gz
gyp http GET https://iojs.org/download/next-nightly/v2.3.1-next-nightly201506308f6f4280c6/SHASUMS256.txt
gyp http 200 https://iojs.org/download/next-nightly/v2.3.1-next-nightly201506308f6f4280c6/SHASUMS256.txt
make: Entering directory '/tmp/node_modules/buffertools/build'
  CXX(target) Release/obj.target/buffertools/buffertools.o
../buffertools.cc: In function ‘v8::Local<v8::Value> {anonymous}::decodeHex(const uint8_t*, size_t, const v8::FunctionCallbackInfo<v8::Value>&, uint32_t)’:
../buffertools.cc:367:71: error: conversion from ‘v8::MaybeLocal<v8::Object>’ to non-scalar type ‘v8::Local<v8::Object>’ requested
   Local<Object> buffer = UNI_BUFFER_NEW(size / 2);
                                                                       ^
../buffertools.cc: In function ‘void {anonymous}::Concat(const v8::FunctionCallbackInfo<v8::Value>&)’:
../buffertools.cc:459:67: error: conversion from ‘v8::MaybeLocal<v8::Object>’ to non-scalar type ‘v8::Local<v8::Object>’ requested
   Local<Object> buffer = UNI_BUFFER_NEW(size);
                                                                   ^
buffertools.target.mk:88: recipe for target 'Release/obj.target/buffertools/buffertools.o' failed
make: *** [Release/obj.target/buffertools/buffertools.o] Error 1
make: Leaving directory '/tmp/node_modules/buffertools/build'

@kkoopa can you let us know if there's anything blocking a release to cope with this so we can make it happen asap?

Activity

  1. kkoopa commented on Jun 30, 2015

    @kkoopa
    kkoopa
    on Jun 30, 2015
    Collaborator

    I'll check, but I don't think there's more than cosmetic fixes (naming and
    such) to fix.

    On Tuesday 30 June 2015 00:20:34 Rod Vagg wrote:

    So! We have V8 4.4 in io.js/next now and the next-nightlies are also coming
    out with both header-only tarballs and a node-gyp that is hacked to
    understand when it's running on a nightly, next-nightly, rc or release
    build and will then grab the headers tarball from the right location. i.e.
    npm install <compiled addon> should work properly on next-nightly builds
    and RC builds that I'm about to start pushing out for io.js v3. We need to
    get NAN rolled out to cope with this now that we have a target to work
    against.

    From this build:
    https://iojs.org/download/next-nightly/v2.3.1-next-nightly201506308f6f4280c
    6/

    $ npm i buffertools
npm http request GET https://registry.npmjs.org/buffertools
npm http 304 https://registry.npmjs.org/buffertools
npm WARN engine buffertools@2.1.2: wanted: {"node":">=0.3.0"} (current:
{"node":"2.3.1-next-nightly201506308f6f4280c6","npm":"2.11.3"})
> buffertools@2.1.2 install /tmp/node_modules/buffertools
> node-gyp rebuild

gyp http GET
https://iojs.org/download/next-nightly/v2.3.1-next-nightly201506308f6f4280c
6/iojs-v2.3.1-next-nightly201506308f6f4280c6-headers.tar.gz gyp http 200
https://iojs.org/download/next-nightly/v2.3.1-next-nightly201506308f6f4280c
6/iojs-v2.3.1-next-nightly201506308f6f4280c6-headers.tar.gz gyp http GET
https://iojs.org/download/next-nightly/v2.3.1-next-nightly201506308f6f4280c
6/SHASUMS256.txt gyp http 200
https://iojs.org/download/next-nightly/v2.3.1-next-nightly201506308f6f4280c
6/SHASUMS256.txt make: Entering directory
'/tmp/node_modules/buffertools/build'
  CXX(target) Release/obj.target/buffertools/buffertools.o
../buffertools.cc: In function ‘v8::Local<v8::Value>
{anonymous}::decodeHex(const uint8_t*, size_t, const
v8::FunctionCallbackInfo<v8::Value>&, uint32_t)’: ../buffertools.cc:367:71:
error: conversion from ‘v8::MaybeLocal<v8::Object>’ to non-scalar type
‘v8::Local<v8::Object>’ requested Local<Object> buffer =
UNI_BUFFER_NEW(size / 2);
                                                                       ^
../buffertools.cc: In function ‘void {anonymous}::Concat(const
v8::FunctionCallbackInfo<v8::Value>&)’: ../buffertools.cc:459:67: error:
conversion from ‘v8::MaybeLocal<v8::Object>’ to non-scalar type
‘v8::Local<v8::Object>’ requested Local<Object> buffer =
UNI_BUFFER_NEW(size);
                                                                   ^
buffertools.target.mk:88: recipe for target
'Release/obj.target/buffertools/buffertools.o' failed make: ***
[Release/obj.target/buffertools/buffertools.o] Error 1
make: Leaving directory '/tmp/node_modules/buffertools/build'

    @kkoopa can you let us know if there's anything blocking a release to cope
    with this so we can make it happen asap?

    Reply to this email directly or view it on GitHub:
    #376

  2. bnoordhuis commented on Jun 30, 2015

    @bnoordhuis
    bnoordhuis
    on Jun 30, 2015
    Member

    buffertools doesn't use nan at the moment (it uses a hand-rolled compat shim) but I can migrate it to nan if you think it's a useful guinea pig.

  3. rvagg commented on Jun 30, 2015

    @rvagg
    rvagg
    on Jun 30, 2015
    MemberAuthor

    not necessary @bnoordhuis, I just use it as a quick test for the runtime, not nan necessarily, cause it's small and a quick compile. bignum is probably just as good for testing nan.

  4. rvagg commented on Jun 30, 2015

    @rvagg
    rvagg
    on Jun 30, 2015
    MemberAuthor

    https://iojs.org/download/rc/v3.0.0-rc1/

    There's an RC1, we'll hold off on publicising it and putting it on iojs.org until we have a NAN release out.

  5. rvagg commented on Jul 1, 2015

    @rvagg
    rvagg
    on Jul 1, 2015
    MemberAuthor

    Some issues in attempting a migration of leveldown to nan@next, which I'm obviously far from achieving but here are some things I've encountered so far:

    • The RC1 build still has the same NODE_MODULE_VERSION as io.js v2, I should have bumped it to 45 and I'll have to do an RC2 to accommodate this. Grr.
    • Internal references in NAN don't use the namespace so unless you are using namespace Nan you get errors, so far just on the NAN_METHOD, NAN_GETTER etc. definitions, should be using Nan::NAN_METHOD_RETURN_TYPE etc.
    • We need to take into account the lack of a native node::smalloc in v3:
    ../node_modules/nan/nan.h:667:13: error: ‘node::smalloc’ has not been declared
     , node::smalloc::FreeCallback callback
    • My compiler is warning of redefinition of EnsureHandleOrPersistent and EnsureLocal, I can comment out the v8::Local<T> ones to make these warnings go away.

    And finally: I have clearly lost track of the rewriting going on in NAN because this is super-painful and I feel like I don't have enough context. The change to implementing Nan versions of FunctionInfoCallback and Persistent have caught me a little off guard. I tried to use the 1to2.js tool initially but with not much success so I backed up and started picking off the changes using sed and now I'm stuck at trying to grok the new funky Maybe stuff.

    So, my take-away is that there's a huge need for documentation to get NAN@2 out, both formal API documentation but also a primer for people upgrading, otherwise we're just going to have a lot of wailing and gnashing of teeth (actually that might be hard to avoid anyway!). I can write docs but I don't have enough information to start with and will likely do a bad job at it. Perhaps @kkoopa et. al. could provide some dot-points of the major changes that I can use as a starting point to begin investigation?

  6. trevnorris commented on Jul 1, 2015

    @trevnorris
    trevnorris
    on Jul 1, 2015
    Collaborator

    Ooh yeah. The removal of smalloc was a last minute change when V8 was updated to v4.4 after latest nan had been released. Couldn't have been accounted for.

  7. mathiask88 commented on Jul 1, 2015

    @mathiask88
    mathiask88
    on Jul 1, 2015
    Contributor

    @rvagg kkoopa wrote a migration tool based on libclang (https://github.com/kkoopa/node-libclang/tree/experiment?files=1) but I don't know the status of that. The plan was to wrap this tool into a prebuild electron app making the conversion as easy as possible (select binding.gyp->press convert->show some help how to deal with Maybes->done)

  8. rvagg commented on Jul 1, 2015

    @rvagg
    rvagg
    on Jul 1, 2015
    MemberAuthor

    that sounds like a lot of work when we need this released yesterday

  9. kkoopa commented on Jul 1, 2015

    @kkoopa
    kkoopa
    on Jul 1, 2015
    Collaborator

    It's not necessary for release. Can come at some later point.

  10. rvagg commented on Jul 2, 2015

    @rvagg
    rvagg
    on Jul 2, 2015
    MemberAuthor

    RC2 https://iojs.org/download/rc/v3.0.0-rc2/ minor V8 update, NODE_MODULE_VERSION bumped

  11. rvagg commented on Jul 2, 2015

    @rvagg
    rvagg
    on Jul 2, 2015
    MemberAuthor

    babel is fine with this version in smoke testing except for:

      1) util regexify:
     AssertionError: /\x66oo|\x62ar/i deepEqual /foo|bar/i

    😕

  12. rvagg commented on Jul 2, 2015

    @rvagg
    rvagg
    on Jul 2, 2015
    MemberAuthor

    aaand I can't smoke test npm@master now either:

    Packaging or installing npm@3.0.0 with npm@2.11.3 is impossible.
Please install npm@^3.0.0-0 from the registry and use that or run your command with
this version of npm with:
    /usr/local/bin/iojs /smoke/npm install --cache-min=Infinity

  13. domenic commented on Jul 2, 2015

    @domenic
    domenic
    on Jul 2, 2015

    @Fishrock123 was looking in to npm@3, summoning him... that is a strange error. Maybe try that thing it asks you to do with cache-min?

  14. rvagg commented on Jul 2, 2015

    @rvagg
    rvagg
    on Jul 2, 2015
    MemberAuthor

    actually, ignore those 2 msgs, I thought I was in the v2.3.2 release thread! this is not relevant to v3.0.0, I confused

  15. kkoopa commented on Jul 4, 2015

    @kkoopa
    kkoopa
    on Jul 4, 2015
    Collaborator

    I've pushed an update. It makes the callback declaration macros use fully qualified names. These should be the only macros and hence the only ones requiring this namespace fix.
    I also renamed Scope to HandleScope and EscapableScope to EscapableHandleScope, to better match v8.

    Regarding changes to persistent and such, they now follow newest v8 API as closely as possible, so you basically just use them as such.

    Regarding smalloc errors and EnsureLocal et al, I don't get warnings about that on my compiler. Which one are you using? Can you fix it?

  16. 15 remaining items

  17. kkoopa commented on Jul 13, 2015

    @kkoopa
    kkoopa
    on Jul 13, 2015
    Collaborator

    I pushed some further commits. Nan::HasInstance is no more. It had become needless sugar.

  18. trevnorris commented on Jul 13, 2015

    @trevnorris
    trevnorris
    on Jul 13, 2015
    Collaborator

    ObjectWrap is no longer used by internal core. For performance I implemented a similar class but that requires less overhead.

    @kkoopa Can you clarify why Nan::HasInstance was removed?

  19. kkoopa commented on Jul 13, 2015

    @kkoopa
    kkoopa
    on Jul 13, 2015
    Collaborator

    It seemed unnecessary. All it did was call FunctionTemplate::HasInstance. I can put it back if someone makes a good argument for why it should stay.

    On July 13, 2015 9:14:41 PM EEST, Trevor Norris notifications@github.com wrote:

    ObjectWrap is no longer used by internal core. For performance I
    implemented a similar class but that requires less overhead.

    @kkoopa Can you clarify why Nan::HasInstance was removed?

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

  20. corbinu commented on Jul 14, 2015

    @corbinu
    corbinu
    on Jul 14, 2015

    @rvagg I tried using your upgrade script here on https://github.com/corbinu/couchnode

    It works great other then a few issues for example line 37 of src/exception.h

    which is

    Handle<Value> args[] = { NanNew<String>(msg.c_str()) };

    got converted to

    Handle<Value> info[] = { Nan::New<String>(msg.c_str()) }.ToLocalChecked();

    rather than

    Handle<Value> info[] = { Nan::New<String>(msg.c_str()).ToLocalChecked() };

    Also happened with line 103 of src/couchbase_impl.cc
    which should be decObj->Set(Nan::New(valueKey), Nan::NewBuffer((char*)bytes, nbytes).ToLocalChecked());

    This may not be as easy to fix but it also missed adding .ToLocalChecked() on line 10-11 of src/constants.cc

    I still don't have all the issues fixed but hoping to soon.

    On another note thanks guys for all your work looks like it will be MUCH better then before!

  21. rvagg commented on Jul 14, 2015

    @rvagg
    rvagg
    on Jul 14, 2015
    MemberAuthor

    Thanks for the feedback @corbinu, I've had a lot of trouble with the .ToLocalChecked() addition too, unfortunately I don't have a good solution for it but on the plus side, at least the compiler picks them up properly too.

    For the ones that it missed on line 10-11, a change from:

      "(NanNew<(v8::)?String>\\([^\"][^\;]+);/\\1.ToLocalChecked();"

    to

      "(NanNew<(v8::)?String>\\([^\"][^\;]*);/\\1.ToLocalChecked();"

    should fix it, it's the single-char variables that are catching it. I've updated the original. Thanks!

  22. kkoopa
    added this to the 2.0 milestone on Jul 19, 2015

  23. kkoopa commented on Jul 19, 2015

    @kkoopa
    kkoopa
    on Jul 19, 2015
    Collaborator

    Getting insertion of ToLocalCheccked() calls fully right using regular expressions will be impossible, even in non-contrived cases. Partly for that reason I started work on a heavy-duty converter based on libclang https://github.com/kkoopa/node-libclang/blob/experiment/analyzer.js The only major obstacle there is getting the physical rewriting part correct, my offsets get messed up now and then for no good reason and it all goes south from there. However, the analyzer part works great in that it finds all the correct locations that need changing. The only drawback with this approach is that only active code paths can be fixed. Anything that uses conditional compilation (for different versions or operating systems or such) will require a run of the analyzer in every environment, but this is only a minor nuisance. Any proper software project should have a way of running the entire code base during testing.

  24. rvagg commented on Jul 22, 2015

    @rvagg
    rvagg
    on Jul 22, 2015
    MemberAuthor

    RC 4 released: https://iojs.org/download/rc/v3.0.0-rc.4/ (properly including all of io.js v2.4.0)

    Details here: nodejs/node#2221 (comment) (note the node-gyp changes, yay!)

  25. kkoopa
    closed this as completedon Jul 31, 2015
  26. tcort
    added a commit that references this issue on Aug 14, 2015

    arc4random.cc: update for nan 2.0

    1961678

  27. thlorenz commented on Aug 20, 2015

    @thlorenz
    thlorenz
    on Aug 20, 2015

    WRT bash script:

    (Linux only atm cause of sed -r but that's fixable)

    Just use sed -E on OSX instead.

  28. thlorenz commented on Aug 20, 2015

    @thlorenz
    thlorenz
    on Aug 20, 2015

    Updated that nifty bash script a bit with things I ran into (just very minor changes) and put it in a public gist so we can keep updating it easier than in this issue.
    https://gist.github.com/thlorenz/7e9d8ad15566c99fd116

  29. rvagg commented on Sep 25, 2015

    @rvagg
    rvagg
    on Sep 25, 2015
    MemberAuthor

    FYI finally finished and published a migration guide https://nodesource.com/blog/cpp-addons-for-nodejs-v4

  30. brett19 commented on Sep 25, 2015

    @brett19
    brett19
    on Sep 25, 2015
    Contributor

    Nice job. Looks awesome!

  31. joelpurra
    added a commit that references this issue on Jul 24, 2016

    Run NAN v1 to v2 upgrade script

    be48e29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions