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

Support --napi and --all together #46

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Oct 20, 2020

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

@vweevers
Copy link
Member

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
Copy link
Contributor Author

aminya commented Oct 20, 2020

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:
#10 (comment)

@vweevers
Copy link
Member

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): #10 (comment)

@aminya
Copy link
Contributor Author

aminya commented Oct 20, 2020

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

@aminya aminya mentioned this pull request Oct 20, 2020
@vweevers
Copy link
Member

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
Copy link
Contributor Author

aminya commented Oct 28, 2020

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
Copy link
Contributor Author

aminya commented Nov 28, 2020

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

@aminya
Copy link
Contributor Author

aminya commented Jan 6, 2021

Bump

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

Successfully merging this pull request may close these issues.

2 participants