-
Notifications
You must be signed in to change notification settings - Fork 37
(fix) Ensure runtime and napi are emitted as needed. #80
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
Conversation
Per issue prebuild#79 there seems to be a regression introduced in 7b6dcbd which caused the `target.runtime` to no longer be emitted when a name is used. A name is also _always_ used even if not passed, as it will grab a name from the `package.json`. This commit also dropped including the `napi` tag, which is still utilized downstream. This change fixes the regression and follows this logic; 1. Emit `name` as a tag (this will always be true due to `addName` function logic) 2. Emit `runtime` as a tag always 3. Emit `napi` as a tag if option enabled (likely always true, as it if defaulted to true)
mafintosh
commented
Mar 11, 2024
Latest node-gyp-build should find it still
strazzere
commented
Mar 11, 2024
Agreed - I only skimmed and did slim testing on it. Just wanted to call it out as I'm not overly familiar with that code. If it seems good to you I have no issues with it - was just trying to go for being overly specific.
Latest node-gyp-build should find it still
I'm not 100% sure this will, the issue raised above for node-usb@2.12.0 was using prebuildify@6.0.0 and node-gyp-build@4.8.0 which are (nearly) the latest of both libraries, right?
strazzere
commented
May 2, 2024
via email
strazzere
commented
May 2, 2024
Ah! I'm caught up again. @thegecko I think you're misunderstanding the context. The latest node-gyp-build will not fix the issue you're seeing. This PR needs to be merged.
The comment you're quoting is in response to my inline comment here;
#80 (comment)
node-usb won't work to pull prebuilts until the PR is merged AFAIK.
thegecko
commented
May 2, 2024
I was referring to the comment from @mafintosh
Latest node-gyp-build should find it still
Which suggests this issue is fixed in the latest node-gyp-build, but I wanted to clarify it isn't (without this fix) AFAICT
strazzere
commented
May 2, 2024
Understood, all caught up. You are correct - I believe the comment was meant to be in response to my (then resolved) comments inlined.
Fixes issue #79.
There seems to be a regression introduced in 7b6dcbd which caused the
target.runtimeto no longer be emitted when a name is used. A name is also always used even if not passed, as it will grab a name from thepackage.json. This commit also dropped including thenapitag, which is still utilized downstream.This change fixes the regression and follows this logic;
nameas a tag (this will always be true due toaddNamefunction logic)runtimeas a tag alwaysnapias a tag if option enabled (likely always true, as it if defaulted to true)