Skip to content
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 use of prebuild with N-API #291

Closed
mhdawson opened this issue Jan 18, 2018 · 13 comments
Closed

Investigate use of prebuild with N-API #291

mhdawson opened this issue Jan 18, 2018 · 13 comments
Assignees
Milestone

Comments

@mhdawson
Copy link
Member

We've been looking at node pre-gyp here #263 should also check out prebuild which is a similar but different tool.

@aruneshchandra aruneshchandra added this to the Milestone 9 milestone Jan 25, 2018
@mhdawson mhdawson removed this from the Milestone 9 milestone Mar 15, 2018
@mhdawson mhdawson added this to the Milestone 10 milestone Apr 5, 2018
@mhdawson
Copy link
Member Author

mhdawson commented Apr 5, 2018

From @nicknasso sounds like this is a blocker for some add-ons to port so moving to higher priority.

@jschlight
Copy link
Collaborator

Since this is related to node-pre-gyp, I'll take a look at this.

@NickNaso
Copy link
Member

NickNaso commented May 14, 2018

@jschlight It could be interesting if it's possible to follow the similar approach used for node-pre-gyp

@jschlight
Copy link
Collaborator

jschlight commented May 31, 2018

I'm posting these comments here for review and comment by the working group.

2018-06-06 I've spent time stepping through the prebuild code and have updated these recommendations. To simplify things, I've removed the recommendation for the napi_versions array property as used in node-pre-gyp in favor of using the exisiting prebuild command line arguments.

2018-06-07 I've incorporated the changes we discussed during today's working group meeting.

Summary

Like node-pre-gyp which we've enhanced for N-API, prebuild appears to be a front end to node-gyp. This means that at a fundamental level, I should be able to apply the same techniques for prebuild that I used for node-pre-gyp.

Essentially, node-pre-gyp is designed to upload binaries to Amazon S3 and other locations, where prebuild uploads to GitHub. So node-pre-gyp requires additional configuration information to specify the upload location and filename format.

The upload and download features are bundled into node-pre-gyp where prebuild has an additional prebuild-install module for downloads that will also need to be enhanced as part of this work.

Unlike node-pre-gyp which is focussed entirely on Node, prebuild also supports builds for Electron and NW.js. The changes I'm recommending below would apply only to the Node builds.

Proposed command line changes

The --runtime parameter

There are currently three runtime values that can be specified: node, electron, node-webkit. I propose adding a new runtime value of node-napi to indicate an N-API build.

The --target parameter

If an N-API build is requested by the runtime parameter, the target value is interpreted to be the N-API version for which the build, or builds, is requested. Zero or more target arguments may be specified.

If an N-API build is requested, and no target argument is supplied, a single build for N-API version 3 will be requested. This is the earliest, widely distributed version of N-API.

The --all parameter

If an N-API build is requested, the all augment is ignored. Only those builds determined by the optional target argument(s) will be built.

The --backend parameter

It is possible that N-API builds will be possible only with the node-gyp backend. This will become more clear as coding work progresses.

The napi_build_version value

For each of the N-API module operations prebuild initiates, it insures that the napi_build_version is set appropriately.

This value is of importance in two areas:

  1. The C/C++ code which needs to know against which N-API version it should compile.
  2. prebuild itself which must assign appropriate path and file names for the uploaded binaries.

Defining NAPI_VERSION for the C/C++ code

The napi_build_version value is communicated to the C/C++ code by adding this code to the binding.gyp file:

"defines": [
    "NAPI_VERSION=<(napi_build_version)",
]

This insures that NAPI_VERSION, an integer value, is declared appropriately to the C/C++ code for each build.

Filename format

To keep things compatible with node-pre-gyp, I propose to use the following filename format for N-API builds:

{module_name}-v{version}-napi-v{napi_build_version}-{platform}-{arch}.tar.gz

My to-do list before serious development

  1. Determine how these proposed changes fit in with the current prebuild implementation. Specifically, the command line arguments.

  2. Review the commitments I've made to improving node-pre-gyp to make sure they make sense for prebuild also.

  3. Review and understand the recommendations here and here for supporting both NAN and N-API in the same module. Insure that node-pre-gyp and prebuild support these recommendations.

@jschlight
Copy link
Collaborator

As noted above, I've added an issue to the prebuild project to track this issue. prebuild/prebuild#220

@jschlight
Copy link
Collaborator

jschlight commented Aug 8, 2018

I've got the basic functionality working for builds, uploads, and downloads. This includes the separate prebuild-install project. I just need to cleanup the code a bit, create and verify some tests, and create the documentation.

I have one question for the group including @gabrielschulhof.

As outlined above, the --runtime parameter I'm adding is node-napi. This results in package names that look something like:

module-name-v1.0.0-node-napi-v2-darwin-x64.tar.gz

This differs somewhat from node-pre-gyp. I'd like to propose that instead of using node-napi for the --runtime parameter, we use napi instead. This would result in package names that are more in alignment with those generated by node-pre-gyp:

module-name-v1.0.0-napi-v2-darwin-x64.tar.gz

IIRC, Gabriel had expressed a concern about eventually needing prebuild to support Electron N-API builds. I'm thinking that for the --runtime parameter we use napi now for Node N-API builds, and add electron-napi later if needed.

Any comments?

@mhdawson
Copy link
Member Author

mhdawson commented Aug 8, 2018

@jschlight I think your suggestion to use napi instead of node-api is reasonable.

@jschlight
Copy link
Collaborator

I will go ahead and change the --runtime parameter from node-napi to napi.

@jschlight
Copy link
Collaborator

There are now two PRs. One for prebuild and another for prebuild-install.

@mhdawson mhdawson modified the milestones: Milestone 10, Milestone 11 Sep 27, 2018
@jschlight
Copy link
Collaborator

The two PRs have landed and the modules have been published to npm.

@NickNaso
Copy link
Member

NickNaso commented Sep 27, 2018

It's really a grate news. Good work @jschlight. Last week I opened a PR where I added a section about pre build tools. See https://github.com/nodejs/node-addon-api/blob/7f7e78534e85cae0c4685158b5187383fca1597c/doc/prebuild_tools.md.
Now I think that I can add prebuild in the list of the tools the support N-API.

@mhdawson
Copy link
Member Author

@jschlight good work and great to hear :)

@mhdawson
Copy link
Member Author

mhdawson commented Oct 4, 2018

Jim reported it landed in the meeting today. Hooray ! closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants