io.js v3 preparation #376
Comments
|
I'll check, but I don't think there's more than cosmetic fixes (naming and On Tuesday 30 June 2015 00:20:34 Rod Vagg wrote:
|
|
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. |
|
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. |
|
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. |
|
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:
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 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? |
|
Ooh yeah. The removal of |
|
@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) |
|
that sounds like a lot of work when we need this released yesterday |
|
It's not necessary for release. Can come at some later point. |
|
RC2 https://iojs.org/download/rc/v3.0.0-rc2/ minor V8 update, |
|
babel is fine with this version in smoke testing except for: 😕 |
|
aaand I can't smoke test npm@master now either: |
|
@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? |
|
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 |
|
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. 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? |
|
Of course I didn't test smalloc on the version affected... Think I've fixed it now. |
|
And the EnsureLocal error is because V8 got rid of the Handle class. v8::Handle is now an alias to v8::Local for legacy code. I just have to add ifdefs around whether to define the Handle version or not. |
|
There, all known issues have been fixed. |
|
Progress update:
@kkoopa can you tell me about the state of docs? What needs to be done in #371 to finish this off? |
|
For the LevelDOWN conversion, here's the bulk of work I've had to do in a simple bash script (Linux only atm cause of #!/bin/bash
files=$@
replacements=(
"NanAsyncWorker/Nan::AsyncWorker"
"NanAsyncQueueWorker/Nan::AsyncQueueWorker"
"NanCallback/Nan::Callback"
"NanNewBufferHandle\\(([^;]+);/Nan::NewBuffer(\\1.ToLocalChecked();"
"(NanNew(<(v8::)?String>)?\\(\"[^\"]*\"\\))/\\1.ToLocalChecked()"
"(NanNew<(v8::)?String>\\([^\"][^\;]*);/\\1.ToLocalChecked();"
"NanNew/Nan::New"
"NODE_SET_PROTOTYPE_METHOD/Nan::SetPrototypeMethod"
"NODE_SET_METHOD/Nan::SetMethod"
"_NAN_METHOD_ARGS_TYPE/Nan::NAN_METHOD_ARGS_TYPE"
"(\\W)?args(\\W)/\\1info\\2"
"(^|\\s)(v8::)?Persistent/\\1Nan::Persistent"
"NanAssignPersistent(<\w+>)?\\(([^,]+),\\s*([^)]+)\\)/\\2.Reset(\\3)"
"NanDisposePersistent\\(([^\\)]+)\\)/\\1.Reset()"
"NanReturnValue/info.GetReturnValue().Set"
"NanReturnNull\\(\\)/info.GetReturnValue().Set(Nan::Null())"
"NanScope\\(\\)/Nan::HandleScope\ scope"
"NanEscapableScope\\(\\)/Nan::EscapableHandleScope scope"
"NanEscapeScope/scope.Escape"
"NanReturnUndefined\\(\\);/return;"
"NanUtf8String/Nan::Utf8String"
"NanObjectWrapHandle\\(([^\\)]+)\\)/\\1->handle()"
"(node::)?ObjectWrap/Nan::ObjectWrap"
"NanMakeCallback/Nan::MakeCallback"
"NanNull/Nan::Null"
"NanUndefined/Nan::Undefined"
"NanFalse/Nan::False"
"NanTrue/Nan::True"
"NanThrow(\w+)?Error/Nan::Throw\\1Error"
"NanError/Nan::Error"
"NanGetCurrentContext/Nan::GetCurrentContext"
"([a-zA-Z0-9_]+)->SetAccessor\\(/Nan::SetAccessor(\\1, "
"NanAdjustExternalMemory/Nan::AdjustExternalMemory"
"NanSetTemplate/Nan::SetTemplate"
"NanHasInstance\\(([^,]+),\\s*([^)]+)\\)/Nan::New(\\1)->HasInstance(\\2)"
)
for file in $files; do
echo $file
for replacement in "${replacements[@]}"; do
cat $file | sed -r "s/${replacement}/g" > $file.$$ && mv $file.$$ $file
done
done |
|
I have a question about the |
|
Re docs: Re Maybe: |
|
Thanks for the update and background @kkoopa. I've just converted canvas, PR is here: Automattic/node-canvas#580 and I've updated my conversion script above to include some new patterns in use there. Also, in the process I figured out what the bug with LevelDOWN was because I was getting something similar with canvas! It's from here in nan.h: NAN_INLINE MaybeLocal<v8::Object> NewBuffer(
char* data
, uint32_t size
) {
// arbitrary buffer lengths requires
// NODE_MODULE_VERSION >= IOJS_3_0_MODULE_VERSION
assert(size <= imp::kMaxLength && "too large buffer");
#if NODE_MODULE_VERSION > IOJS_2_0_MODULE_VERSION
return node::Buffer::New(v8::Isolate::GetCurrent(), data, size);
#else
return MaybeLocal<v8::Object>(
node::Buffer::Use(v8::Isolate::GetCurrent(), data, size));
#endif
}Note the Changing the I see that we've removed |
|
No, this is how it should be. Trevor finally cleaned up Node's Buffer implementation and this follows that, so it's a matter of missing documentation. If you want the buffer copied, you should use |
|
Upgrade bignum, tuned the script some more. justmoon/node-bignum#55 Got some hiccups though.
v8::Local<v8::String> fn_name = New(name);
fn->SetName(fn_name);But the compiler complains. Changing to this sorts it out: v8::Local<v8::String> fn_name = New(name).ToLocalChecked();
fn->SetName(fn_name);
shrug |
|
btw, that bash script above applied straight to bignum without any additional modifications needed to make it compile |
|
|
|
I have no insight on the |
|
Fixed SetMethod. 10f258d |
|
That assert is here. inline void Wrap(v8::Handle<v8::Object> handle) {
assert(persistent().IsEmpty());
assert(handle->InternalFieldCount() > 0);
handle->SetAlignedPointerInInternalField(0, this);
persistent().Reset(v8::Isolate::GetCurrent(), handle);
MakeWeak();
}According to the docs https://iojs.org/api/addons.html#addons_wrapping_c_objects There has to be at least one internal field present before calling void MyObject::Init(Local<Object> exports) {
Isolate* isolate = exports->GetIsolate();
// Prepare constructor template
Local<FunctionTemplate> tpl = FunctionTemplate::New(isolate, New);
tpl->SetClassName(String::NewFromUtf8(isolate, "MyObject"));
tpl->InstanceTemplate()->SetInternalFieldCount(1); |
|
I pushed some further commits. |
|
@kkoopa Can you clarify why |
|
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 [email protected] wrote:
|
|
@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 got converted to rather than Also happened with line 103 of src/couchbase_impl.cc 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! |
|
Thanks for the feedback @corbinu, I've had a lot of trouble with the For the ones that it missed on line 10-11, a change from: to should fix it, it's the single-char variables that are catching it. I've updated the original. Thanks! |
|
Getting insertion of |
|
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!) |
Used conversion script:
nodejs/nan#376 (comment)
|
WRT bash script:
Just use |
|
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. |
|
FYI finally finished and published a migration guide https://nodesource.com/blog/cpp-addons-for-nodejs-v4 |
|
Nice job. Looks awesome! |
- The upgrade steps are almost undocumented, but simplified by running a series of `sed` commands on the source files. - The script was [originally suggested](nodejs/nan#376) in a github issue for NAN. - One version has been [released as a gist](https://gist.github.com/thlorenz/7e9d8ad15566c99fd116). - [A modified version](https://gist.github.com/joelpurra/a1129ca1336c14bfd51b1a7ad0f79171) which works better with `getdns-node` was used. - This commit contains only automated fixes; the next commit contains manual fixes to some regexp misses.
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/
@kkoopa can you let us know if there's anything blocking a release to cope with this so we can make it happen asap?
The text was updated successfully, but these errors were encountered: