-
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
macOS: wheels names disregarding deployment target for cp38+ #818
Comments
I feel like this was a bug that we were going to fix, but seems to have not been fixed. It should be fixed, or we should use pypa/wheel#407 which I haven't had a chance to implement yet. |
Agreed, this looks like a bug to me. I think that we should be setting Lines 89 to 99 in 02652e6
So I think we should bring the same logic into the code, too.
Edit: whoops! |
Please see #819 :) |
@boschmitt would you mind taking a look at #820? I'm having trouble recreating this bug. |
@joerick I had a look at #820, but I couldn't figure it out why the errors is not happening. First, I thought that it was because I'm using I tried to simplify the workflow as much as possible: Here is the result The relevant parts:
Produced wheels:
I'm doing some others tests on my side and I will try to look a bit further on #820. EDIT: It might be a long shot, but: Is there a possibility that this only happens with |
CMAKE_BUILD_PARALLEL_LEVEL - Hmm, that looks like you are not doing a standard setuptools build. It might handle the naming correctly but other tools might not. What are you using? |
scikit-build >= 0.12.0 - Ouch, I could have sworn we were supposed to handle this correctly. I expect https://github.com/scikit-build/scikit-build/blob/c0741f9a1de9dd97e0a1e6f0b3b02fcd76221418/skbuild/command/bdist_wheel.py#L69 might be the culprit? Possibly setuptools is taking this exactly if forced in this way. |
Probably ideally should be fixed in both libraries, cibuildwheel and scikit-build. |
I think its a standard setuptools build. I pretty much copied what is in the documentation. For these last tests, I removed anything that might be source of "weirdness", including
I'm don't have the required knowledge about the internals of |
Thanks for the recreation @boschmitt! Hmm... personally, I'm not sure we should really 'fix' this issue unless we can pin the problem down to cibuildwheel. For me, the question is: is the 'correct' value of The $ python3.9 -c 'import setuptools._distutils.util; print(setuptools._distutils.util.get_host_platform())'
macosx-10.9-universal2
$ MACOSX_DEPLOYMENT_TARGET=10.15 python3.9 -c 'import setuptools._distutils.util; print(setuptools._distutils.util.get_host_platform())'
macosx-10.9-universal2 These values come from sysconfig, they appear to be baked in when python was compiled. So I'm not sure that I'd consider setting So my feeling is to stick with our current approach, which is as close to the python-default behaviour as possible, while doing the minimum to support cross-compiling. |
I'm bitten by this in the far flung future of 2024: https://github.com/pypa/cibuildwheel/blob/main/cibuildwheel/macos.py#L245-L260 env.setdefault("MACOSX_DEPLOYMENT_TARGET", "11.0" if config_is_arm64 else "10.9")
if python_configuration.version not in {"3.6", "3.7"}:
if config_is_arm64:
# macOS 11 is the first OS with arm64 support, so the wheels
# have that as a minimum.
env.setdefault("_PYTHON_HOST_PLATFORM", "macosx-11.0-arm64")
env.setdefault("ARCHFLAGS", "-arch arm64")
elif config_is_universal2:
env.setdefault("_PYTHON_HOST_PLATFORM", "macosx-10.9-universal2")
env.setdefault("ARCHFLAGS", "-arch arm64 -arch x86_64")
elif python_configuration.identifier.endswith("x86_64"):
# even on the macos11.0 Python installer, on the x86_64 side it's
# compatible back to 10.9.
env.setdefault("_PYTHON_HOST_PLATFORM", "macosx-10.9-x86_64")
env.setdefault("ARCHFLAGS", "-arch x86_64") I don't understand why Maybe the solution is to move from cibuildwheel to scikit-build but right now I am procrastinating on that... |
Scikit-build is a build backend, cibuildwheel is a build front end that produces wheels. You can’t switch between them. What is your build backend? Sounds like it’s not respecting this, which means it’s probably one that was not really designed for binaries. I plan to fix this in hatchling in the future, and poetry-core probably doesn’t care. |
Thanks for clarifying that.
https://github.com/makslevental/mlir-wheels/blob/main/pyproject.toml#L13 build-backend = "setuptools.build_meta" which either means |
Setuptools is a build backend. I think your problem is you don't set MACOSX_DEPLOYMENT_TARGET. Set and use the standard variable for that (not OSX_VERSION), otherwise cibuildwheel sets it for you, which is what you are seeing. Most proper build backends (including setuptools) use MACOSX_DEPLOYMENT_TARGET instead of trusting what's in Also, you should use macos-14 for the Apple Silicon jobs instead of cross-compiling, simpler and faster. I expect you are not making usable SDists, since there's no MANIFEST.in. Scikit-build-core would be quite a bit simpler! |
Sorry I don't understand - I am setting CMAKE_OSX_DEPLOYMENT_TARGET but you're talking about the env MACOSX_DEPLOYMENT_TARGET that then sets the default for the cmake. But who's reading MACOSX_DEPLOYMENT_TARGET? I don't see that in cibuildwheel? Oh you're right it's setuptools. Sneaky sneaky...
Yea it's on my todo but my todo gets longer every day and CI is not ever at the top... Thanks for the pointers! I think I'll be able to find my way out now. |
I have a Github workflow that builds wheels for macOS. Since my library depends on C++17 for both core language features and library features, I set
MACOSX_DEPLOYMENT_TARGET
to be fairly high, i.e.,10.15
.When building the wheels, I get wheels with the correct name for CPython versions
3.6
and3.7
. However, the other versions are named*10.9*
:I believe the culprit code is:
cibuildwheel/cibuildwheel/macos.py
Lines 286 to 290 in 2709a49
As a workaround, I have manually set
_PYTHON_HOST_PLATFORM
to bemacosx-10.15-x86_64
, but I'm not sure if there might be other implications.The text was updated successfully, but these errors were encountered: