-
Notifications
You must be signed in to change notification settings - Fork 583
chore: MacOS compatibility lateral changes #17864
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
Conversation
Could you point to the changed async ssl package definition in this PR? |
"async_kernel.v0.14.0" | ||
"async_rpc_kernel.v0.14.0" | ||
"async_ssl.v0.14.0" | ||
"async_ssl.v0.14.0-o1labs" |
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.
should be here @glyh
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.
it was very strange, this file did not show up for me either? I had to refresh the page like twice
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.
I'm saying the source code of this new package
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.
ah I see, my fault
https://github.com/o1-labs/async_ssl#v0.14-o1labs
I also have an eternal pull request so we can see the diff clearly and what I and @Trivo25 changed to make it fall into line
o1-labs/async_ssl#1
!ci-build-me |
!ci-toolchain-me |
!ci-build-me |
!ci-toolchain-me |
!ci-build-me |
691ba09
to
a4d43e4
Compare
!ci-toolchain-me |
!ci-build-me |
How optimisation flags are handled should be removed from dune, and xtask should be used. See the directory xtask in proof-systems. |
I think we might want someone actually working on MacOS to confirm this is working. We don't have a CI workflow for MacOS unfortunately. |
Could we add one using GitHub Actions? |
Sure, I'd support that :) |
Looking at I'm going to leave it as is, @dannywillems, thoughts? |
Do I need a !ci-bypass-changelog here? |
I think we can bypass changelog for this, I don't have that permission unfortunately. |
!ci-bypass-changelog |
Yes, I think this is just pinning a package and cleaning the build log output, right? Nothing interesting for the changelog. |
!ci-build-me |
1c045c3
to
12e0884
Compare
Looks like we already support Removed the optimization flag rules |
I have an M1 that I haven't used for coding in a long while. So its env should still contain all of the mac-related errors it had earlier (if not more!). Thus, I will look into this and give feedback as part of the review loop. Is it OK if I do so on Monday? @bleepbloopsify @glyh |
moves `async_ssl` from public package (v0.14.0) to our own pin (v0.14.0-o1labs), since its not compatible with macos.
12e0884
to
993f2ec
Compare
!ci-build-me |
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.
IT WORKS ON M1 🤯
What else can I say: BRAVO 💃🏻
Could we consider add a github workflow so to ensure we don't accidentally break it in the future? Not something that have to be done in this PR, but very preferrable to have. |
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. I would be in favor of adding a GitHub actions workflow for macOS as part of a follow-up. This would ensure that we keep supporting macOS.
macos-13, macos-14, macos-15 and macos-latest are available platforms for workflows. A GitHub reusable action could be used to have a single workflow file and support all platforms at the same time.
I have a WIP macos build in o1js at the moment (on GitHub actions) Will probably port that over to mina once it's finished |
This is the current efforts at "native build workflow" on we're building without |
moves
async_ssl
from public package (v0.14.0) to our own pin (v0.14.0-o1labs), since its not compatible with macos.also, adds another case tosrc/lib/crypto/kimchi_bindings/stubs/dune
that removes optimization flags not supported onarm64
. (these flags are intel only, and only pollute the build log).Those changes were already done here: o1-labs/proof-systems#3248, we just have to wait for proof-systems to get bumped first
Explain your changes:
Explain how you tested your changes:
opam switch import opam.export
dune build
o1js
,cd src/mina
,git checkout leon/async-ssl-macos-compat
,cd ..
,git submodule update --init --recursive
, and thennpm run build:bindings-all
. (I did this on mac). You will see significant build log shrinkage because of the dune change, and it will also build natively on macos (woo).Checklist: