Remove node-pre-gyp, use prebuildify

[thom-nic]

Added Dockerfiles for cross-platform build on linux-x64, arm64 and arm32. Note that on ARM platforms unit tests presently fail :( Tests pass on all architectures (x64,arm,arm64/glibc and musl) and nodejs versions (10,12,14,16 x64/glibc)

Note if you want to run/debug unit tests in an interactive environment it's possible to docker run -it blah/bcryptjs-linux-armhf-builder which will start a bash session inside the container.

Edit: I should mention, the tests have probably always failed on ARM in a way that has nothing to do with prebuildify or docker/ARM emulation. I did try to checkout the master branch on an arm device and run the tests, and IIRC they failed the same way. But the good news is, now it's easy to do full cross-platform build and test in CI using these arm32 and arm64 docker images. This is all fixed.

Edit 2: worth noting, the package/ release process needs to be updated a bit. Unlike node-pre-gyp which would download a platform specific prebuilt archive, prebuildify publishes all prebuilt binaries to NPM (or github nodejs registry). So while CI could paralellize builds for different platforms, each prebuild artifact needs to be collected into a single unified folder before publishing to NPM. i.e. when you do an npm publish your folder structure should look like this:

[project root]
 |
 + prebuilds/
    + darwin-x64/
    + linux-arm64/
    + linux-arm/
    + linux-x64/

So I'm not yet clear on what an automated CI publish process would look like. Basically you need a sort of "reducer" step that runs after all of the parallel platform-specific prebuilds have completed. I've been playing with github actions and think there's a workable solution there. See comment below I have github actions working now.

Fixes #858
Fixes #665

[thom-nic]

Hey bcryptjs maintainers!

I've spent more time on this recently and believe I have a workable solution with github actions (I have a fork of node-sqlite3 ported to prebuildify + gh actions.) It will parallelize tests on multiple nodejs versions and also parallelizes the native build for different platforms (presently linux-{x64,arm,arm64}. May be possible to build for Alpine/musl too. So this now has potential to resolve #858

That said, I still get inconsistent test results between platforms. At the moment, this branch has test failures on Ubuntu 20.04 on x86 but the build passes on my macbook. 🤷 I forgot I had tried to change nodeunit for nodeunit-tape-compat which has a builtin hard-coded timeout which explains the inconsistent test failures.

Aside: While nodeunit should probably be replaced, it still works and changing the test script to npx nodeunit removes it from the dependency tree and avoids the inevitable npm audit problems. IMO tape/ tap are its own can of worms because they tap has a ridiculous dependency tree and doesn't appear to be easily avoided by npx since tap is designed to be explicitly require()'d from test files 😑

[thom-nic]

Ok these appear to be in good shape now. I added nodejs 10 and alpine builders to the build matrix as well. You can see the actions run against a PR on my fork here: https://github.com/thom-nic/node.bcrypt.js/pull/1/checks

[cokia]

I think this is an important issue too.

Due to the spread of M1 Macs, many developers have begun to use ARM, which is an increasingly high demand problem.

[cchepeau-mwb]

Hey there, any idea on when this could get merged?

[recrsn]

Let's merge it.

Can you rebase it again with some changes in main?

[recrsn]

The latest release got rid of nodeunit and tap

[mrinc]

@recrsn @thom-nic just bumping this up

[thom-nic]

I will rebase this week

[mpauly-exnaton]

@thom-nic I made an attempt at rebasing over here.
Things to watch out for:

• I removed github actions as introduced by @recrsn and kept the one introduced by @thom-nic
• In line with upstream, I dropped support for old node versions and added Node 18 to the build matrix
• I discarded the changes you made to the tests as the tests are now promise-based. I increased the timeouts for the tests. Without this increase the tests were failing.

With these changes the build is passing for me, as you can see here.
I hope this helps to identify the pain-points in rebasing. If you want I can also open a PR with these changes against master.

I should also notice that I am by no means an expert in building native node extensions, and just followed your changes. So please have a careful look/feel free to discard everything I did.

[mfernandes-alcumus]

when is this getting merged?

[JasonMan34]

What exactly is the state of this PR? @thom-nic @recrsn
If it's a problem of rebasing, is @mpauly-exnaton 's fork any help?

I wouldn't mind forking myself and applying the changes from this PR, or do anything else I can to help promote #665

[thom-nic]

This PR is two years old, seems like we're all on our own :( Good luck y'all.

[thom-nic]

Apparently node-gyp just published v10 over the weekend and dropped support for nodejs 14. I pinned the installed version in the dockerfiles.

FWIW the nodejs version used when building is independent of the runtime nodejs version support. I verified this by building running the build on node 18 then changing to node v14 and running the test suite.

[thom-nic]

@recrsn do you plan to drop the appveyor and travis CI?

[thom-nic]

Ok only using two dockerfiles with --platform to build for x86-64 + arm32 + arm64

[recrsn]

@recrsn do you plan to drop the appveyor and travis CI?

Yes

[thom-nic]

I think I mostly have CI running correctly. I have the workflows running on another branch in a private repo and I believe it's working or very close to. For some reason the arm64 workflow is taking forever at the moment (~20 minutes+) before it fails because the repetition tests timeout. It was much faster before so I'm not sure what changed or if there's some hiccup in github's infra at the moment.

Worth pointing out, the original ci.yml only does tests across node versions but does not do xplatform tests (but those are run as part of the build-pack-publish script.)

[recrsn]

Github is releasing arm64 runners soon, so if the tests are taking forever, we can hold it off for a while.

Meanwhile, I already use a Mac and two RPIs to test releases.

[thom-nic]

Ok I have the build and test running on all platforms, without hardcoding an exceptionally long timeout - only during the workflow. Given that it all passes now, it would be really nice to merge this now and if arm64 runners can be utilized in the future, change it then.

I don't really want to have to address this issue again in the future.

[recrsn]

This looks good now

[AaronMoat]

@recrsn would it be possible to cut a release for this, please? It happens to also remove a vulnerable dependency, which I'm sure will start to nag people's CI systems :)

https://security.snyk.io/vuln/SNYK-JS-INFLIGHT-6095116

[JasonMan34]

@recrsn would it be possible to cut a release for this, please? It happens to also remove a vulnerable dependency, which I'm sure will start to nag people's CI systems :)

https://security.snyk.io/vuln/SNYK-JS-INFLIGHT-6095116

@recrsn Any update?