Prebuild binaries #136
Prebuild binaries #136
Conversation
aminya
force-pushed
the
prebuild
branch
2 times, most recently
from
9b86af1
to
19e0e6d
Compare
|
|
||
| - Change the version in `package.json`, make a new git tag, and push it to GitHub. | ||
| - Wait until the GitHub Actions on the master branch pass. | ||
| - The artifacts of the latest GitHub Action run should be downloaded from the actions tab of the GitHub repository |
There was a problem hiding this comment.
This seems wrong. We should not be downloading the artifacts of the latest GitHub action. We should be pulling the artifacts from the action that was built via the release tag that we pushed.
I would also prefer leveraging a different release system (I don't want to push artifacts for linux, osx, or 32 bit windows to the npm package). I am not familiar enough with prebuild here, but I vastly prefer node-pre-gyp, which pushes artifacts to s3 / github releases and downloads only the artifacts necessary for the platform you're using. For instance, a Windows platform should never download linux/mac os binaries.
There was a problem hiding this comment.
Also, the downloading of artifacts is at install time with node-pre-gyp, rather than at publish time. I think that's where the difference is actually coming from.
There was a problem hiding this comment.
I don't recommend using anything other than prebuildify + node-gyp-build. I have tried all the possible solutions, and the most performant and reliable one is this configuration.
If you read the readme for prebuildify, the contributors of prebuild also recommend this over all the other solutions:
https://github.com/prebuild/prebuildify#prebuildify
With prebuildify, all prebuilt binaries are shipped inside the package that is published to npm, which means there's no need for a separate download step like you find in prebuild. The irony of this approach is that it is faster to download all prebuilt binaries for every platform when they are bundled than it is to download a single prebuilt binary as an install script.
Always use prebuildify --@mafintosh
There was a problem hiding this comment.
I understand that the prebuildify team suggests using prebuildify, but I've been involved with many packages that do not leverage prebuildify and they work wonders. I also like being able to decouple the prebuild process, because this PR lacks the ability to precompile binaries for Electron - something that solutions like node-pre-gyp allow you to do easily. From my perspective a solution like nodegit leverages is much preferable.
There was a problem hiding this comment.
That's not quite accurate. The Electron artifacts are already downloaded. See this:
https://github.com/aminya/nsfw/suites/2602423618/artifacts/56919535
It's up to you, I have been using this configuration inside ZeroMQ, Atom, Nteract, etc.
There was a problem hiding this comment.
it would be unreliable (downloading issues, rate limiting, etc) and the installation time would be more.
Do you have a good write up from anyone illustrating what exactly is meant here by "unreliable" or "slow"? I think these words by themselves are pretty vague. How often are we seeing install failures due to unreliability in the community? How much slower is our install? If NSFW was installed as a module in an application like VSCode, Atom, or GitKraken - is the increase in time to install nsfw the bottleneck that those teams will be looking to solve in their bootstrap procedures?
I would definitely say that without clear metrics to demonstrate how much more reliable/fast prebuildify is over node-pre-gyp or similar, it's not clear to me that we should leverage this process at the expense of introducing additional work for people who leverage this package. The way I am thinking about it is that VSCode, GitKraken, and Atom all ship a copy of NSFW, do we want them to be responsible for cleaning up our unused files, or do we want to make it our responsibility to ship a leaner package?
I am open to learning about these concerns, it could very well be a valuable thing to take back into NodeGit. I think I just need a little help answering these questions to see where my intuition is failing, and it's also possible that my intuition is on point, too.
I hope I'm not being rude. I really do appreciate the time and effort you put in here. I want to be clear where I'm failing to get behind prebuildify. I definitely need convincing to stay on the current course.
There was a problem hiding this comment.
Especially because I know this is a highly requested feature. So it would be awesome to get this done with help from the community.
There was a problem hiding this comment.
Well, I am not a fan of premature optimizations either. The installation size of NSFW is currently 18 MB. You could make that only ~2MB if you merge this PR. This is an 9X improvement. Nodegit that you mentioned is over 100MB. You could bring that to ~4MB if you provided prebuilds.
Node-gyp-build (this PR) is only 2 KB
https://bundlephobia.com/result?p=node-gyp-build@4.2.3
Node-pre-gyp itself is 0.6 MB (minifed)
https://bundlephobia.com/result?p=node-pre-gyp@0.17.0
Prebuild-install is 100 KB (minified)
https://bundlephobia.com/result?p=prebuild-install@6.1.2
Also, as far as I know, prebuildify is the only solution that is able to provide prebuilds for Napi + Electron without requiring any rebuilding afterward. This is huge for me and the users of Atom, Ntract, ZeroMQ, etc.
I will be fine if you want to switch to prebuild-install or node-pre-gyp "later" provided that you can include the Electron prebuilds somehow and prove that the final size is less. But I will not personally spend time investing in those and their huge number of issues. I am instead making cmake-ts to replace all of them based on the experience I've got from trying each.
Here are some of the issues for those:
134 issues node-pre-gyp
https://github.com/mapbox/node-pre-gyp/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc
Some prebuild-install issues
prebuild/prebuild-install#145
https://github.com/prebuild/prebuild-install/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc
The prebuild organization itself recommends prebuildify over prebuild/prebuild-install.
prebuild/prebuild-install#150
And here is the only prebuildify issue (which is actually a feature that I have fixed in cmake-ts).
prebuild/prebuildify#49
There was a problem hiding this comment.
Your numbers are straight up wrong.
For one, you've dropped the pdb files which are a requirement to send out with any prebuilts (necessary to setup crash reporting in applications). When including the pdb file for every single .node on windows you're going to see roughly a 8 meg increase per .pdb file, which there will be a 1-to-1 relationship with the windows .node files.
Your absolute baseline if you're shipping the correct prebuilts is:
1 napi build per OS at roughly .5 megs each
an additional napi build per OS for electron base each also roughly .5 megs each
pdb files for all 4 windows builds 8 megs each
For a grand total of 36 megs, twice the install size you're listing for nsfw. No thank you.
If we keep rolling out this package to install bases where we're dropping necessary files for apps like the one I ship, we're creating additional work for me to have to recompile these packages for pdbs so I can symbolize crash reports from customer stacktraces. While I'm talking in the first person here, I am not alone in the Electron community when it comes to these issues.
There was a problem hiding this comment.
Also, neat side-effect of this I discovered in nodegit we were missing a clean operation on our electron prebuilder that we're running for the node releases.
There was a problem hiding this comment.
I think, generally, this is a great solution and job well done - however, I am not keen on leveraging prebuild if artifacts are downloaded at publish time rather than install. Can this be configured? If it can, let's take that approach instead. If not, perhaps let's leverage node-pre-gyp or opt for another solution that has artifact downloading occuring at install time, rather than publish.
This adds prebuild support for NSFW. The binaries are built inside GitHub Actions and are uploaded as artifacts.
See this run as an example:
https://github.com/aminya/nsfw/actions/runs/792375126
And the artifacts uploaded:
https://github.com/aminya/nsfw/suites/2602423618/artifacts/56919535
Node-gyp-build checks if prebuilds are available for that arch and os, and if not available (on non-common configurations), it builds the binaries from the source.
Fixes #123