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

chore: add darwin/arm64 prebuild support #113

Merged
merged 1 commit into from
Feb 7, 2024
Merged

chore: add darwin/arm64 prebuild support #113

merged 1 commit into from
Feb 7, 2024

Conversation

jeluard
Copy link
Contributor

@jeluard jeluard commented Feb 2, 2024

A new macos runner has been released. This image runs on M1 architecture, allowing to create prebuilds for darwin/arm64.

@jeluard jeluard requested a review from a team as a code owner February 2, 2024 16:21
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]
Copy link
Contributor Author

@jeluard jeluard Feb 2, 2024

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)

Copy link
Member

@matthewkeil matthewkeil left a comment

Choose a reason for hiding this comment

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

LGTM!! 🚀

Copy link
Member

@matthewkeil matthewkeil left a 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

@jeluard
Copy link
Contributor Author

jeluard commented Feb 3, 2024

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 added prebuild merely surface it.

Copy link
Member

@matthewkeil matthewkeil left a 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 added prebuild 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....

@matthewkeil
Copy link
Member

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.

@wemeetagain wemeetagain merged commit cef9a8b into ChainSafe:master Feb 7, 2024
24 checks passed
@jeluard jeluard deleted the jeluard/osx-prebuild branch February 9, 2024 08:37
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.

3 participants