prebuild / prebuildify

Create and package prebuilds for native modules
MIT License
200 stars 37 forks source link

Support `--napi` and `--all` together #46

Open aminya opened 4 years ago

aminya commented 4 years ago

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

vweevers commented 4 years ago

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.

aminya commented 4 years ago

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

vweevers commented 4 years ago

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

aminya commented 4 years ago

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

vweevers commented 4 years ago

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!

aminya commented 4 years ago

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 commented 4 years ago

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

aminya commented 3 years ago

Bump