Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Switch to N-API prebuilds #331

Merged
merged 3 commits into from
Apr 13, 2021
Merged

Conversation

dennisameling
Copy link
Contributor

@dennisameling dennisameling commented Nov 18, 2020

Closes #330

This PR creates N-API prebuild binaries instead of separate ones for Node and Electron. This is supported by NodeJS versions 8.11.2 and higher: https://nodejs.org/api/n-api.html#n_api_n_api_version_matrix

Testing this PR

Tested this on GitHub Desktop by doing yarn remove keytar && yarn add @dennisameling/keytar-temp (https://github.com/dennisameling/node-keytar/releases/tag/v7.4.99-beta) - it works as expected 🚀

C:\repos\desktop\app>yarn add @dennisameling/keytar-temp
yarn add v1.21.1
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
success Saved lockfile.
success Saved 3 new dependencies.
info Direct dependencies
└─ @dennisameling/keytar-temp@7.1.1-beta
info All dependencies
├─ @dennisameling/keytar-temp@7.1.1-beta
├─ mkdirp-classic@0.5.3
└─ node-addon-api@3.0.2
Done in 18.29s.

Output of prebuild-install --verbose also shows that prebuild is correctly downloading the N-API prebuild binaries:

C:\repos\node-keytar>prebuild-install --verbose
prebuild-install info begin Prebuild-install version 6.0.0
prebuild-install info looking for cached prebuild @ C:\Users\denni\AppData\Roaming\npm-cache\_prebuilds\14731b-keytar-temp-v7.1.1-beta-napi-v3-win32-arm64.tar.gz
prebuild-install http request GET https://github.com/dennisameling/node-keytar/releases/download/v7.1.1-beta/keytar-temp-v7.1.1-beta-napi-v3-win32-arm64.tar.gz
prebuild-install http 200 https://github.com/dennisameling/node-keytar/releases/download/v7.1.1-beta/keytar-temp-v7.1.1-beta-napi-v3-win32-arm64.tar.gz
prebuild-install info downloading to @ C:\Users\denni\AppData\Roaming\npm-cache\_prebuilds\14731b-keytar-temp-v7.1.1-beta-napi-v3-win32-arm64.tar.gz.21480-fe4ffc5c96c92.tmp
prebuild-install info renaming to @ C:\Users\denni\AppData\Roaming\npm-cache\_prebuilds\14731b-keytar-temp-v7.1.1-beta-napi-v3-win32-arm64.tar.gz
prebuild-install info unpacking @ C:\Users\denni\AppData\Roaming\npm-cache\_prebuilds\14731b-keytar-temp-v7.1.1-beta-napi-v3-win32-arm64.tar.gz
prebuild-install info unpack resolved to C:\repos\node-keytar\build\Release\keytar.node
prebuild-install info install Successfully installed prebuilt binary!

@dennisameling
Copy link
Contributor Author

Test release with Windows/macOS/Linux binaries: https://github.com/dennisameling/node-keytar/releases/tag/v7.4.99-beta

Will test extensively in the coming days on different platforms

@dennisameling
Copy link
Contributor Author

Let's put this on hold until develar/app-builder#47 is fixed - that will ensure that prebuild-install will look for N-API prebuilds if a package supports N-API 👍🏼

@shiftkey
Copy link
Contributor

@dennisameling I think I'm interested in revisiting this now that our prebuild support is expanding to other setups (ARM, Alpine, etc).

Did you have any bandwidth coming up to update this PR now that we're migrated over to Actions? I'm happy to take a swing at it if that's not the case, but carving out time is my eternal challenge these days...

@dennisameling
Copy link
Contributor Author

Sorry for the late reply @shiftkey - will try to make some time for this in the coming weekend! 👍🏼

@dennisameling
Copy link
Contributor Author

@shiftkey rebased, but this is dependent on #375 for the failing Ubuntu runs. Let me know if I can be of any help with that PR, have subscribed to it so will continue here once that's done!

@shiftkey
Copy link
Contributor

@dennisameling I'm reverting the dependency update that introduced this issue in #377 so we can march onwards with this work

@shiftkey
Copy link
Contributor

@dennisameling #377 has been merged, please rebase when you have time and let's see if we're good

@dennisameling
Copy link
Contributor Author

@shiftkey looks like we're good to go now! Would be great if we could release a new major version (beta) with N-API to see if it works as expected. I could e.g. try with GitHub Desktop and see if all tests are passing there

@shiftkey
Copy link
Contributor

@dennisameling that's the plan, cc @sergiou87 for 👀

@sergiou87
Copy link
Collaborator

@sergiou87
Copy link
Collaborator

@trulyronak it'd be great if you could give that a try too, since you had issues with the first build 😅 🤞

@shiftkey
Copy link
Contributor

Merging this in now, but I'd like to adapt this step to support setting the next dist-tag on publish when I push a pre-release tag, so I can freely publish builds using Actions and not have them bleed over to latest for users:

- run: npm publish --access public
name: Upload to NPM
env:
NODE_AUTH_TOKEN: ${{ secrets.NPM_AUTH_TOKEN }}

@shiftkey shiftkey merged commit d04d668 into atom:master Apr 13, 2021
@dennisameling dennisameling deleted the switch-to-napi branch April 15, 2021 19:05
@cb1kenobi
Copy link

Thank you guys so much!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider only offering prebuilds for N-API v3
4 participants