search

prebuild / prebuildify

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

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

Open strazzere opened 7 months ago

strazzere commented 7 months ago

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)
mafintosh commented 6 months ago

Latest node-gyp-build should find it still

strazzere commented 6 months ago

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 commented 5 months ago

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 5 months ago

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 https://github.com/prebuild/prebuildify/pull/80#issuecomment-2091632094, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEYIRRP536H6U77DM6TK3LZAKT6FAVCNFSM6AAAAABELROUOCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJRGYZTEMBZGQ . You are receiving this because you authored the thread.Message ID: @.***>

strazzere commented 5 months ago

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; https://github.com/prebuild/prebuildify/pull/80#discussion_r1520020289

node-usb won't work to pull prebuilts until the PR is merged AFAIK.

thegecko commented 5 months ago

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 5 months ago

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