search

prebuild / prebuildify

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

`napi` is no longer output as a tag #79

Open strazzere opened 7 months ago

strazzere commented 7 months ago

I'm unsure if this was intentional when setting napi to default, and upstream tools are meant to just "assume" that everything is napi, however this change;

https://github.com/prebuild/prebuildify/commit/7b6dcbd0860a82db8804079ba55663affd9ae555#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346L114-L116

Is causing prebuilt binaries to not be detected, at a minimum, by people using the tooling in node-gyp-prebuilt. E.g. -- node-usb https://github.com/node-usb/node-usb/issues/743

I can submit PR with a testcase and fix for this, if you deem this an oversight on the previous patch. However if not, and this is working as intended, would you be open to a PR to enable outputing napi explicitly? I'm not overly familiar with node-gyp style prebuilds and can't tell if this was intended or not. It seems more like a bug to me.

Thanks!

mafintosh commented 7 months ago

Def oversight! Lets just make it assume its tagged with napi if named

strazzere commented 7 months ago

Ok, here is the logic I'm going with;

1 . Ensure we utilize runtime still when available, as the current logic doesn't allow this if there is a name to use. Emit this.

  1. napi is already defaulted to true if not provided, so we will always add this part when true. Ensure this is emitted as it no longer is being emitted.

There is a potential issue here if people are not careful with their naming, e.g. - it includes some type of tag. Potentially this would be a future PR for using encodeName to prevent any tags from getting set via this?