-
Notifications
You must be signed in to change notification settings - Fork 37
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
base: master
Are you sure you want to change the base?
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)
Latest node-gyp-build should find it still |
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. |
I'm not 100% sure this will, the issue raised above for |
This all worked for me locally with node-usb when I submitted the PR.
Though, this was a few months ago so maybe latest has drifted or changed.
To be honest, this entire thing has left my brain as I fixed it locally and
then node-usb reverted back anyway.
I'm unsure if this PR is stale, but I didn't want to hound anyone for
trying to get it merged.
…On Thu, May 2, 2024, 14:13 Rob Moran ***@***.***> wrote:
Latest node-gyp-build should find it still
I'm not 100% sure this will, the issue raised above was using
***@***.*** and ***@***.***
—
Reply to this email directly, view it on GitHub
<#80 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEYIRRP536H6U77DM6TK3LZAKT6FAVCNFSM6AAAAABELROUOCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJRGYZTEMBZGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Ah! I'm caught up again. @thegecko I think you're misunderstanding the context. The latest The comment you're quoting is in response to my inline comment here;
|
I was referring to the comment from @mafintosh
Which suggests this issue is fixed in the latest |
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.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 thepackage.json
. This commit also dropped including thenapi
tag, which is still utilized downstream.This change fixes the regression and follows this logic;
name
as a tag (this will always be true due toaddName
function logic)runtime
as a tag alwaysnapi
as a tag if option enabled (likely always true, as it if defaulted to true)