OpenAssetIO icon indicating copy to clipboard operation
OpenAssetIO copied to clipboard

Fix Python linking

Open feltech opened this issue 3 years ago • 1 comments

What

Ensuring Python version

find_package(Python, when given a specific version, only guarantees a version greater than or equal to. This means an unexpected Python can be found (e.g. system 3.8 when requested conan's 3.7). We should add EXACT to the find_package call.

We also still default to 3.7 when it should be 3.9 for CY2022.

Using shared libpython

This is only a problem if we have a need to link to libpython, which for most purposes we don't (though see next point re. embedding)

The current conancenter cpython package recipe defaults to shared=False, meaning libpython is a static library and the python interpreter (presumably) has the library compiled in.

Setting shared=True gives a shared library, but the RPATH of the interpreter is not set, so it does not function (e.g. for creating the venv and running tests) without patching.

find_package(Python prefers shared libraries but will silently use static libraries if that is all that is found. If we want to force shared libraries then we must set Python_USE_STATIC_LIBS to false.

Using embedded Python interpreter (for testing / app integration)

When creating a target with an embedded Python interpreter (e.g. for tests, as used in a prototype for testing C++ loading the Python plugin system) we must add Development.Embed to the list of COMPONENTS passed to find_package(Python.

This has the effect of making any Python-related target link to libpython (even if pybind::module is the only linked library, whose whole purpose is to avoid linking libpython). This is bad - Python extension modules should not link directly to libpython. This appears to be a bug in either pybind or Python's CMake helpers.

If Development.Embed is not specified (i.e. only Development.Module) then everything works fine, but we cannot create a (test) executable with an embedded Python interpreter.

Beyond use for testing, this may be a problem when adopters of OpenAssetIO come to use it in their application with an embedded Python interpreter.

feltech avatar Jul 21 '22 15:07 feltech

Re. libpython being erroneously linked in with pybind11::module (i.e. Python extension modules) when the Development.Embed component of Python is available:

I discovered the problem is the autogenerated conan Findpybind11.cmake, which makes some odd choices that should probably be reported upstream.

Conan overrides pybind11::pybind11 so it doesn't match the pybind original (which just defines headers) but instead links in everything to do with pybind in one mega "global" target (see pybind11_COMPONENTS in Findpybind11.cmake). This includes the pybind11::embed target that brings in libpython as a dependency.

At the bottom of the autogenerated Findpybind11.cmake it finally brings in pybind's own pybind11Common.cmake. In here we see

set_property(
  TARGET pybind11::module
  APPEND
  PROPERTY INTERFACE_LINK_LIBRARIES pybind11::pybind11)

I.e. pybind11::module is linked to (the overridden global mega target) pybind11::pybind11, which in turn links to pybind11::embed, and causes the erroneous libpython linking.

EDIT: ... and now I know what to look for, turns out this is a known bug: https://github.com/conan-io/conan-center-index/issues/6605

feltech avatar Jul 21 '22 22:07 feltech

The pybind11 Conan recipe bug was fixed a while ago. See 6fcefba from #764 for explanation.

feltech avatar Jun 13 '23 10:06 feltech

All these problems have been resolved by other work.

elliotcmorris avatar Mar 07 '24 16:03 elliotcmorris