ChainSafe / blst-ts

Typescript wrapper for https://github.com/supranational/blst native bindings, a highly performant BLS12-381 signature library
Other
18 stars 13 forks source link

feat: move napi code, utils and config files out of rebuild to repo root #130

Closed matthewkeil closed 6 months ago

matthewkeil commented 6 months ago

~NOTE: builds on PR #129. That must get merged first. May need to update merge target if github doesnt automatically update after merge~ Updated.

Description

How to Test

git submodule update --init --recursive
yarn
yarn test
yarn build:fuzz
yarn test:fuzz
jeluard commented 6 months ago

Would it be worth considering https://github.com/ChainSafe/blst-ts/issues/117 ? Ideally, the main package.json would be dependency free, and everything related to building would be in a separate package.

This would probably also help with integrating wasm, if we ever go that road.

matthewkeil commented 6 months ago

Would it be worth considering #117 ? Ideally, the main package.json would be dependency free, and everything related to building would be in a separate package.

This would probably also help with integrating wasm, if we ever go that road.

@jeluard I did update to work toward that and will meet that as an end goal. There is another PR upcoming tomorrow, once this gets merged, to clean up the package.json and CI workflow. They are both in "roughly" final form in the moster PR #125 but need to do a bit of touch-up tomorrow.

You can see them here: https://github.com/ChainSafe/blst-ts/pull/125/files

The build/install script is also in there so you can check it out

jeluard commented 6 months ago

Awesome! I still see things like node-gyp as dependency in the main package.json. Hopefully we can have it defined somewhere else :)

matthewkeil commented 6 months ago

Awesome! I still see things like node-gyp as dependency in the main package.json. Hopefully we can have it defined somewhere else :)

Cannot. It's a hard dependency for the install command on non-prebuilt platforms. It must be there and needs to be a dep not a devDep. Same with node-addon-api.

jeluard commented 6 months ago

Awesome! I still see things like node-gyp as dependency in the main package.json. Hopefully we can have it defined somewhere else :)

Cannot. It's a hard dependency for the install command on non-prebuilt platforms. It must be there and needs to be a dep not a devDep. Same with node-addon-api.

The dependency is to have an install script that generates the right binary at the right place? Then I would expect that this install script can actually call a different and independent script in a different package, with a separate package.json? I might be missing something here.

nflaig commented 6 months ago

Awesome! I still see things like node-gyp as dependency in the main package.json. Hopefully we can have it defined somewhere else :)

Cannot. It's a hard dependency for the install command on non-prebuilt platforms. It must be there and needs to be a dep not a devDep. Same with node-addon-api.

The dependency is to have an install script that generates the right binary at the right place? Then I would expect that this install script can actually call a different and independent script in a different package, with a separate package.json? I might be missing something here.

Looking at https://www.npmjs.com/package/bcrypt?activeTab=dependencies it seems like it is required, that's the most used native module I know about. It uses https://www.npmjs.com/package/@mapbox/node-pre-gyp, instead of node-gyp

matthewkeil commented 6 months ago

@jeluard its a requirement. The issue is for unsupported systems building is necessary.

We are bundling pre-built binaries for the most common systems (osx-x64, osx-arm, linux-x64, linux-arm, windows) and only on the current version of node (20) in the npm tarball. This is in an attempt to keep the tarball smallish (each .node file is about 3mb x 5 architectures is roughly 12mb of bloat because only 1 will be applicable for an architecture/node combo).

In the release for each version we will host prebuilt binaries for those same system architectures but for other versions of node (18 and 21) which is an extra 10 binaries.

The gyp process is for all the other cases that are not represented by the above. For instance some raspberry pi systems you risc or x32 architectures. Both are supported by node and lodestar can run on them. This is also a package used by the community and there are probably other use cases we are not even considering. For those situations the bindings need to be built.

That is where node-gyp comes in. For combinations of system architectures other than the 5 listed above, and for alternate versions of node the bindings need to be compiled at install time via npm. This is why the package.json has a gypfile key because it instructs npm to call node-gyp with the binding.gyp to build a suitable version of the code. Gyp is environment aware and will use the local tooling that is compatible with the environment to compile to a version of assembly code that will run on the given architecture.

BTW @nflaig i did look into using one of the industry solutions for this issue and researched several. node-pre-gyp is a package that does what our workflow does manually.

The issue with the existing packages is that they do something similar but none exactly fit our use case because our original system was manually built so i just went with what we already have and do. In particular node-pre-gyp stores the prebuilt binaries in AWS S3 and not in a github release which is more in keeping with the open source ideology and our current process. Prebuildify also is an option and I think it uses the github releases but there was something different that conflicted with our current system but its been a while and i dont remember off the top of my head what it was…