-
Notifications
You must be signed in to change notification settings - Fork 13
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
chore: add darwin/arm64 prebuild support #113
Conversation
os: [ubuntu-20.04, macos-latest, windows-latest] | ||
node: [14, 16, 17, 18, 20] | ||
os: [ubuntu-20.04, macos-12, macos-14, windows-latest] | ||
node: [16, 17, 18, 20] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node 14
is not available on this new image. As it's pretty old now (node 20
is the LTS), I removed it.
macos-12
is kept to allow for x64
architecture prebuild (macos-latest
will move to arm64
in the future, so set the version explicitly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, just noticed that the specs are failing for some reason…. hmm
They now pass (after restarting them). I saw them passing before too.
I am not clear how significant this is. Is it only on this specific platform? Or just a flaky test that also happen on other platforms? It has to be noted that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They now pass (after restarting them). I saw them passing before too. For reference the error was:
1) altair/eth_aggregate_pubkeys eth_aggregate_pubkeys_x40_pubkey: AssertionError: expected '0xc00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000' to deeply equal null at Context.<anonymous> (test/spec/index.test.ts:85:54) at processImmediate (node:internal/timers:478:21)
I am not clear how significant this is. Is it only on this specific platform? Or just a flaky test that also happen on other platforms?
It has to be noted that
darwin/arm64
users compiles this exact version (potential bu included) on their machine transparently when installing this lib (tests are not run then). So if there's an issue, the newly addedprebuild
merely surface it.
That is SUPER weird.... That seems VERY bad to have a spec fail randomly... sigh... there was nothing you did to make that though with this PR so gonna approve this but make a note of it to make sure that case is covered in the rebuild. I have addressed it in some versions but noticed it did not trigger sometimes without the added code for that condition and now know why. double hmmm....
I am not sure of how we cut a release for this repo. Have only been working on the rebuild stuff. Do you want to sort through how to get this published after you merge @jeluard ? I dont think we need to actually publish a new version but am not sure. You can find the answer in the scripts dir where the binaries get downloaded. |
A new macos runner has been released. This image runs on M1 architecture, allowing to create
prebuilds
fordarwin/arm64
.