-
Notifications
You must be signed in to change notification settings - Fork 694
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
Fix support for Apple M1 #753
Conversation
…ion since they do not yet have M1 support, but Python, LLVM, Emscripten and Binaryen will be native.
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.
Generally looks great. If you want to attach the python tar archive I'm happy to upload it to google storage for you.
macOS Arm64 python: http://clb.confined.space/dump/python-3.9.2-1-macos-arm64.tar.gz |
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!
arm64 python uploaded.
Do you want me to upload the x86-64 mac python too or (perhaps better) leave that version as if for now (will require forking upstream-main
on line 490 I guess?)
Our min spec for Intel macOS is version 10.13, so I've been wrangling today a chance to do the Intel side build either on real macOS 10.13, or with environment variables
(where If you'd like to do an upload, if you can do it with the above flags, that'd be great. Or you can do just an upload with a regular build, and I can also do a local build just for our Unity deployment purposes. Or if you want to wait, I can submit that build as well once I get my hands on a suitable Intel mac environment to work with. |
Can we just least the x86-64 mac python on 3.7.4 and not upload anything new there? |
This would also make this change more precise as it will effect M1 users. |
We could, but it will require splitting the manifest sdks part per both macOS archs. I would prefer not to do that. |
I don't really mind which option we choose, but this PR is currently blocked on getting a new python x86-64 binary right? Forking the manifest is one way to unblock it. I don't have access to a macOS machine myself. (I guess its possible to use the circleci mac via SSH bot perhaps?) |
That's right - I'll unblock that tomorrow and upload the new binary, did not have a chance to do it today. |
Added http://clb.confined.space/dump/python-3.9.2-1-macos-x86_64.tar.gz for macOS 10.13 min version build. Also can you reupload http://clb.confined.space/dump/python-3.9.2-1-macos-arm64.tar.gz . I updated that one to run with minimum version 11.0 (rather than 11.1 that it was previously targeting). Also updated the PR to specify these 10.13 and 11.0 minimum requirements for the build script. |
Done What was the min version in the previous/current version of python for x86_64? |
It would be whatever macOS version was installed on the system where the zips were created on. |
min_macos_version_line = '-mmacosx-version-min=' + min_macos_version # Specify the min OS version we want the build to work on | ||
build_flags = min_macos_version_line + ' -Werror=partial-availability' # Build against latest SDK, but issue an error if using any API that would not work on the min OS version | ||
env = os.environ.copy() | ||
env['MACOSX_DEPLOYMENT_TARGET'] = min_macos_version |
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.
Apparently you only need either the command line of the environment variable, not both:
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.
That is true, but there is ambiguity in which method is preferred (https://stackoverflow.com/questions/25352389/what-is-the-difference-between-macosx-deployment-target-and-mmacosx-version-min highlights the cmdline flag no longer works, and https://github.com/devernay/xcodelegacy#using-the-older-sdks highlights the environment variable should be deprecated), and which method is supported on different compilers, so the general practice is to supply both. There is no harm in specifying them both for the build.
min_macos_version = '10.13' | ||
elif platform.machine() == 'arm64': | ||
prefix = '/opt/homebrew' | ||
min_macos_version = '11.0' |
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.
else assert?
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 code will fail on the next line with these variables not defined, preferred not to add redundant lines (like the unnecessary else:
in if x: return foo else:
you often comment about)
I wonder if we can figure that out.. it would be good to know if we are increasing of decreasing the min requirments with this change.. maybe we inspect the binaries to figure it out (i'll give it a go) |
Perhaps should also mention this in the |
Looks like the previous version was 10.12:
|
macOS 10.13 was released in September 2017. Cross comparing system requirements at https://en.wikipedia.org/wiki/MacOS_High_Sierra vs https://en.wikipedia.org/wiki/MacOS_Sierra , there are no mac hardware that would not be able to update from macOS 10.12 to macOS 10.13 (and hardware from 2009 onwards is compatible), so not concerned about bumping the minimum version. Emsdk on macOS requires system installed python in order to install Emsdk bundled python. According to https://medium.com/@youngstone89/mac-os-x-sierra-python-installation-28db734a2a60 and https://stackoverflow.com/questions/41647424/how-to-install-updated-python-on-mac-os-sierra-with-brew, macOS 10.12 had Python 2.7.10 preinstalled, no Python 3. According to https://stackoverflow.com/a/33175893 and https://stackoverflow.com/questions/51804203/atom-prioritizes-python-2-7-over-python-3-7-on-macos macOS 10.13 ships with the same Python 2.7.10 as well. So that will be the same minimum version that Emsdk needs to work with. |
This is all fine with me. I just want to make sure we track this stuff. Thanks for investigating. |
Fix support for Apple M1. Node.js will still run via Rosetta 2 emulation since they do not yet have M1 support, but Python, LLVM, Emscripten and Binaryen will be native.
In order for Python to actually be native, will need to upload new Python to Google CDN built with update_python.py.