Skip to content
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

[python] Don't link C extensions to libpython #1395

Merged

Conversation

DownerCase
Copy link
Contributor

Description

Fixes the Python C extensions having an explicit link dependency on libpython. In environments without libpython this causes problems. The easiest way to encounter this is through pyenv.
Eg: Building the wheel using a Python install that has libpython and trying to install and run it in a Python without it (static build).

The first hint of the problem is running ldd on the shared object when using a pyenv installation (for example):

ldd out/wheel/build/python/ecal/_ecal_core_py.so
        linux-vdso.so.1 (0x00007ffefb729000)
        libpython3.12.so.1.0 => not found
        ...

This will actually run fine if it is run in a Python interpreter that has a shared libpython as the wheels inherit the symbols from Python (see the PEP 513 link below). However; trying to run this eCAL wheel in a statically built Python will cause an error like:

python samples/python/benchmarks/latency_snd/latency_snd.py
Traceback (most recent call last):
  File "/home/downercase/dev/ecal/samples/python/benchmarks/latency_snd/latency_snd.py", line 24, in <module>
    import ecal.core.core as ecal_core
  File "/home/downercase/dev/ecal/.venv_312_static/lib/python3.12/site-packages/ecal/core/core.py", line 26, in <module>
    import ecal._ecal_core_py as _ecal
ImportError: libpython3.12.so.1.0: cannot open shared object file: No such file or directory

PEP 513 supporting material

Fixing this is one small step closer to redistributable wheels.

Related issues

Cherry-pick to

  • 5.12 (old stable)
  • 5.13 (current stable)

@DownerCase
Copy link
Contributor Author

Update: Need to do some more testing/investigation on this... Looks like it may only be a CPython 3.8+ solution.
https://bugs.python.org/issue21536
python/cpython#12946

@KerstinKeller
Copy link
Contributor

Does this actually work? I though linkage was required, but since the build is green, it's not necessary?
Maybe correct linkage is already assumed by using the CMake helper functions to create Python extensions?

However, in general, the whole Python build process should be triggered through Python instead of through CMake.
E.g. Python should trigger CMake. I started to investigate some Python build backends (scikit-build-core, ...) but in the end I somehow got stuck because I couldn't quite model the correct behavior.

It's definately on our list to have proper pip support and universal linux wheels and most effort should go in that direction.
But per se - if that works, let's just remove those two linking lines 😄

@DownerCase
Copy link
Contributor Author

Does this actually work? I though linkage was required, but since the build is green, it's not necessary? Maybe correct linkage is already assumed by using the CMake helper functions to create Python extensions?

Your helper here correctly calls Python_add_library with module which implicitly adds a dependency to Python::Module

I reproduced the error on Linux using Python 3.12 and confirmed that removing the link fixes it for "build with shared -> use with static".

From the research it looks like this was only made possible with Python 3.8 so just need to check behaviour under 3.7 and re-verify Windows too.

However, in general, the whole Python build process should be triggered through Python instead of through CMake. E.g. Python should trigger CMake. I started to investigate some Python build backends (scikit-build-core, ...) but in the end I somehow got stuck because I couldn't quite model the correct behavior.

It's definately on our list to have proper pip support and universal linux wheels and most effort should go in that direction. But per se - if that works, let's just remove those two linking lines 😄

This is definitely something I would love to see too. Happy to help out where possible to make it happen for eCAL 6 😄

@DownerCase
Copy link
Contributor Author

DownerCase commented Mar 2, 2024

TLDR: Everything looks good 😄

From the research it looks like this was only made possible with Python 3.8 so just need to check behaviour under 3.7 and re-verify Windows too.

I read my link more closely, looks like it was only changing how distutils builds C extensions, so not relevant in this situation. Even if it does give some helpful discussion.

Anyway, onto my verification testing.

Windows:
Static Python builds on Windows isn't really a thing as far as I can tell. eg
This patch removes duplicate linking entries.
This is the only difference in the Ninja build rules. Left is before, right is after.
image

Ubuntu 22.04 LTS (WSL) with Python 3.7 built via pyenv:

Before:
Shared Python show links to libpython*.so.1.0 as you'd expect
Static Pythons do not.
✅ Using static built wheel in shared
❌ Using shared built wheel in static

After:
✅ Static build wheel in shared
✅ Shared built wheel in static

Inspecting the build differences again we see a very similar thing, expect that the libpython linkage is completely removed.
image

To summarize everything.

  • The ecal_add_python_module helper correctly calls Python_add_library with the MODULE argument.
  • This creates a CMake library target correctly linked for Python C extensions (and includes the Python.h include directory).
  • Prior to this patch eCAL was also linking again, explictly to libpython via Python::Python
  • For Windows this had no real consequence.
  • Under Linux this adds a dependency of the shared object on libpython
  • As a result, any statically linked versions of Python (eg: custom source build) cannot use an eCAL wheel built by a shared (i.e: most package manager versions) Python build.
  • This patch removes the unneeded dependency on libpython on Linux
  • This is fine as C extensions inherit symbol definitions from the parent Python execution environment. (Technical terms probably not quite right, but you get the idea)

(This is probably the most effort I've put into a 2 line change 😆)

@KerstinKeller
Copy link
Contributor

Thanks for all the investigation that you put into this topic 👍
On Windows you can really see, that the linker argument to the Python lib was present twice.
On Linux I am not quite sure I get the part about inheriting the symbols definitions, but as long, as they are there, that sounds good.

I will merge your PR, thanks for contributing!

@KerstinKeller KerstinKeller merged commit 953c671 into eclipse-ecal:master Mar 4, 2024
7 checks passed
@DownerCase DownerCase deleted the fix/dont_link_libpython branch March 4, 2024 11:34
FlorianReimold pushed a commit that referenced this pull request Mar 5, 2024
Cherry pick to:
5.12
5.13.
FlorianReimold pushed a commit that referenced this pull request Mar 5, 2024
Cherry pick to:
5.12
5.13.
FlorianReimold pushed a commit that referenced this pull request Mar 5, 2024
Cherry pick to:
5.12
5.13.
FlorianReimold pushed a commit that referenced this pull request Mar 5, 2024
Cherry pick to:
5.12
5.13.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants