fix: android CMake support #5733
Conversation
Signed-off-by: Henry Schreiner <[email protected]> Co-authored-by: Malcolm Smith <[email protected]>
henryiii
force-pushed
the
henryiii/fix/cmakeandroid
branch
from
6a92b34 to
9967879
Compare
| # On Android, older versions of CMake don't know that modules need to link against | ||
| # libpython, so Python::Module will be an INTERFACE target with no associated library | ||
| # files. | ||
| get_target_property(module_target_type ${_Python}::Module TYPE) | ||
| if(ANDROID AND module_target_type STREQUAL INTERFACE_LIBRARY) | ||
| set_property( | ||
| TARGET ${_Python}::Module | ||
| APPEND | ||
| PROPERTY INTERFACE_LINK_LIBRARIES "${${_Python}_LIBRARIES}") | ||
| endif() |
There was a problem hiding this comment.
I just looked at the original problem in vcpkg (pybind11 2.6.13) and came to a similiar fixup, however injected on top of find_package(Python). With 3.0.0, I found android broken again for a different reason, and now I'm here with questions:
older versions of CMake don't know that modules need to link against libpython
Has this issue been reported to CMake? I cannot see any relevant changes, so I don't understand how this is limited to "older versions".
IMO the situation is similar to Windows DLLs: no undefined symbols. And this is how it is handled for Windows:
https://github.com/Kitware/CMake/blob/6754d2f9b8d5ba24b87db232a0aedca8d15c707d/Modules/FindPython/Support.cmake#L1517-L1522
https://github.com/Kitware/CMake/blob/6754d2f9b8d5ba24b87db232a0aedca8d15c707d/Modules/FindPython/Support.cmake#L4327-L4335
There is no trace of ANDROID in this file.
set_property(TARGET ${_Python}::Module ...)
IMO it should be avoided to modify imported targets from other CMake modules. In particular, I don't see why it is not sufficient to modifiy pybind11::module.
INTERFACE_LINK_LIBRARIES "${${_Python}_LIBRARIES}"
This is the spot which causes trouble in vcpkg now. ${${_Python}_LIBRARIES} is initialized for multi-config use in target_link_libraries, with debug and optimized keywords which are not allowed when setting the property directly.
My initial fixup is to use $<LINK_ONLY:${_Python}::Python>.
Alternatively, is possible to use target_link_libraries(pybind11::module INTERFACE ${${_Python}_LIBRARIES})
Disclaimer: I'm a python rookie.
vcpkg WIP: microsoft/vcpkg#46539
There was a problem hiding this comment.
set_property(TARGET ${_Python}::Module ...)IMO it should be avoided to modify imported targets from other CMake modules. In particular, I don't see why it is not sufficient to modifiy
pybind11::module.
... Maybe the goal is to fix the following?
Python_add_library(MyModule2 src2.cpp)
target_link_libraries(MyModule2 PRIVATE pybind11::headers)There was a problem hiding this comment.
Testing with the openimageio port, using ${${_Python}_LIBRARIES} still fails because the actual variable Python3_LIBRARIES is not set in the scope of this line of code.
There was a problem hiding this comment.
Okay, the openimageio issue is from looking for Interpreter Development.Module components if not on Windows. Development should be at least in OPTIONAL_COMPONENTS.
There was a problem hiding this comment.
Hi, it was me who originally wrote this code in #5714, but I didn't subscribe to this PR so I didn't notice your comments. Sorry about that.
Has this issue been reported to CMake? I cannot see any relevant changes, so I don't understand how this is limited to "older versions".
I'm currently working on a fix which I'll submit to CMake, and then I'll post a link here so you can review it. In the long run, this should allow all of these workarounds to be removed.
IMO it should be avoided to modify imported targets from other CMake modules. In particular, I don't see why it is not sufficient to modifiy
pybind11::module.... Maybe the goal is to fix the following?
Python_add_library(MyModule2 src2.cpp) target_link_libraries(MyModule2 PRIVATE pybind11::headers)
Yes, that was the reason. This pattern is recommended in pybind11's documentation, so it will be used by many Python packages.
${${_Python}_LIBRARIES}is initialized for multi-config use intarget_link_libraries, withdebugandoptimizedkeywords which are not allowed when setting the property directly.Alternatively, is possible to use
target_link_libraries(pybind11::module INTERFACE ${${_Python}_LIBRARIES})
Thanks, that seems like a good solution. I'll test it, and if it works, I'll submit it as another pybind11 PR.
Description
Pulled out the CMake changes from #5714.
Suggested changelog entry: