-
Notifications
You must be signed in to change notification settings - Fork 54
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
Platform Tag Improperly Set for Mac OS Wheels #977
Comments
Were you using CMake's documentation on Also, out of curiosity, where do you use this? Shouldn't the CI system set this accordingly? Footnotes |
Besides cibuildwheel, conda-build/rattler also sets it. If you are not building redistributable wheels, you generally don't need to set it at all, and if you are, you should be using something like cibuildwheel that can build redistributable wheels, this is one part of that. Usually you should set it via environment variable, so that all dependencies also build with the correct deployment target. Setting it manually would potentially not build everything with the correct target. If you set it in pyproject.toml, how would you handle conflicts with cibuildwheel and other build environments? You can already set (And if you are using cibuildwheel, you can set this in pyproject.toml for cibuildwheel already) |
Hey guys! Thanks for the quick response. Correct. I should clarify. When I'm wrapping a library with no dependencies that require relocation. As such cibuildwheel is overkill for my use case. Have either of you considered scikit-build-core running outside cibuildwheel? Shouldn't that be your primary use case? I'm comfortable with cmake and find sckikit-build-core behavior here counterintuitive. Clobbering defines and list file settings isn't the expected behavior. As a workaround hatch can be configured to set Thanks again and keep up the great work! |
Not sure what you mean here. It gets removed? We don't alter
For me, CI wise, I have one project that I test against Github Actions, conda and Fedora. But after CD, I only test them within Fedora because I don't know of tooling to test those other than |
My guess is "clobbered" means that when it computes the wheel tag it doesn't look up CMAKE_OSX_DEPLOYMENT_TARGET but just sets the current macOS version as the tag. If you set it in your CMakeFile, it is not reasonable to look it up, though it can be done if it's set in your pyproject.toml, but that seems a little weird that they would not be the same. And you'd have to look in both the cibuildwheel does a lot of other things for you too, such as downloading the official macOS installers for Python rather than the GitHub Actions installers1 (which are compiled for the lowest version GitHub supports, not 10.13), running auditwheel, etc. We very much support running outside of cibuildwheel if you don't want to redistribute, but for redistribution, we require you use a tool that supports redistribution, like cibuildwheel, multibuild, conda-build, rattler, or any distributions (pyodide, homebrew, fedora, Debian, arch, etc). Footnotes
|
No problem. When I say "clobbered' I mean that the setting I'm passing is being ignored or overwritten. Right on. I build on Github Actions for Windows, Linux, and MacOS. Shout out to dockcross! It's a great tool that I use to build, repair, and test manylinux wheels using a slim 55 line bash script. In my experience those tools aren't required for redistributable wheels. |
Windows is easy, nothing special is required. :) To be clear, all any of these tools are doing is setting Anyway, I wonder if we could inspect |
Ok, so if my understanding is correct, and @michaeltryby hope you can confirm as well. The issue is not on CMake build phase, i.e. it should work fine when run as
Once you get into the nitty-gritty and publish on PyPI for all linux users, then the gnarly issues with C compilers, standards, etc. pop up. Otherwise power to you to use your favorite build process, it helps us find these edge cases ;)
The lack of
Yeah makes sense. Let's hope somebody doesn't set it with |
Thanks! Keep it up! |
Hey Guys! Wanted to add latest observations to the Issue. Hopefully this will help you better diagnose the problem. When building wheels on an Intel Mac and cross-compiling arm64 using Xcode. I added the following to my pyproject.toml file.
I've confirmed that the cmake build has the When I install the wheel and run tests they fail because the binaries are in fact being compiled arm64.
So it seems that the scikit-build-core scheme for determining the platform tag for Mac builds is not working properly. Hope this helps! Thanks! |
You have to set this with environment variables to cross-compile. If you hard code it in This works on all backends, like setuptools, meson-python, maturin, etc. (For completeness, here is the macOS version setting too: https://github.com/pypa/cibuildwheel/blob/b656a829c8a01088b5448cea955a5e441de557ce/cibuildwheel/macos.py#L295-L316) |
It's completely reasonable to expect |
The issue is that right now But you pointed out a workaround for you is to set Also about the current issue, is it a problem when you try to build and install for your current macOS environment? I would think that CMake defaults would be clever enough to build when no extra variables are passed to work for your current environment. The default experience that a user running Otherwise I guess you are trying to cross-compile? In which case is it for your local test system or for deployment like in PyPI? For the latter you should consider making the For the case of manually defining the variables as [tool.scikit-build.cmake.define]
CMAKE_OSX_ARCHITECTURES = "arm64"
CMAKE_OSX_DEPLOYMENT_TARGET = "11.0" it would be a footgun because if you then share your project with somebody running intel macOS, then they would not be able to run that executable either. So we are more weary about blindly reading the variable directly because we do not want to just hand over a footgun, so apologies for the multiple back and forward on this while we iron out how/if this needs to be supported. |
Binary distributions like wheels were created so that end users would not have to build packages themselves. Therefore, the primary actor in configuring and building binary distributions has shifted from the end user to the package develeloper. If an end user on a platform without a wheel does build the package, they aren't going to be running cibuildwheel. They are probably going to build it directly on their target platform using scikit-build-core or the backend specified by the developer in the pyproject.toml file. Among other things, the move away from setup.py to pyproject.toml rationalizes and centralizes build configuration settings. Therefore, an end user building a package themselves should only have to look in one place -- the pyproject.toml file -- to adjust build settings for their target platform. Your reliance on undeclared dependencies like cibuildwheel and on undocumented environment variables is far more of a "footgun" than using a documented feature of your package -- I understand that you have constraints on how to implement things, however, leaving this broken in not a good design tradeoff. |
The "developer" produces wheels for common platforms. It's fine to assume they follow standards that work across all backends for those - If a "user" runs I think its fine to expect scikit-build-core to respect the standard1 mechanisms for cross-compiling and macOS version targeting like all other build backends for Python, and not be able to read the CMake way of doing things (remember, it can be set via define, by args, in your CMakeLists, or ideally a toolchain file!), even if it would be slightly better to be able to respect both. I think if it's reasonable to add, we can add it, but it's a) a low priority, b) requires writing a parser for CMakeCache.txt (hopefully doess't require parsing a toolchain file!), c) is rather invasive to the build procedure, since computing a tag now requires reading and parsing files produced in an earlier step, and d) might actually encourage bad practices like hardcoding platform assumptions into a pyproject.toml file. On the plus side, though, this might be helpful for linux cross-compiling, where there are no standards currently and it's likely to take a while to get a standard. Short-term, improving our docs is probably the first thing to work on. https://scikit-build-core.readthedocs.io/en/latest/crosscompile.html does mention ARCHFLAGS, but could also mention MACOSX_DEPLOYMENT_TARGET and could mention things that are not supported. Also, I see we have custom handling for CMAKE_SYSTEM_PROCESSOR in cmake_args (only). I think fixing that to handle all posibilties (and we could go ahead and add handling for at least the macOS version too while at it) could be medium term priority. And I'd be fine to review a PR for even a low priority plan. ;) If you are cross-compiling, you should also be setting at least Footnotes
|
Thanks for the clarification. I appreciate your efforts. Keep up the good work! |
Just to emphasise, we are not dependent on I for example am involved in the Fedora packaging ecosystem and try to make that process as easy as possible. One workflow that we are not covering yet, but we should, is yocto project (somebody is working on it I think). But other than that I don't think we have any other projects on our radar, so do let us know if you think we should check it. |
Update: we need to know the wheel tag before we make a build directory, since we allow |
Great work Henry! I'm migrating a project from scikit-build to scikit-build-core and have found the process straight forward.
I do have a suggestion though ...
Currently, the only way to set the cmake variable "MACOSX_DEPLOYMENT_TARGET" is via environment variable. Attempts to pass it as an argument to cmake or set it within a CMakeLists.txt file are clobbered by the default to the current MacOSX version. This logic is easily observed in the code and straightforward to reproduce.
The behavior, however, seems counterintuitive to CMake variable setting presedence which goes something like ...
passed command line argument > setting in list file > environment variable > default
Please consider updating this behavior.
Keep up the great work!
The text was updated successfully, but these errors were encountered: