-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
napi
is no longer output as a tag
#79
Comments
Def oversight! Lets just make it assume its tagged with napi if named |
Ok, here is the logic I'm going with; 1 . Ensure we utilize 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 |
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)
I'm unsure if this was intentional when setting
napi
to default, and upstream tools are meant to just "assume" that everything isnapi
, however this change;7b6dcbd#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 node-usb/node-usb#743I 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 withnode-gyp
style prebuilds and can't tell if this was intended or not. It seems more like a bug to me.Thanks!
The text was updated successfully, but these errors were encountered: