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

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

Merged
merged 11 commits into from
Apr 5, 2024

Conversation

matthewkeil
Copy link
Member

@matthewkeil matthewkeil commented Apr 4, 2024

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

  • Move rebuild/src to src
  • Move rebuild/lib to lib
  • Move rebuild/tools to scripts
  • Move/Merge rebuild folder config files and configurations to root files
  • Delete now-unsued rebuild folder config files
  • Fix import paths from rebuild/lib to lib
  • Move binding.gyp from rebuild to root and update paths so build works
  • Move build, clean, lint scripts from rebuid/package.json to root file and update a couple dependency versions
  • Run lint and fix a couple of errors that it found

How to Test

git submodule update --init --recursive
yarn
yarn test
yarn build:fuzz
yarn test:fuzz

@matthewkeil matthewkeil requested a review from a team as a code owner April 4, 2024 14:44
@jeluard
Copy link
Contributor

jeluard commented Apr 4, 2024

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.

@matthewkeil
Copy link
Member Author

matthewkeil commented Apr 4, 2024

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

jeluard commented Apr 4, 2024

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

@matthewkeil
Copy link
Member Author

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.

@matthewkeil matthewkeil changed the base branch from mkeil/migrate-rebuild-delete-swig-code to master April 5, 2024 03:47
Copy link
Contributor

@twoeths twoeths left a comment

Choose a reason for hiding this comment

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

the steps work in my local env

@matthewkeil matthewkeil merged commit 0890ba5 into master Apr 5, 2024
1 check passed
@jeluard
Copy link
Contributor

jeluard commented Apr 5, 2024

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

nflaig commented Apr 5, 2024

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
Copy link
Member Author

matthewkeil commented Apr 5, 2024

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

@matthewkeil matthewkeil deleted the mkeil/migrate-rebuild-move-napi-src-to-root branch April 22, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants