-
-
Notifications
You must be signed in to change notification settings - Fork 815
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
N-API Support for node-sqlite3 #830
Comments
Hello @sampsongao - this is great - thank you for your efforts on N-API. Yes, I would like to support N-API in node-sqlite3. I see you've created ttps://github.com/mhdawson/node-sqlite3/tree/v3.1.4-abi to test the N-API port. How did it go? Was there any performance impact? |
The v3.1.4-abi is the port directly using N-API. But since the N-API is quite verbose, so we also developed C++ helpers to make it simpler to port. I am currently working on porting node-sqlite3 to helpers. And there are some problems I still need to address. Feel free to take a look at the v3.1.4-napi-wrapper branch. |
Thanks for the details @sampsongao. Are there any examples of ports of very simple modules you could share as well? For reference our team is planning to work on porting our simple example node module: mapbox/node-cpp-skel#45 |
You can take the node-nanomsg port as reference. If you want to take something a little more complex, you can checkout the node-canvas port. Also, take a look on the node-addon-api. That is the wrapper I was talking about. |
@springmeyer - you can tryout the N-API yeoman generator to get started with this https://github.com/digitalinfinity/generator-napi-module. |
@springmeyer How is your experience using N-API is like? We would like to hear your feedback and the obstacle you faced during your migration. |
@sampsongao - thank you for the ping. I've not had a chance yet to experiment with the N-API but I will let you know as soon as I do. |
Hi @springmeyer , any update on your experiment? |
@sampsongao @aruneshchandra - do you have a branch of latest node-sqlite3 that is working against latest |
@springmeyer You can find it here. https://github.com/mhdawson/node-sqlite3/tree/node-addon-api |
@sampsongao - thanks, however I hit these errors trying to build that branch: https://gist.github.com/springmeyer/f767c07b662c200121a0296b2fe36a08. Is it working for you? What node version are you testing with? I tried with node v8.9.3 on OS X 10.12 |
@sampsongao any idea about the above errors? |
@sampsongao bump |
I am using node master now which is node 10 pre. However, I can't reproduce the problem on my machine and it seems to be node-addon-api related. Can you make sure you are using [email protected]. |
Actually, it is problem with std::vector. The assign method is a build in method for std::vector. Do you know any reason vector is not working in X code? |
Yes, I am using [email protected] (https://github.com/nodejs/node-addon-api/releases/tag/v1.1.0) |
std::vector works fine, I think this is a bug in your port:
The problem looks like you are trying to convert an integer to a pointer, which does not look like valid C++:
|
I don't have time to debug N-API or the port you've provided. I really appreciate you getting it going but unless it builds on all platforms I can't move forward since I don't understand N-API internals yet. So I'm considering this blocked until someone else gets https://github.com/mhdawson/node-sqlite3/tree/node-addon-api to compile on OS X and Linux. @sampsongao can you please create a PR against this repo, then we'll get the benefit of travis testing to see whether this problem is also causing breakages on other platforms like linux. |
I had tried it with node v9.3.0 and on Ubuntu 16.04. It built without error. I think this is a compiler-specific problem. I will try to see how to fix this and get back to you. |
Might be libc++ specific then and perhaps replicable on linux with |
Hi @springmeyer! I've merged a fix for your issue (I was able to replicate the problem on my mac), and the port should compile and pass all local tests on all major platforms. https://github.com/mhdawson/node-sqlite3/tree/node-addon-api Let me know if there are any further issues or if you have any questions. |
@anisha-rohra thank you very much for replicating and fixing that issue. I just pulled the latest code from https://github.com/mhdawson/node-sqlite3/tree/node-addon-api and then hit:
I was able to solve this by adding: diff --git a/binding.gyp b/binding.gyp
index 83f0d43..606b145 100644
--- a/binding.gyp
+++ b/binding.gyp
@@ -34,6 +34,12 @@
}
]
],
+ 'xcode_settings': {
+ 'MACOSX_DEPLOYMENT_TARGET':'10.8',
+ 'CLANG_CXX_LIBRARY': 'libc++',
+ 'CLANG_CXX_LANGUAGE_STANDARD':'c++11',
+ 'GCC_VERSION': 'com.apple.compilers.llvm.clang.1_0'
+ },
'defines': [ 'NAPI_DISABLE_CPP_EXCEPTIONS' ],
"cflags": [ "-include ../src/gcc-preinclude.h" ],
"sources": [ Is it correct/intended that node-addon-api requires C++11 for http://en.cppreference.com/w/cpp/utility/initializer_list? After building in c++11 mode then the compile and link worked, but I'm hitting this test failure:
Are you also seeing that test failure? |
Hi @springmeyer ! So on my personal machine, I don't need the patch in order to compile and I don't have any tests failing. I'm going to investigate by running everything on an older OSX (specifically 10.8) and also determine whether the requirement for C++11 that you ran into can be removed. Could you let me know what version of node and OSX you're running? Thank you for the update and information! |
@anisha-rohra OS X
|
Hello again @springmeyer I've been working on getting access to as close to an exact match to your environment and I finally managed to do so. However, despite working on a 10.12 machine on node v9.4 with the same clang version, the 132 tests in the suite all pass. Is there any other environment-specific variables that could differentiate your machine to that of a standard Mac OS X so that I can better replicate your results? |
Hi @anisha-rohra I have access to a Mac running
I've cloned |
Hello @springmeyer, just pinging to find out if you've had a chance to look into our issue with node-addon-api. As it stands, when building on a 10.12 machine with node v9.4, I'm unable to replicate your issue. I'd appreciate it if you had to chance to look into any environment specific issues that would cause this differentiation so that I can try to recreate your environment and fix the failing test case. |
I've committed the patch you provided @springmeyer to our fork. Would it be possible for you to create a node-addon-api branch for me to create a PR against? |
@anisha-rohra okay, I've created a branch/PR at #979. Is that what you meant? In the process of creating it I noticed that two tests were commented out. I re-enabled them and noticed that they now hang/timeout for me. Do they hang for you? They are https://github.com/mapbox/node-sqlite3/blob/8f73e6f74991910c2712c31e0aca6c6d41c2465c/test/other_objects.test.js#L18-L40 |
Yes they do hang for me too, the person previously working on the port had them commented out because napi currently doesn't support checking whether an Object is a Date or a RegExp object. I'm working on adding that functionality, and then I'll make the changes to our port such that those tests pass. I'll ping you when the needed changes have been made. |
@anisha-rohra any progress on fixing the hang? |
@anisha-rohra - I was able to replicate the problem on travis. While it hangs on OS X it crashes on linux/travis with:
See https://travis-ci.org/mapbox/node-sqlite3/jobs/395989495#L571 Please let me know if you know a fix. |
Found nodejs/node-addon-api#57. |
I'm now actively working on a PR to address this issue. My immediate objective is to apply the commits @sampsongao and @anisha-rohra previously created for this issue to the current codebase up to the point where the libuv dependencies need to be addressed. |
At this point I've got nearly all the unit tests passing. I will be working with Michael Dawson to configure my development system to debug the remains failing unit tests. |
@jschlight - great news! Please push up a PR as soon as you have something ready. Thank you. |
@jschlight any update on progress here? |
Michael Dawson and I were able to resolve the issues with my development system. I am able to build and run the N-API module on my development system and run the Mocha tests in |
The Mocha tests are all passing on my Mac development system, but the AppVeyor and Travis CI operations are filling in the compile process. I'm now investigating why that's the case. |
I've rebased @jschlight's branch on the latest master and converted the new code added to N-API. I've also replaced the libuv async work code with N-API code. Disabling N-API C++ exceptions helped reveal why some tests were failing which I fixed. Some tests are still failing and there are some random segmentation faults on some platforms if anyone wants to take a look. |
Thank you so much @mohd-akram for your work on this. I've merged your changes into my fork and resolved a handful of conflicts. I'm now looking into the CI errors. Is it possible to see the errors you're getting from Travis CI and AppVeyor for your fork? The N-API team meets for an hour each week on Zoom. You are most welcome to join us to answer any questions you may have. You can see the meeting specifics on the Node.js Foundation Calendar under the heading Node.js N-API team weekly meeting. The meeting is currently held on Mondays. |
You can see the failures here. I was able to avoid the segmentation fault in Node.js 8 on macOS by modifying the |
Thank you @mohd-akram. I will look at this more closely first thing in the morning. |
@mohd-akram We discussed your macOS issue during today's N-API Team call. It was suggested that it might help to set I'd like to see if we can get to a common codebase. Do you see any issues with me deleting my fork of mapbox/node-sqlite3 and forking off of your project instead? |
@jschlight @mohd-akram - what are the next steps on getting node-sqlite3 upgraded to use N_API? If you have something that is close to ready I recommend creating a PR against this repo. Thanks! |
I've rebased and created PR #1304. |
@springmeyer @kewde At the request of the N-API team, I've run some benchmarks on the N-API implementation compared to the NAN version. The results are available here: nodejs/abi-stable-node#288 (comment) Overall, the I'm now monitoring incoming |
The recently announced Node 8 has a new experimental feature called N-API which is aimed at reducing maintenance cost for node native addons. Checkout this blog for more details on its benefits.
Module
node-sqlite3
is one of the top 30 native modules by dependency count, and in order to help the Node.js community make the important transition to N-API, we are hoping you will be able to work with us in order to portnode-sqlite3
to support N-API. Your support and feedback is important in making this effort a success.I am part of the N-API working group and and would like to talk to you about how you can can get started with N-API and what help we might be able to provide. I'm available to talk on the phone individually if you'd like. Alternatively, if you prefer, feel free to jump in our WG meeting which happens every Thursday at 1.30 Eastern / 10.30 Pacific US time, to discuss any issues.
Here’s a video of N-API in action
The text was updated successfully, but these errors were encountered: