N-API Support #345

inspiredware · 2018-02-05T18:44:05Z

This pull request implements changes necessary to support the new N-API ABI-stable API for building Node native add-ons. The N-API Working Group discussion motivating these changes can be found here:

nodejs/abi-stable-node#263

I've thoroughly tested this code on Mac, Windows, and ARM. In addition, I've included a new app7 project for automated testing.

All build.test.js automated tests pass on Mac, Windows, and ARM with the sole exception of test 4 for app1 on Windows only.

TAP version 13
# app1 passes --nodedir down to node-gyp via node-pre-gyp
ok 1 Expected command to fail
ok 2 should contain common.gypi not found
# app1 passes --nodedir down to node-gyp via npm
ok 3 Expected command to fail
not ok 4 should contain common.gypi not found

I'm fairly confident this failure is not related to the changes I've made to support N-API builds.

I'm anxious to receive your comments and suggestions as landing this PR is necessary for N-API to leave experimental status.

bnoordhuis

Not a maintainer, just some drive-by comments.

bnoordhuis · 2018-02-06T17:41:42Z

lib/build.js

@@ -16,7 +17,13 @@ function do_build(gyp,argv,callback) {
                          concat(['--']).
                          concat(result.unparsed);
        }
+		if (!err && result.opts.napi_build_version) {
+			napi.swap_build_dir_in(result.opts.napi_build_version);
+		}


You're using tabs but, looking at the surrounding code, it should be spaces. Happen in a few other places too.

bnoordhuis · 2018-02-06T17:49:24Z

lib/util/napi.js

+
+module.exports.validate_package_json = function(package_json) { // return err
+	var binary = package_json.binary;
+	var module_path_ok = binary.module_path && binary.module_path.includes('{napi_build_version}');


I'm not sure what node versions node-pre-gyp supports but String.p.includes() doesn't exist in node.js <= v0.12. Use String.p.indexOf() if that's an issue.

bnoordhuis · 2018-02-06T17:55:13Z

lib/util/napi.js

+
+	if (napi_build_versions) {
+		napi_build_versions.forEach(function(napi_build_version){
+			if (!((parseInt(napi_build_version) === napi_build_version) && (napi_build_version > 0))) {


I'd be explicit about the base and write parseInt(napi_build_version, 10). Likewise on line 119.

Aside: your clauses have some superfluous parentheses.

bnoordhuis · 2018-02-06T18:00:55Z

test/app7/binding.gyp

+      "sources": [ "<(module_name).cc" ],
+      'product_dir': '<(module_path)',
+      "xcode_settings": {
+        "MACOSX_DEPLOYMENT_TARGET":"10.9",


Just curious, is there any particular reason to set this?

I created app7 by copying one of the existing test apps as a starting point. The declaration came along for the ride.

I appreciate your taking the time to review my code and for your constructive suggestions. Thank you.

inspiredware · 2018-02-08T17:30:48Z

I am working to determine why the builds for Node versions < 8 are failing.

inspiredware · 2018-02-13T04:22:21Z

At this point, all of the Travis CI build tests are completing successfully.

For the AppVeyor builds, two tests for each of the builds for Node 8+ are failing. The tests are related to downloading the pre-built binaries. The reason for the failures is the fact that the current node-pre-gyp release, on which the tests rely, does not support the new text substitution implemented by this pull request.

In the test logs we can see that the test is attempting to download the following URL:

https://node-pre-gyp-tests.s3-us-west-1.amazonaws.com/app7/v0.1.0/Release/app7-v0.1.0-win32-x64-napi-v%7Bnapi_build_version%7D.tar.gz

Notice the URL contains the substitution text %7Bnapi_build_version%7D which is not supported by the current node-pre-gyp release.

Manually substituting the supported N-API versions 1 and 2 results in URLs that successfully download.

https://node-pre-gyp-tests.s3-us-west-1.amazonaws.com/app7/v0.1.0/Release/app7-v0.1.0-win32-x64-napi-v1.tar.gz
https://node-pre-gyp-tests.s3-us-west-1.amazonaws.com/app7/v0.1.0/Release/app7-v0.1.0-win32-x64-napi-v2.tar.gz

If you'd like, I can add code to build-test.js to temporarily disable the two failing tests. I'd then plan to create another PR to reenable the tests after this PR lands.

Otherwise, I feel this PR is ready to merge. However, I'm open to any suggestions to improve it.

mhdawson · 2018-02-15T18:38:35Z

@inspiredware looks like you addressed the CI issues and it will be green once the PR changes. Looking forward to seeing it land.

inspiredware · 2018-02-20T21:44:54Z

The N-API Working Group is meeting tomorrow. Would it be possible for the maintainers to post an update here, even if it's just an indication of when you think you'll be able to look at this PR? Many thanks!

springmeyer · 2018-03-10T07:34:19Z

@inspiredware excellent work on this. I've reviewed (finally, sorry for the wait), and will merge now. This will land in v0.8.0.

springmeyer · 2018-03-10T07:45:12Z

hmm, holding on v0.8.0 release. @inspiredware I'm seeing https://travis-ci.org/mapbox/node-pre-gyp/jobs/351625424#L990 on travis:

  node-pre-gyp ERR! stack Error: 404 status code downloading tarball https://node-pre-gyp-tests.s3-us-west-1.amazonaws.com/app7/v0.1.0/Release/app7-v0.1.0-linux-x64-napi-v%7Bnapi_build_version%7D.tar.gz

For node v8 and v9 - any idea what might be wrong? Perhaps the v{napi_build_version} token is not getting properly replaced?

springmeyer · 2018-03-10T15:45:29Z

@inspiredware found the problem with the tests: b22612c

springmeyer · 2018-03-10T16:07:33Z

node-pre-gyp@0.8.0 is now released with N-API support. Thanks @inspiredware and team: https://www.npmjs.com/package/node-pre-gyp

mhdawson · 2018-03-13T22:57:04Z

Many thanks @springmeyer

aruneshchandra · 2018-03-14T18:23:36Z

thanx a lot @springmeyer !

inspiredware · 2018-03-14T23:49:58Z

Thank you @springmeyer. I'm available to diagnose and correct any issues that may arise.

rolftimmermans · 2018-03-31T09:22:08Z

@inspiredware Can I ask for clarification on how this feature is meant to be used? If I understand correctly the N-API version replaces the Node module version and instead of publishing one binary for each platform + Node module version, we now have to publish a binary for each platform + N-API version?

I am attempting to build a module with node-pre-gyp that uses N-API. I am running into a few issues, and wonder what your opinion is on them. Am I using this wrong?

My CI process basically executes node-pre-gyp configure and node-pre-gyp build followed by tests. This causes the module to be built 3 times (once for each N-API version defined in my package.json – version 1, 2 and 3). This confuses me, can I not just only get the most recent version to build? Surely building all N-API versions with a single Node version is not very useful, is it, since the N-API version in use by that version of Node will be the same anyway? This suspicion seems confirmed by the fact that the resulting binary for different N-API versions seems 100% identical.
Consequently, when building on an earlier version of Node in CI (say version 8.8) the module will also be attempted to be built for N-API versions that are higher than the one that is available in version 8.8 of Node. This causes the build to fail because I am checking for NAPI_BUILD_VERSION to compile a polyfill implementation for recent N-API features. When the module is compiled with NAPI_BUILD_VERSION set to 3 in Node 8.8 (which does not have N-API version 3), compilation fails. I expect only the current version of N-API included with the running version of Node to be built...? I'm not sure how the build could ever succeed for N-API versions beyond the one of the current Node runtime?
How can I know what to put in "napi_versions" in package.json? The N-API build feature of node-pre-gyp now seems rather naive; it just restarts the build for each listed version, regardless whether they exist at all...
I have absolutely no idea how the N-API integrates with the node-addon-api package, but perhaps that's a separate issue.

Would love to hear your thoughts on these issues. I'm eager to get this all working and publish a stable N-API enabled module but not really succeeding so far.

If you wish to take a look, the current code can be found in https://github.com/rolftimmermans/zeromq-ng/tree/various-nodes.

inspiredware · 2018-04-01T16:34:28Z

@rolftimmermans thank you for your questions. They help us improve our documentation for all users.

One of the primary benefits of N-API is that the API is "ABI stable." What this means practically is that an N-API add-on you publish today will work indefinitely for all future versions of Node. Today the N-API version is 2. So, if you build today with "napi_versions": [2], your add-on will work today and with all future versions of N-API. node-pre-gyp gives you the ability to build this version for multiple platforms.

You mention that you're creating a polyfill for features recently added to N-API. I assume those features would have been added in version 2. So in this case you need to build against versions 1 and 2. As you mention, this will fire off two builds, with NAPI_BUILD_VERSION set to 1 and 2 respectfully. You can use the NAPI_BUILD_VERSION define for conditional compilation.

This build will continue to work for all future versions of N-API beyond version 2.

The question of building with earlier versions of Node is confusing because builds can occur in two different settings: 1. You build as a publisher. This is the whole purpose of node-pre-gyp. 2. Builds can also occur when a user attempts to install your add-on and a pre-built binary is not available.

When you build as a publisher, you need to be building with a version of Node that supports the highest N-API version against which you are building. If you declare that your module supports N-API version 3, you need to be building with a version of Node that supports N-API version 3. A version of Node that supports a certain N-API version can also be used to build an add-on for all previous N-API versions. I agree that the current implementation is naive and will add a version check.

If your users are not able to download one of your pre-built binaries, they will need to build it themselves. In that case 'node-pre-gyp` checks to see the N-API version supported by the Node they have installed. It chooses the highest version of your add-on supported by the user's Node and build against just that one version. If the user's Node version is not high enough, the user receives an error message and the build fails.

The node-addon-api package is an awesome tool and is highly recommended. It is a set of C++ header files that make it really easy to create and manipulate JavaScript objects from C++.

I hope I've answered your questions. If not, please let me know.

rolftimmermans · 2018-04-02T15:08:16Z

@inspiredware Thanks for your excellent detailed response. This clears up a lot of confusion for me.

I have one remaining question: what is the story for running tests and CI? Say I want to test the module with a certain, slightly older Node version that supports N-API but does not have the latest N-API version yet (example: some Node 8.x with N-API v1). How do I build it for just this N-API version? This is something I haven't been able to figure out. Previously I would just do a node-pre-gyp build but that now builds all N-API versions and therefore can fail in this scenario with N-API versions higher than the one supported by the targeted runtime.

inspiredware · 2018-04-05T20:33:56Z

@rolftimmermans as we've discussed, I'll work on making the following improvements to the N-API support for node-pre-gyp.

When building for publishing, node-pre-gyp will verify that the build for each N-API version specified in the package.json file is supported by the version of Node being used for the build. If the version is not supported, node-pre-gyp will issue a warning. If no version can be built, node-pre-gyp will issue an error. This will permit add-ons to be tested against older versions of Node that do not support newer N-API versions.
Implement a new build configuration flag, something like --build-current-napi-version-only, that will build only the highest supported N-API version. This will be useful for test builds where only the most current N-API build is of interest by suppressing builds that are eventually ignored.
Improve recovery from build errors so that a configure is not required after a build failure.

Please let know if there is anything else that comes to mind. Thanks again for your help improving this code.

jschlight · 2018-05-31T00:37:26Z

@ghost here, formerly @inspiredware. Just a note that I've not forgotten these proposed changes and expect to get to them in the next couple of weeks.

jschlight · 2018-07-02T19:02:01Z

This work is continued at #402.

N-API Support

jschlight added 5 commits January 31, 2018 18:19

Initial commit.

8bc03dc

Installation fixes, app7 test app, README.md

35c20f7

Windows fixes

411f5be

Fixes clean and app7 for automated testing

1122fdb

Fix for reveal command

f0719bd

jschlight mentioned this pull request Feb 5, 2018

Investigate integration/awareness of N-API with node pre-gyp nodejs/abi-stable-node#263

Closed

bnoordhuis reviewed Feb 6, 2018

View reviewed changes

jschlight added 2 commits February 7, 2018 14:53

Code cleanup.

82a641e

Fix for code cleanup

488ac7b

jschlight added 3 commits February 8, 2018 09:56

CI tweaks

8f7c497

Addresses CI build errors

9870491

Another CI build tweak

9684ef6

mhdawson mentioned this pull request Mar 5, 2018

Bring N-API out of experimental nodejs/TSC#501

Closed

Merge branch 'master' into napi-support

c31cce4

springmeyer merged commit 9bb97af into mapbox:master Mar 10, 2018

inspiredware deleted the napi-support branch March 14, 2018 23:50

lovell mentioned this pull request Mar 16, 2018

N-API support lovell/farmhash#19

Closed

springmeyer mentioned this pull request Mar 21, 2018

Packages have too many files in them. #356

Closed

springmeyer mentioned this pull request Apr 26, 2018

Port to N-API mapbox/node-cpp-skel#45

Closed

This was referenced May 31, 2018

Investigate use of prebuild with N-API nodejs/abi-stable-node#291

Closed

Add support for N-API builds prebuild/prebuild#220

Closed

jschlight mentioned this pull request Jun 25, 2018

node v4 error when using napi_versions/napi_build_versions #396

Open

This was referenced Aug 5, 2021

[Snyk] Security upgrade node-pre-gyp from 0.5.31 to 0.8.0 ilatypov/node-pre-gyp#5

Open

[Snyk] Security upgrade node-pre-gyp from 0.5.31 to 0.8.0 ilatypov/node-pre-gyp#7

Open

snyk-bot mentioned this pull request Sep 1, 2021

[Snyk] Security upgrade node-pre-gyp from 0.5.31 to 0.8.0 ilatypov/node-pre-gyp#10

Open

hyj1991 pushed a commit to X-Profiler/node-pre-gyp that referenced this pull request Jun 16, 2023

Merge pull request mapbox#345 from inspiredware/napi-support

c50fe56

N-API Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

N-API Support #345

N-API Support #345

inspiredware commented Feb 5, 2018

bnoordhuis left a comment

bnoordhuis Feb 6, 2018

bnoordhuis Feb 6, 2018

bnoordhuis Feb 6, 2018

bnoordhuis Feb 6, 2018

jschlight Feb 7, 2018

inspiredware commented Feb 8, 2018

inspiredware commented Feb 13, 2018

mhdawson commented Feb 15, 2018

inspiredware commented Feb 20, 2018

springmeyer commented Mar 10, 2018

springmeyer commented Mar 10, 2018

springmeyer commented Mar 10, 2018

springmeyer commented Mar 10, 2018

mhdawson commented Mar 13, 2018

aruneshchandra commented Mar 14, 2018

inspiredware commented Mar 14, 2018

rolftimmermans commented Mar 31, 2018 •

edited

Loading

inspiredware commented Apr 1, 2018

rolftimmermans commented Apr 2, 2018 •

edited

Loading

inspiredware commented Apr 5, 2018

jschlight commented May 31, 2018 •

edited

Loading

jschlight commented Jul 2, 2018

N-API Support #345

N-API Support #345

Conversation

inspiredware commented Feb 5, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Feb 6, 2018

Choose a reason for hiding this comment

bnoordhuis Feb 6, 2018

Choose a reason for hiding this comment

bnoordhuis Feb 6, 2018

Choose a reason for hiding this comment

bnoordhuis Feb 6, 2018

Choose a reason for hiding this comment

jschlight Feb 7, 2018

Choose a reason for hiding this comment

inspiredware commented Feb 8, 2018

inspiredware commented Feb 13, 2018

mhdawson commented Feb 15, 2018

inspiredware commented Feb 20, 2018

springmeyer commented Mar 10, 2018

springmeyer commented Mar 10, 2018

springmeyer commented Mar 10, 2018

springmeyer commented Mar 10, 2018

mhdawson commented Mar 13, 2018

aruneshchandra commented Mar 14, 2018

inspiredware commented Mar 14, 2018

rolftimmermans commented Mar 31, 2018 • edited Loading

inspiredware commented Apr 1, 2018

rolftimmermans commented Apr 2, 2018 • edited Loading

inspiredware commented Apr 5, 2018

jschlight commented May 31, 2018 • edited Loading

jschlight commented Jul 2, 2018

rolftimmermans commented Mar 31, 2018 •

edited

Loading

rolftimmermans commented Apr 2, 2018 •

edited

Loading

jschlight commented May 31, 2018 •

edited

Loading