-
Notifications
You must be signed in to change notification settings - Fork 239
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
DOC: Document macOS 3.8 arm64 workaround #1871
Conversation
The reason this is this way is because you can't target 10.9-10.15 with 3.9, only 11+, unlike every other binary, including the 3.9+ universal builds. 3.8 was actually built with 11+, even on the Intel side. Maybe we should change this though, especially now that ARM runners are common (and hopefully no one will care about Python 3.8 by the time all the Intel runners are gone)? We could error if MACOSX_DEPLOYMENT_TARGET was set to anything under 11 on 3.8 running from ARM building Universal or Intel wheels? |
Actually, shouldn't this only affect targeting ARM, not Universal or Intel? I'm confused now. This is an issue for the universal build, but the ARM build should be safe, I think. We didn't have ARM runners back when I added 3.8. If that's the case, we can also enable testing for 3.8 as long as it's ARM only and not Universal. |
I'm not sure I understand the subtleties here, but in my case at least the error happens for me on GHA
For me I originally saw the error with |
Hmm, guessing Actions is down? |
Nah, happy to see it exist, no credit needed. |
We currently don't change this based on MACOSX_DEPLOYMENT_TARGET (and that would be a little tricky, I think), but ARM should always treat it as if it's 11, since that's the first macOS for ARM. So I'm thinking while our current system is sadly required for universal2, it's safe to do this for pure ARM. I don't think we reinstall, so this could cause an issue if someone builds non-pure-ARM (including universal) and ARM in the same job. @mayeut, thoughts? |
Looks like the issue was due to the fact we don't reinstall Python if it exists: (FYI, due to that, you can also manually "fix" this by pre-installing Python.org universal installer before running cibuildwheel, IIRC, but not the cleanest solution) Step 1 I think then would be to reinstall Python if it's not the right variety. Looks like no one ever took that up. |
Ahh sorry I didn't read the whole issue, just jumped to a potential solution and tried it 🤦
Okay I did that in h5py/h5py@51bb92d and it seems to work! https://github.com/h5py/h5py/actions/runs/9472998912/job/26099471057 |
I'm not sure there's a clean solution. Force re-install might be one for CI with a warning on local builds. |
Okay I've updated this PR just to be a doc update! |
docs/faq.md
Outdated
|
||
There is also an 'experimental' installer available that's built natively for arm64 that you can install manually with something like: |
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.
Isn't the workaround documented just below here already? The workaround shown installs the official build, rather than GitHub's recompilation with a higher MACOSX_DEPLOYMENT_TARGET. (Assuming they are recompiled, not sure they are anymore)
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's the same experimental installer used on GHA for arm64.
arm64 on GHA always uses installers from python.org (this wasn't the case for x86_64 before python 3.11)
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.
Would it speed up cibuildwheel for a user to pre-install these via setup-python, actually? Might be a bit of a micro-op, just thinking.
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.
Would it speed up cibuildwheel for a user to pre-install these via setup-python, actually? Might be a bit of a micro-op, just thinking.
The only diff in speed I could see would be downloading from GitHub vs python.org for non pre-installed pythons. I don't think it would be noticeable given only 3.8 is not preinstalled on macos-14 (and might add another source of potential failure), we can see in the test workflow that for the action sample build, python 3.9+ cibuildwheel install time is 0s. The same goes for Python 3.11+ on x86_64.
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.
If they're functionally equivalent then I'm happy to just change this doc to use setup-python
and remove the manual install steps. It's more standard practice across GHA to use setup-python
so if it's functionally equivalent or better to use it, it seems like the docs should suggest that rather than curl
/sudo
/sh
steps suggested currently. I'll change the text to remove the old way, but if that's not preferred feel free to close!
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.
The old way works on other CI systems, the GHA one is GHA only. I wonder if tabs here would be better? GHA and general.
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.
Good call, changed to tabs!
Co-authored-by: Matthieu Darbois <[email protected]>
Thanks for the fixes, applied! |
* BUG: Fix bug with macOS 3.8 arm64 URL * TST: Ping * DOC: Update * FIX: Remove old * DOC: Both * Apply suggestions from code review Co-authored-by: Matthieu Darbois <[email protected]> --------- Co-authored-by: Matthieu Darbois <[email protected]>
I just hit #1278 :
https://github.com/h5py/h5py/actions/runs/9472096444/job/26096834822#step:9:483
Using the suggestion from #1278 (comment) in h5py/h5py@87cf54c fixes it:
https://github.com/h5py/h5py/actions/runs/9472215836/job/26097237985?pr=2444#step:9:179
@cretz do you want to open a quick PR instead so you can get credit (and blame 😄 ) for this change?