kelektiv / node.bcrypt.js

bcrypt for NodeJs
MIT License
7.44k stars 512 forks source link

Remove node-pre-gyp, use prebuildify #890

Closed thom-nic closed 10 months ago

thom-nic commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

recrsn commented 1 year ago

Let's merge it.

Can you rebase it again with some changes in main?

recrsn commented 1 year ago

The latest release got rid of nodeunit and tap

mrinc commented 1 year ago

@recrsn @thom-nic just bumping this up

thom-nic commented 1 year ago

I will rebase this week

mpauly-exnaton commented 1 year ago

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

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 commented 1 year ago

when is this getting merged?

JasonMan34 commented 10 months ago

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 commented 10 months ago

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

thom-nic commented 10 months ago

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.

recrsn commented 10 months ago

@thom-nic Thats the advantage of napi, ABI stability. Regardless, we build in the earliest supported nodejs version to prevent any lurking bugs, if any

thom-nic commented 10 months ago

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

thom-nic commented 10 months ago

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

recrsn commented 10 months ago

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

Yes

thom-nic commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.

Screenshot 2023-11-02 at 9 35 40 AM
AaronMoat commented 8 months ago

@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 commented 7 months ago

@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?