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

(fix) Ensure runtime and napi are emitted as needed. #80

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

Conversation

strazzere
Copy link

Fixes issue #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)

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)
index.js Outdated Show resolved Hide resolved
@mafintosh
Copy link
Collaborator

Latest node-gyp-build should find it still

@strazzere
Copy link
Author

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.

@thegecko
Copy link

thegecko commented May 2, 2024

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

strazzere commented May 2, 2024 via email

@strazzere
Copy link
Author

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

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

Understood, all caught up. You are correct - I believe the comment was meant to be in response to my (then resolved) comments inlined.

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.

None yet

3 participants