magiclen / node-crc

To compute CRC values by providing the length of bits, expression, reflection, an initial value and a final xor value. It has many built-in CRC functions.
MIT License
31 stars 9 forks source link

Prebuilt binaries to avoid requiring Rust tooling everywhere #9

Open wraithan opened 3 years ago

wraithan commented 3 years ago

Given the change over to using rust, and the lack of rust tooling in most CI infrastructure by default, it might be nice to provide prebuilt binaries.

There are a few solutions to this, https://github.com/prebuild/prebuildify appears to be one of the most modern solutions to this problem, it includes the prebuilt binaries in the package so no extra downloads needed. https://github.com/prebuild/prebuild-install is an older solution but I've seen it used pretty successfully, artifacts are separate and you use github releases to host them. Since you've already moved to github actions, it should make the configuration pretty simple, if the artifacts end up hosted in the github releases. I've not looked into if there are any special considerations for doing this with Neon.

If this is interesting to you, I can spend some time trying to make this work and provide a PR within the next 2-3 weeks.

magiclen commented 3 years ago

I think this a great idea. Any PR is welcome!

rafaelmaeuer commented 3 years ago

Using prebuild binaries would probably solve #8 too.

rafaelmaeuer commented 3 years ago

@wraithan how are your plans regarding this? If you don't plan a PR soon, I would give it a try...

rafaelmaeuer commented 3 years ago

@wraithan are you still there? Any comment on this?

wraithan commented 3 years ago

Sorry, I've been busy at work and these notifications went to my personal email (since they weren't on work repo) and I missed them. We lowered the priority of moving to node-crc@2 and have been digging in to a few other things lately. I don't have any code to share, I only got as far as playing with prebuildify to learn a bit before I tried to get it working on this codebase.

rafaelmaeuer commented 3 years ago

I also gave prebuildify a short try today, but getting this error when running prebuildify --all --strip:

node:events:371
      throw er; // Unhandled 'error' event
      ^

Error: spawn node-gyp ENOENT
    at Process.ChildProcess._handle.onexit (node:internal/child_process:282:19)
    at onErrorNT (node:internal/child_process:480:16)
    at processTicksAndRejections (node:internal/process/task_queues:83:21)
Emitted 'error' event on ChildProcess instance at:
    at Process.ChildProcess._handle.onexit (node:internal/child_process:288:12)
    at onErrorNT (node:internal/child_process:480:16)
    at processTicksAndRejections (node:internal/process/task_queues:83:21) {
  errno: -2,
  code: 'ENOENT',
  syscall: 'spawn node-gyp',
  path: 'node-gyp',
  spawnargs: [
    'rebuild',
    '--target=5.0.0',
    '--devdir=/var/folders/79/20rjf5910x9c32y41kvvtcv80000gn/T/prebuildify/node',
    '--target_arch=x64',
    '--release'
  ]
}
rafaelmaeuer commented 2 years ago

So I finally found the time to search for a solution, first I was playing around with napi-rs. It was possible to produce a prebuild binary which can be used with require (see rafaelmaeuer/node-crc/tree/dev/napi).

Then I found out, that cargo already produces the prebuild-binary index.node which can be used as main file in package.json. Mocca-Tests are still passing and I tested it in docker without the need of installing the rust toolchain.

I have made the following PR, please review and test it: https://github.com/magiclen/node-crc/pull/12

rafaelmaeuer commented 2 years ago

Just found this error in github-actions:

Error: /home/runner/work/***/***/node_modules/node-crc/index.node: invalid ELF header

Seems like pre-compiling need to handle all the possible OS (win/mac/linux) and platform types (x86/x64/arm64/aarch64).

rafaelmaeuer commented 2 years ago

After learning about the necessity of cross-platform binaries (invalid ELF header in Github actions) I ended up writing a build-script based on cross-rs and rustup/cross-compilation which can produce binaries for following platforms:

I slightly modified the lib to load the correct binary on each platform, I updated #12 accordingly.

rafaelmaeuer commented 2 years ago

@magiclen would you mind reviewing or commenting on it?

rafaelmaeuer commented 2 years ago

Leaving some links here for documentation:

magiclen commented 1 year ago

@rafaelmaeuer Thank you for everything you have done. My idea is to separate the precompiled .node files and automatically download them from the internet during the npm install node-crc stage. If the download fails, then local compilation will be performed.

I guess we need a script to build the .node files for different architectures and upload them to github releases. And when running npm install node-crc, we need a script to detect the environment and download the corresponding .node file or run the compilation.