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

Rebuild of Nodejs Bindings #160

Closed
wants to merge 17 commits into from

Conversation

matthewkeil
Copy link
Member

@matthewkeil matthewkeil commented Feb 27, 2023

Follow-up to PR #147 and related to Issue #153, Issue #149 and Issue #118.

Inclusions

Notes:

  • I am adding some additional unit tests for the bindings today but those should not affect the code as I was running the code on watch while I worked and saw the errors being thrown

@matthewkeil
Copy link
Member Author

I just saw the workflow fail and am updating that now. Will push a commit in two shakes. Just want to test it first

@matthewkeil
Copy link
Member Author

matthewkeil commented Feb 28, 2023

I added the -fPIC and the library builds fine using npm on ubuntu. But now there is an error when the tests run symbol lookup error: c-kzg-4844/bindings/node.js/build/Release/kzg_bindings.node: undefined symbol: blst_p1_uncompress. I will continue to debug in the morning

The CI using ubuntu was working with the make commands and only broke after the code changes and switching to npm command. I can try to move back to the make command for this PR and can perhaps do the npm fix on a subsequent PR to not hold things up.

In the mean time I added a matrix strategy to the workflow to test the bindings against mac/windows/linux for node versions 12, 14, 16, 17, 18 so we can see build/test results against a few different environments until things are stable

@matthewkeil matthewkeil changed the title Polish Nodejs Bindings Rebuild of Nodejs Bindings Feb 28, 2023
@dapplion
Copy link

To ease reviews I would close this PR and split into progressive chunks:

non-core logic

  • Update CI workflow(s)
  • Add eslint
  • Update misc config files
  • Expand tests against current logic in main

core logic

@matthewkeil
Copy link
Member Author

Closing this and will reopen as several separate PR's

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.

2 participants