prebuildify icon indicating copy to clipboard operation
prebuildify copied to clipboard

Support `--napi` and `--all` together

Open aminya opened this issue 5 years ago • 8 comments

This makes it possible to provide --napi and --all together. This means older Node versions can also be built using prebuildify.

aminya avatar Oct 20 '20 03:10 aminya

Just curious, why would you want to? A major benefit of N-API is forwards compatibility. If you make a prebuild for node 8 it'll also work on 10, 12, 14 and future.

vweevers avatar Oct 20 '20 06:10 vweevers

n-api is still software after all. They may want to break something 😄 We use CI to build our executable, so building more does not really make a difference for us. It would be better to build against the exact version.

Our code does not build with older versions of Node when the target is newer. See my comment here: https://github.com/prebuild/prebuildify/issues/10#issuecomment-712532649

aminya avatar Oct 20 '20 06:10 aminya

They may want to break something

Not likely, that goes against its design philosophy.

Note that including more prebuilds will increase your package size.

Regarding the referenced issue, there is a fix (PR welcome): https://github.com/prebuild/prebuildify/issues/10#issuecomment-510804377

vweevers avatar Oct 20 '20 06:10 vweevers

Well, I don't have a direct solution for #10. This PR is what I have come up with so far.

aminya avatar Oct 20 '20 06:10 aminya

For #10 we just need:

if (runtime === 'node') {
  // work around bug introduced in node 10's build https://github.com/nodejs/node-gyp/issues/1457
  args.push('--build_v8_with_gn=false')
}

Would you like to send a new PR? Thanks!

vweevers avatar Oct 25 '20 12:10 vweevers

For #10 we just need:

if (runtime === 'node') {
  // work around bug introduced in node 10's build https://github.com/nodejs/node-gyp/issues/1457
  args.push('--build_v8_with_gn=false')
}

Would you like to send a new PR? Thanks!

OK. I will make a new PR, but the feature that this PR adds is separate from that issue.

aminya avatar Oct 28 '20 06:10 aminya

Could you merge this? Your suggestion is independent of what this adds. #10 is already fixed.

aminya avatar Nov 28 '20 08:11 aminya

Bump

aminya avatar Jan 06 '21 02:01 aminya