-
Notifications
You must be signed in to change notification settings - Fork 34
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
Investigate integration/awareness of N-API with node pre-gyp #263
Comments
The module in question is mapbox/node-pre-gyp. There is a nice background piece, Cross-platform addons with node-pre-gyp.
I am researching the current interoperability between |
I've updated my N-API-based module to use There may be a straightforward solution to using |
Okay, so what I’m thinking based off the discussion we’re having right now:
|
Considering that the
"8.9.0": {
"node_abi": 57,
"v8": "6.1"
},
"9.0.0": {
"node_abi": 59,
"v8": "6.2"
} We could piggyback on this file to include N-API version numbers. But I need to know which Node versions support which N-API version. |
Right now there are only 2 N-API versions so it should be easy to figure that out, more complicated is that we were only planning to update once we exited experimental which may complicate things. |
Once option may be to consider that only the first version of 8.X, 6.X etc where N-API is non-experimental is supported but I'm not sure how practical that might be. |
The value of OPTION 1 If the If the This approach essentially says OPTION 2 If the If the In this option, the heuristic needs to be determined only once and should be accurate indefinitely. As deployments of Node supporting |
Note: This proposal was last updated 2018-01-18. It's been superseded by the actual Implementation description found as a comment farther down. I'm posting this proposed implementation for everyone's review. I'm anxious to receive any feedback you may have. Proposed implementationNew path substitution variables Add two new substitution variables for the
New Add an optional new
Changes to publishing Add code to support the
Changes to installing Add code to support the
Concerns
|
The case that I think the array could handle (not with the current description though since it only uses the lowest) is:
If package.json just had the lowest N-API version supported, at install time how would we know Version 2 was available for use ? |
I think I'm getting greater clarity here. If a module specifies If so, the node-pre-gyp publisher will need to fire off three builds and uploads, one targeting each of the three supported versions. Then the correct binary can be located and downloaded at installation time. In the example shown here, the node-pre-gyp publishing system will need to be running a version of Node supporting N-API version 5 or later. I'm assuming that a system running a version of Node supporting N-API version 5 can compile C/C++ code targeting N-API version 4 and lower? Correct? |
If a module specifies "napi-versions":[3,4,5] it is declaring that the code will work under any of those three versions. How does it achieve this? Is the C/C++ code using conditional compilation?
In the example shown here, the node-pre-gyp publishing system will need to be running a version of Node supporting N-API version 5 or later. I'm assuming that a system running a version of Node supporting N-API version 5 can compile C/C++ code targeting N-API version 4 and lower? Correct?
|
I've updated the proposed implementation shown above. The changes are:
|
Overall looks reasonable to me. In terms of This issue could be resolved if we require the napi-versions property to be present if the {napi_version} substitution string is used. It would also greatly simplify the node-pre-gyp installation code by eliminating the need for sequential download requests at installation time.
|
This is an update on the PR I've been working for |
@jschlight sounds like you are making good progress :) |
Note: This implementation note was last updated 2018-01-31. It represents the current implementation of the impending ImplementationNew variables available to
|
I've completed all the coding for the https://github.com/inspiredware/node-pre-gyp/tree/napi-support Although I could submit a PR now, there are still a handful of things I'd like to cleanup before I make a formal PR to the
These tasks will probably take through the end of this week. |
I've completed all of the objectives in the previous comment. However, 3 of the 28 automated tests are failing. The failing tests are related to looking out on the file system for specific files in specific locations. For N-API builds, these files are in different locations. I'd like to see if I can address these three failures before I put in the pull request in on Monday. |
I've just made the PR to Fingers crossed. |
Closing as PR has landed. |
One of the wins for using N-API is not needing a binary for every version of Node. pre-gyp current assumes there is a different one so some investigation/change are needed to optimize for an N-API world.
The text was updated successfully, but these errors were encountered: