fix: add Android support #1142
fix: add Android support #1142
Conversation
README.md
Outdated
| project(${SKBUILD_PROJECT_NAME} LANGUAGES C) | ||
| find_package(Python COMPONENTS Interpreter Development.Module REQUIRED) | ||
| find_package(Python COMPONENTS Development.Module REQUIRED) |
There was a problem hiding this comment.
Skipping interpreter can be good and bad. Iirc cross compilation with cross-env behaves differently with and without it, but I don't remember which one is more preferred.
There was a problem hiding this comment.
I think CMake 4.1 kind of forces the no Interpreter case. From what I understand, you can set a emulated Python for it, though.
There was a problem hiding this comment.
IIRC, since we don't correctly find and pass the Python library, adding the Interpreter components helps on Windows. If we could reliably find the Python library, then we don't need the Interpreter component. For some reason CMake is better than us at finding Python. I'd like a more principled approach than #1093's seeming "try everything" method, but we likely need something to get rid of the Interpreter component.
There was a problem hiding this comment.
I personally think the user should provide the hints themselves when cross-compiling in the toolchain file, and we just document that. There are too many edge-cases we would have to account for, which otherwise could be better handled by SYSROOT and such
There was a problem hiding this comment.
I'm referring to when not cross compiling - we currently should compute this unless the user specifies something different, but instead we usually don't find it and rely on CMake to find the Python library using the interpreter. Yes, when cross-compiling, it has to be specified.
docs/guide/crosscompile.md
Outdated
| - An Android | ||
| [toolchain file](https://cmake.org/cmake/help/latest/variable/CMAKE_TOOLCHAIN_FILE.html) | ||
| which adds the location of the Python headers and libraries to | ||
| `CMAKE_FIND_ROOT_PATH`. |
There was a problem hiding this comment.
Until #1050 is resolved, better add a note about how to pass CMAKE_TOOLCHAIN_FILE. Also should add an explicit note about what happens if you are finding Interpreter component or not.
There was a problem hiding this comment.
I've added more details of the toolchain file, and reverted all the Interpreter changes as explained in my comment below.
| if python_library: | ||
| cache_config[f"{prefix}_LIBRARY"] = python_library | ||
| if python_sabi_library: | ||
| cache_config[f"{prefix}_SABI_LIBRARY"] = python_sabi_library |
There was a problem hiding this comment.
This part needs some discussion. I think this would point these to an unwanted location. Could really use a more concrete example of how to cross-compile to see how these are expanded. #1050 is also relevant for these.
There was a problem hiding this comment.
This is what we already have, though, just with android added alongside windows?
There was a problem hiding this comment.
Yes, but I don't believe it is needed or that it has the intended effect when cross-compiling. E.g. are we passing the library of the host or target environment there? A more concrete example of how a cross-compiled android project would help a lot, as well as how each environment does it
There was a problem hiding this comment.
If we don't pass it, then it is required to include Interpreter, since CMake has to find it, it does so by queuing the Python interpreter (which is set to sys.interpreter, so literally the same python we are in now). If a user provides a toolchain file for cross compiling, these get overridden.
Though, to your point, since android will never be the host, not sure adding it here is needed?
There was a problem hiding this comment.
If we don't pass it, then it is required to include
Interpreter, since CMake has to find it, it does so by queuing the Python interpreter (which is set tosys.interpreter, so literally the same python we are in now).
That is specific to windows though right? Because on linux it would use the bash file (forgot the name of it) that contains the configure values used when compiling (dunno why they don't provide something similar to windows though).
Though, to your point, since
androidwill never be the host, not sure adding it here is needed?
Yes that would be preferred imo with relevant documentation
There was a problem hiding this comment.
It's not needed on other platforms, only Windows and Android care. So I'm not really sure, as it's not used.
I saw that in the CMake documentation, but I initially rejected it because starting up a whole Android emulator just to run a short Python script would be too slow. However, then I realized that cibuildwheel's simulated Android Python environment effectively is an emulator as far as FindPython is concerned. So all we need to do is add a dummy CMAKE_CROSSCOMPILING_EMULATOR to the toolchain file (pypa/cibuildwheel#2591), and then we can build for Android with the This is a much better solution, because it's compatible with the many existing packages which already have |
|
(I'll work on helping with this when I get back from India, sorry for the delay!) |
The main motivation for this PR is CMake policy CMP0190, which will prohibit calling FindPython with both the
InterpreterandDevelopmentcomponents at the same time when cross-compiling. To encourage packages to be compatible with cross-compilation, I've removedInterpreterfrom all the documentation, examples and tests, except for the places which actually needed it.CMake is still able to find the Android Python libraries by itself without an interpreter, but it takes a very long time (20 seconds). Going by the
--debug-findoption, this may be because it's searching a huge number of nonexistent paths, including every possible combination of Python version andabiflagssquared (both for the directory name and the library name).So I've changed builder.py to tell CMake the Python library location on Android, as it already does on Windows.
This performance problem doesn't affect the other Unix platforms, because they have the
python3.x-configscript available.See also: