Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Prebuild binaries #136
base: master
Are you sure you want to change the base?
Prebuild binaries #136
Changes from all commits
a9b4091
16470ad
b06803a
6e745af
09c5b35
62945c4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/[email protected]
Node-pre-gyp itself is 0.6 MB (minifed)
https://bundlephobia.com/[email protected]
Prebuild-install is 100 KB (minified)
https://bundlephobia.com/[email protected]
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 eachFor 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.