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

Prebuild binaries #136

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,59 @@ on:
name: Run Tests

jobs:
prebuild:
name: Prebuild
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os:
- ubuntu-20.04
- macos-latest
- windows-latest
node_version:
- 14
node_arch:
- x64
include:
- os: windows-latest
node_version: 14
node_arch: x86
steps:
- uses: actions/checkout@v2

- name: Install Node
uses: actions/setup-node@v2
with:
node-version: ${{ matrix.node_version }}
architecture: ${{ matrix.node_arch }}

- name: Install Dependencies and Build
run: yarn

- name: Prebuild
run: yarn run prebuild
env:
ARCH: ${{ matrix.node_arch }}

- name: Upload artifacts
uses: actions/upload-artifact@v2
with:
path: ./prebuilds

test:
name: Tests
needs: prebuild
strategy:
matrix:
node: [10, 12]
os: [windows-2016, ubuntu-16.04, ubuntu-18.04, macOS-latest]
node_arch:
- x64
include:
- os: windows-latest
node: 14
node_arch: x86
aminya marked this conversation as resolved.
Show resolved Hide resolved
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@master
Expand All @@ -23,6 +69,18 @@ jobs:
uses: actions/setup-node@master
with:
node-version: ${{ matrix.node }}
architecture: ${{ matrix.node_arch }}

- name: Download prebuilds
uses: actions/download-artifact@v2

- name: Install prebuilds
shell: bash
run: |
rm -rf build
mkdir prebuilds
mv artifact/* prebuilds/
ls prebuilds

- run: yarn

Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ coverage

# Compiled binary addons (http://nodejs.org/api/addons.html)
build/Release
prebuilds

# Dependency directory
# https://www.npmjs.org/doc/misc/npm-faq.html#should-i-check-my-node_modules-folder-into-git
Expand Down
23 changes: 23 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
## Releasing a new npm version
aminya marked this conversation as resolved.
Show resolved Hide resolved

- 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
Copy link
Contributor

@implausible implausible Apr 28, 2021

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.

Copy link
Contributor

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.

Copy link
Author

@aminya aminya Apr 28, 2021

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

Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@implausible implausible Apr 28, 2021

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.

Copy link
Author

@aminya aminya May 1, 2021

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.

image

image

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

Copy link
Contributor

@implausible implausible May 5, 2021

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 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.

Copy link
Contributor

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.

- The artifacts.zip should be extracted and placed under the `prebuilds` folder (replacing the old folder if it exists).

The `prebuilds` folder should look like the following.
```
repository-root
|-prebuilds
|_linux-x64
|...
|_darwin-x64
|...
|_win32-x64
|...
```

- Then:
```
npm publish
```
2 changes: 2 additions & 0 deletions binding.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// don't move this file. It should be next to package.json
module.exports = require('node-gyp-build')(__dirname);
39 changes: 39 additions & 0 deletions js/scripts/prebuild.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
const {spawnSync} = require('child_process');

main().catch(e => {
throw e;
});

async function main() {
console.log('Building distribution binary...'); // eslint-disable-line no-console

const prebuildArch = getNodearch(process.env.ARCH);

// napi is forward compatible so we only build for two targets (Node and Electron)
let prebuildScript = `prebuildify --napi --arch=${prebuildArch} -t 12.0.0 -t [email protected] --strip`;

if (process.platform == 'linux') {
prebuildScript = `${prebuildScript} --tag-libc`;
}

spawnSync(prebuildScript, {
shell: true,
stdio: 'inherit',
encoding: 'utf8',
});
}

/**
* @param {string | undefined} arch the given architecture
* @returns {string} the Node architecture to build for
*/
function getNodearch(arch) {
if (!arch) {
return process.arch;
}
if (arch === 'x86') {
return 'ia32';
} else {
return arch;
}
}
2 changes: 1 addition & 1 deletion js/src/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const { promises: fs } = require('fs');
const path = require('path');

const NSFW = require('../../build/Release/nsfw.node');
const NSFW = require('../../binding');

function NSFWFilePoller(watchPath, eventCallback, debounceMS) {
const { CREATED, DELETED, MODIFIED } = nsfw.actions;
Expand Down
13 changes: 9 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
"description": "A simple file watcher for Node",
"main": "js/src/index.js",
"scripts": {
"install": "node-gyp-build",
"lint": "eslint js/src js/spec",
"test": "yarn lint && node js/scripts/test.js"
"test": "yarn lint && node js/scripts/test.js",
"prebuild": "node ./js/scripts/prebuild.js"
},
"repository": {
"type": "git",
Expand All @@ -25,17 +27,20 @@
"js/src",
"src",
"includes",
"binding.gyp"
"binding.gyp",
"prebuilds"
],
"homepage": "https://github.com/axosoft/node-simple-file-watcher",
"dependencies": {
"node-addon-api": "*"
"node-addon-api": "*",
"node-gyp-build": "^4.2.3"
},
"devDependencies": {
"eslint": "^6.8.0",
"executive": "^1.6.3",
"fs-extra": "^7.0.0",
"mocha": "^7.1.1"
"mocha": "^7.1.1",
"prebuildify": "^4.1.2"
},
"keywords": [
"FileWatcher",
Expand Down