-
Notifications
You must be signed in to change notification settings - Fork 101
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
Add macOS universal2 wheel building support #115
Conversation
setuptools_rust/setuptools_ext.py
Outdated
universal2 = all(ext.universal2 for ext in self.distribution.rust_extensions) \ | ||
if self.distribution.rust_extensions else False | ||
if universal2 and plat.startswith("macosx_"): | ||
plat = "macosx_10_9_universal2" |
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 am not sure if this is the right way to set the platform tag for universal2 wheel.
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 looks like sysconfig.get_platform()
might return something with universal2
in it, which could be better? pypa/setuptools#2520
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 only returns macosx-10.9-universal2
on macOS universal2 Python build, does not play well with cross compilation.
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 doesn't return universal2
with Apple's bundled Python in Xcode which is universal2 build itself.
❯ python3
Python 3.8.2 (default, Dec 21 2020, 15:06:03)
[Clang 12.0.0 (clang-1200.0.32.29)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.prefix
'/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8'
>>> import sysconfig; sysconfig.get_platform()
'macosx-10.14.6-arm64'
❯ file /Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/bin/python3.8
/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/bin/python3.8: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit executable x86_64] [arm64:Mach-O 64-bit executable arm64]
/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/bin/python3.8 (for architecture x86_64): Mach-O 64-bit executable x86_64
/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/bin/python3.8 (for architecture arm64): Mach-O 64-bit executable arm64
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 looks like
sysconfig.get_platform()
might return something withuniversal2
in it, which could be better? pypa/setuptools#2520
Sysconfig.get_platform() will return "universal2" as the "CPU architecture" when Python was build as a universal 2 binary using the configure script. This is true for the (currently experimental) universal2 installer for Python 3.9.1 on Python.org, as wel as recent 3.10 alpha installers.
The python included in Xcode does not return "universal2" in the platform tag, likely because it was build twice and lipo-d together.
My reason for filing #108 is to have a way to build universal2 wheels that match the python.org installers (including a deployment target of macOS 10.9 if that is at all possible using the default rust toolchain). Such wheels would also be compatible with other macOS builds of CPython (such as the ones in Xcode and home-brew).
Having support for this in this project is a prerequisite for me asking users of setuptools-rust to provide universal2 binary wheels (although there's also the question of CI support, none of the large cloud CI systems currently have Apple M1 runners)
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 think to set deployment target to macOS 10.9 you just need to set environment variable MACOSX_DEPLOYMENT_TARGET=10.9
. https://stackoverflow.com/a/64864364
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 looks like sysconfig.get_platform()
might return something with universal2
in it on some macOS versions: pypa/setuptools#2520
... rather than adding a universal2
option, should we be detecting this and building universal2
wheels automatically?
cc @ronaldoussoren in case you know about this?
setuptools_rust/setuptools_ext.py
Outdated
universal2 = all(ext.universal2 for ext in self.distribution.rust_extensions) \ | ||
if self.distribution.rust_extensions else False | ||
if universal2 and plat.startswith("macosx_"): | ||
plat = "macosx_10_9_universal2" |
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 looks like sysconfig.get_platform()
might return something with universal2
in it, which could be better? pypa/setuptools#2520
Rebased and added tests. |
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.
Thanks, and sorry for the slow review again by me.
What do you think if instead of setting universal2=True
in the RustExtension
class, we use an environment variable (or an option to bdist_wheel
) to configure this?
This is because I think that users running python setup.py install
won't care at all about whether their binary is universal2
and might just find it harder to build if they start getting error messages about lipo
and fat-macho
.
Really the only time this setting is relevant (I think?) is when package maintainers are running their CI jobs to build wheels.
It looks like ARCHFLAGS
might be an appropriate environment variable to look at: https://stackoverflow.com/questions/64364310/how-do-i-resolve-error-architecture-not-supported-during-pip-install-psutil
examples/universal2_abi3/Cargo.toml
Outdated
git = "https://github.com/PyO3/pyo3.git" | ||
# version = "0.13.2" |
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 guess same TODO could go here.
Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
I wasn't aware of |
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.
Thanks for the multiple revisions to this PR, this looks great to me now!
I think it needs some documentation, but there's not really a good place to document it so I'll do that when I write some readthedocs documentation soon.
Based on #114 fixes #108