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

Figure out which version to build prebuilts on #553

Closed
ralphtheninja opened this issue Dec 15, 2018 · 19 comments
Closed

Figure out which version to build prebuilts on #553

ralphtheninja opened this issue Dec 15, 2018 · 19 comments
Milestone

Comments

@ralphtheninja
Copy link
Member

ralphtheninja commented Dec 15, 2018

From https://nodejs.org/en/docs/guides/abi-stability/

A given version n of N-API will be available in the major version of Node.js in which it was published, and in all subsequent versions of Node.js, including subsequent major versions.

This tells me that if you build on the minimum possible node version (for leveldown this means v8.6.0) that will cover all future versions. Should we pick this strategy when making prebuilt binaries? At the moment it seems to work well to pick the current node version (v11.4.0 at time of writing) and make it work for earlier versions, but afaik n-api is not guaranteed to be backwards compatible, right?

Input appreciated.

EDIT:

Slightly below above mentioned quote it says:

That is, each version of N-API conveys the same meaning as a minor version in the semver system, meaning that all changes made to N-API will be backwards compatible.

So it seems we might as well pick the latest version of node.

@ralphtheninja ralphtheninja added this to the 5.0.0-rc1 milestone Dec 15, 2018
@ralphtheninja
Copy link
Member Author

Note: This hasn't been true from the start though, since n-api in node v8.0.0 has a different api compared to v8.6.0 (e.g. napi_create_error()).

@ralphtheninja ralphtheninja changed the title Figure out which version to build prebuilts Figure out which version to build prebuilts on Dec 15, 2018
@vweevers
Copy link
Member

@gabrielschulhof @mcollina Could you advise us here?

@mcollina
Copy link
Member

n-api has exited experimental status a little later in the Node 8 LTS cycle. I would build it with the latest version of older LTS line that it is supported. Currently, latest Node 8.

@vweevers
Copy link
Member

Unfortunately, prebuildify --napi always targets latest node:

https://github.com/mafintosh/prebuildify/blob/5fe60fb80635f0945f7ea64b1ba4ab4c8a27d049/bin.js#L27-L34

@ralphtheninja
Copy link
Member Author

ralphtheninja commented Dec 16, 2018

@vweevers Maybe we can patch prebuildify to do something like:

if (argv.napi && !targets) {
  targets = [
    abi.supportedTargets.filter(onlyNode).pop(),
    abi.supportedTargets.filter(onlyElectron).pop(),
  ]

  if (targets[0].target === '9.0.0') targets[0].target = '9.6.1'
}

Then we should be able to do prebuildify --target whatever --napi

@vweevers
Copy link
Member

I was thinking that too, but from the current code, it seems @mafintosh has different ideas about what the most suitable node version is?

BTW, should we also define an explicit NAPI_VERSION? Which:

restricts the N-API surface to just the functionality that was available in the specified (and earlier) versions

@ralphtheninja
Copy link
Member Author

I was thinking that too, but from the current code, it seems @mafintosh has different ideas about what the most suitable node version is?

Yes.

BTW, should we also define an explicit NAPI_VERSION?

Yeah, we can do this. This should be set to 3 (which is the version used at v8.x LTS)

@vweevers
Copy link
Member

if (argv.napi && !targets) {
  targets = [
    abi.supportedTargets.filter(onlyNode).pop(),
    abi.supportedTargets.filter(onlyElectron).pop(),
  ]

  if (targets[0].target === '9.0.0') targets[0].target = '9.6.1'
}

That doesn't cover electron, unless we call prebuildify twice.

@ralphtheninja
Copy link
Member Author

ralphtheninja commented Dec 16, 2018

We can do (with the patch)

prebuildify -t 8.14.0 -t electron@3.0.0 --napi

@vweevers
Copy link
Member

Ah, of course. 👍

@ralphtheninja
Copy link
Member Author

There's a TODO comment in there as well to use --lts. Maybe we can do something about that?

@ralphtheninja
Copy link
Member Author

On second thought, I think I prefer using -t because then we can do prebuildify -t $(node -v) -t electron@3.0.0 on Travis.

@vweevers
Copy link
Member

There's a TODO comment in there as well to use --lts. Maybe we can do something about that?

Question is what --lts means. Oldest LTS, latest LTS, latest LTS that has non-experimental N-API, ..

On second thought, I think I prefer using -t because then we can do prebuildify -t $(node -v) -t electron@3.0.0 on Travis.

Yeah.

@ralphtheninja
Copy link
Member Author

ralphtheninja commented Dec 16, 2018

Question is what --lts means. Oldest LTS, latest LTS, latest LTS that has non-experimental N-API, ..

This is why I changed my mind again 😄

@vweevers
Copy link
Member

vweevers commented Dec 16, 2018

So, action items:

  • Add #define NAPI_VERSION 3
  • Make PR to prebuildify
  • Patch node-gyp-build to support npm i --build-from-source Patch node-gyp-build to handle --build-from-source #566
  • Update our version of prebuildify
  • Remove node 6 from Travis and AppVeyor, and make prebuilds on node 8, and add electron
  • Remove x86 from AppVeyor?

@ralphtheninja
Copy link
Member Author

prebuild/prebuildify#17

@ralphtheninja
Copy link
Member Author

@vweevers Should we still build on Current/node on appveyor/travis?

@vweevers
Copy link
Member

@vweevers Should we still build on Current/node on appveyor/travis?

No opinion. We will have to update the matrix as node releases come out anyway, because we have to specify the version number on which to prebuild. I'm fine with either current/node or a hardcoded 11.

@ralphtheninja
Copy link
Member Author

@vweevers Closing this.

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

3 participants