Skip to content
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

Introduce prebuildify #2368

Merged
merged 3 commits into from
Dec 27, 2021
Merged

Introduce prebuildify #2368

merged 3 commits into from
Dec 27, 2021

Conversation

thegecko
Copy link
Contributor

Building on the great work porting to N-API, this PR introduces prebuildify along with CI steps to create prebuilt binaries for:

  • Darwin (Intel and Arm 64)
  • Win32 (x86 and x64)
  • Linux x64

A sample run can be seen here:

https://github.com/thegecko/node-serialport/actions/runs/1573801837

The release check looking for refs/tags/@serialport/bindings@ should pick up the artifacts created and add them to the release.

The prepublishOnly script should download the prebuilds for distribution prior to publishing.

Note: I've also restricted the Node engine versions to only support NAPI version 6 due to v6 API calls being used

@GazHank
Copy link
Contributor

GazHank commented Dec 13, 2021

This looks really good.

It would be nice to see if we could add a Musl option into the Linux build too, I think there are a few alpine users who would appreciate that :-)

@thegecko
Copy link
Contributor Author

It would be nice to see if we could add a Musl option into the Linux build too, I think there are a few alpine users who would appreciate that :-)

This should be possible using prebuildify-cross, see:

https://github.com/node-usb/node-usb/blob/master/package.json#L54

However, my experiments threw up errors:

@serialport/bindings: IOError: [Errno 2] No such file or directory: './build/../../../input/node_modules/node-addon-api/nothing.target.mk'

see: https://github.com/thegecko/node-serialport/runs/4508679497?check_suite_focus=true

This would require further investigation, perhaps as a follow-up PR?

@robertsLando
Copy link

@thegecko
Copy link
Contributor Author

I suggest to use the approach used by leveldown

This is what I did for node-usb but in addition added Linux x86 support and used custom containers for required packages (libudev-dev).

@robertsLando
Copy link

@thegecko Ok so what about using same approach here?

@thegecko
Copy link
Contributor Author

@thegecko Ok so what about using same approach here?

I did, see above for the error and suggestion this is investigated and brought in a separate PR to unblock progress

@thegecko
Copy link
Contributor Author

Apologies if my comments above didn't make sense.

Using prebuildify-cross generates the following errors on Linux:

@serialport/bindings: IOError: [Errno 2] No such file or directory: './build/../../../input/node_modules/node-addon-api/nothing.target.mk'

Any pointers as to why this is happening would help with a fix as I don't know enough about node-serialport (e.g. any missing packages on older linux distros).

When fixed, this will produce a wider array of linux-based binaries.

Until then, this PR should bring parity with the existing releases (64 bit Linux/Windows/MacOS) which could optionally be merged. A local build would still be triggered on installation for other architectures.

@thegecko
Copy link
Contributor Author

Success!

I found the issue with using prebuildify-cross and this PR now successfully creates prebuilt binaries for the following architectures:

  • android-arm
  • android-arm64
  • darwin-x64+arm64 (inc. M1)
  • linux-arm (e.g. RPi)
  • linux-arm64 (e.g. RPi 64)
  • linux-x64 (musl and glibc)
  • win32-ia32
  • win32-x64

Sample CI run here:

https://github.com/thegecko/node-serialport/actions/runs/1598927595

@robertsLando
Copy link

Apologies if my comments above didn't make sense.

No worries I missed some things, glad you fixed that BTW, well done! Hope this gets merged soon

cc @GazHank @reconbot

@reconbot
Copy link
Member

This is fantastic work

A few thoughts;

  • We currently support LTS nodes. Which includes Node 12. Node 12 only added N-API version 6 to v12.17.0 and onword. I'm ok with this but we should update our documentation to node this fact. Making it clear we only support the latest version of each LTS version. (I've just merged a PR to drop node 10 with v10 of serialport, but it did support v6 with v10.20.0.)
  • node-gyp-build doesn't download the builds whereas node prebuild-install did. So uploading them to the release doesn't help anyone install the package without building locally unless they figure out how to go about it.
  • One of the holdups on publishing via github actions and including the prebuilt binaries in the npm package is the mono repo. Lerna just makes releasing everything on the server a lot harder. I am pro moving bindings to it's own repo and setting up semantic-release if that has the support of the people here I'll fork this repo and go do that. I think the benefits outweigh the costs. (but maybe it's worth going through them.)

@thegecko
Copy link
Contributor Author

We currently support LTS nodes. Which includes Node 12. Node 12 only added N-API version 6 to v12.17.0 and onword. I'm ok with this but we should update our documentation to node this fact. Making it clear we only support the latest version of each LTS version. (I've just merged a PR to drop node 10 with v10 of serialport, but it did support v6 with v10.20.0.)

There is an engine restriction in the bindings package in this PR, but perhaps it should be included in all packages?
As a side-note, Node v14 was the earliest version I could get the prebuilds working, however it should still generate NAPI v6 compatible binaries.

node-gyp-build doesn't download the builds whereas node prebuild-install did. So uploading them to the release doesn't help anyone install the package without building locally unless they figure out how to go about it.

While prebuildify doesn't download from GitHub (it includes the binaries in the release), I can see two benefits for keeping the binaries in the release:

  • It's used as a common area to collect all artifacts from each prebuild. The prebuildify-ci package expects to download all the binaries from the release (using the current package version) prior to packaging for release.
  • It's a history of released binaries which may be easier to access than unpacking them from npm releases

One of the holdups on publishing via github actions and including the prebuilt binaries in the npm package is the mono repo. Lerna just makes releasing everything on the server a lot harder. I am pro moving bindings to it's own repo and setting up semantic-release if that has the support of the people here I'll fork this repo and go do that. I think the benefits outweigh the costs. (but maybe it's worth going through them.)

I'm not aware of your release process, but if it's a manual step, (e.g. using lerna to run npm publish across all packages locally), then in theory it should still work using the additional prepublishOnly script to download the binaries from the release. Personally I avoid automatic releases from CI.
I'll let others discuss additional benefits/drawbacks of splitting from a monorepo, however.

thegecko and others added 3 commits December 25, 2021 12:19
* Add prebuildify-cross cwd

* Tidy syntax

* More tidy
@reconbot
Copy link
Member

Ahh that makes sense.

@reconbot reconbot merged commit d50673f into serialport:master Dec 27, 2021
@reconbot
Copy link
Member

Lets do all the things;

  • I'm going to fork this into a new bindings-cpp repo and @serialport/bindings-cpp package
  • I do want to have a a ci published approach, we have a few maintainers and githubs access control is much better than npms.
  • I cannot wait to try out the cross builds! 🕺 💃

@thegecko thegecko mentioned this pull request Dec 29, 2021
@reconbot
Copy link
Member

reconbot commented Jan 2, 2022

I've got a broken build in a new repo https://github.com/serialport/bindings-cpp/actions/runs/1647155559 python 3 doesn't exist on all the docker images and we seem to need it for some reason

@reconbot
Copy link
Member

reconbot commented Jan 2, 2022

I opened an issue for it serialport/bindings-cpp#1

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

Successfully merging this pull request may close these issues.

4 participants