Skip to content

Don't link to libpython for Python >= 3.8 #109

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

Merged
merged 1 commit into from
Jun 10, 2020

Conversation

gferon
Copy link
Contributor

@gferon gferon commented Jun 2, 2020

As of Python 3.8, python C extensions modules should not link to
libpython, unless they embed the interpreter in their code.

Relevant upstream PR: python/cpython#12946

Signed-off-by: Gabriel Féron [email protected]

As of Python 3.8, python C extensions modules should not link to
libpython, unless they embed the interpreter in their code.

Relevant upstream PR: python/cpython#12946

Signed-off-by: Gabriel Féron <[email protected]>
@Ghostkeeper
Copy link
Contributor

It looks also like this won't be possible on Windows, according to that PR and the changelog of Python. So this only applies to Linux then.

I'm using Python 3.8 in my development environment and linking to CPython works just fine. I understand that this makes it easier to create a build script, but with the build script already set up does it have any tangible advantage?

@hroncok
Copy link
Contributor

hroncok commented Jun 5, 2020

I'm using Python 3.8 in my development environment and linking to CPython works just fine.

Yes, linking works fine. So does not linking. Not linking makes it possible to import the module from a debug build of Python. Linking gives no advantages. So:

Linking to libpython:

  • works
  • can be safely done everyhwhere
  • the module can only be imported in Python linked against the same libpython

Not linking to libpython:

  • works (on Python 3.8+ on Unix)
  • needs "logic" to determine whether to link (or using python-config --libs)
  • the module can be imported into Python linked against a different libpython (e.g. the debug build)

I understand that this makes it easier to create a build script...

I don't understand this part. What build script?

@Ghostkeeper
Copy link
Contributor

Ghostkeeper commented Jun 9, 2020

I don't understand this part. What build script?

I meant the CMake scripts, such as the modified file: https://github.com/Ultimaker/libArcus/blob/master/cmake/SIPMacros.cmake . I guess technically that would be a "configure" script to the maintainers, sorry 😃 You wouldn't need to find the Python libraries, and then wouldn't need to write that TARGET_LINK_LIBRARIES command. But this is not an advantage for us since we got it working this way, and we'd need to keep it in anyway because we're using Python 3.5 for the AppImage.
Since Python 3.8 the debug build is also binary compatible with the release build. I get what you mean though, in case people want to import this Python 3.8 build into Python 3.9. That is the tangible benefit I was asking for.

@Ghostkeeper Ghostkeeper merged commit 4ebc453 into Ultimaker:master Jun 10, 2020
@krop
Copy link
Contributor

krop commented Jul 27, 2020

This commit breaks the build when using '-Wl,--no-undefined'

eg:

[ 54%] Linking CXX shared library Arcus.so
/usr/bin/cmake -E cmake_link_script CMakeFiles/python_module_Arcus.dir/link.txt --verbose=1
/usr/bin/c++ -fPIC -O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type -flto=auto -g -DNDEBUG -O2 -g -DNDEBUG -flto=auto -Wl,--as-needed -Wl,--no-undefined -Wl,-z,now -shared -Wl,-soname,Arcus.so -o Arcus.so CMakeFiles/python_module_Arcus.dir/python/sipArcuspart0.cpp.o CMakeFiles/python_module_Arcus.dir/python/sipArcuspart1.cpp.o CMakeFiles/python_module_Arcus.dir/python/sipArcuspart2.cpp.o CMakeFiles/python_module_Arcus.dir/python/sipArcuspart3.cpp.o CMakeFiles/python_module_Arcus.dir/python/sipArcuspart4.cpp.o CMakeFiles/python_module_Arcus.dir/python/sipArcuspart5.cpp.o CMakeFiles/python_module_Arcus.dir/python/sipArcuspart6.cpp.o CMakeFiles/python_module_Arcus.dir/python/sipArcuspart7.cpp.o CMakeFiles/python_module_Arcus.dir/python/PythonMessage.cpp.o  -Wl,-rpath,/data/3D/libArcus/build: libArcus.so.1.1.0 /usr/lib64/libprotobuf.so -lpthread 
/usr/lib64/gcc/x86_64-suse-linux/10/../../../../x86_64-suse-linux/bin/ld: /tmp/Arcus.so.i06lpg.ltrans0.ltrans.o: in function `meth_SocketListener_stateChanged':
/usr/include/python3.8/object.h:459: undefined reference to `_Py_NoneStruct'
/usr/lib64/gcc/x86_64-suse-linux/10/../../../../x86_64-suse-linux/bin/ld: /tmp/Arcus.so.i06lpg.ltrans0.ltrans.o: in function `meth_SocketListener_messageReceived':
/usr/include/python3.8/object.h:459: undefined reference to `_Py_NoneStruct'
/usr/lib64/gcc/x86_64-suse-linux/10/../../../../x86_64-suse-linux/bin/ld: /tmp/Arcus.so.i06lpg.ltrans0.ltrans.o: in function `meth_SocketListener_error':
/usr/include/python3.8/object.h:459: undefined reference to `_Py_NoneStruct'
/usr/lib64/gcc/x86_64-suse-linux/10/../../../../x86_64-suse-linux/bin/ld: /tmp/Arcus.so.i06lpg.ltrans0.ltrans.o: in function `meth_Socket_getState':
/data/3D/libArcus/build/python/sipArcuspart0.cpp:317: undefined reference to `PyEval_SaveThread'
/usr/lib64/gcc/x86_64-suse-linux/10/../../../../x86_64-suse-linux/bin/ld: /data/3D/libArcus/build/python/sipArcuspart0.cpp:319: undefined reference to `PyEval_RestoreThread'
/usr/lib64/gcc/x86_64-suse-linux/10/../../../../x86_64-suse-linux/bin/ld: /tmp/Arcus.so.i06lpg.ltrans0.ltrans.o: in function `meth_Socket_connect':
/data/3D/libArcus/build/python/sipArcuspart0.cpp:458: undefined reference to `PyEval_SaveThread'
/usr/lib64/gcc/x86_64-suse-linux/10/../../../../x86_64-suse-linux/bin/ld: /data/3D/libArcus/build/python/sipArcuspart0.cpp:460: undefined reference to `PyEval_RestoreThread'
/usr/lib64/gcc/x86_64-suse-linux/10/../../../../x86_64-suse-linux/bin/ld: /tmp/Arcus.so.i06lpg.ltrans0.ltrans.o: in function `meth_Socket_connect':
/usr/include/python3.8/object.h:459: undefined reference to `_Py_NoneStruct'

[cut]

@hroncok
Copy link
Contributor

hroncok commented Jul 27, 2020

Breaks how?

@krop
Copy link
Contributor

krop commented Jul 27, 2020

The comment above was edited.

@hroncok
Copy link
Contributor

hroncok commented Jul 28, 2020

Well it does "break" it indeed, but only because you say "every reference needs to be defined" which is not true for Python extension modules. What is the purpose for using -Wl,--no-undefined?

@Ghostkeeper
Copy link
Contributor

Heh, gave me quite a headache but found an issue with this code. I made an adjustment here to allow our build server to run it again: b6c89c7

See the commit message for the reasoning. We had temporarily reverted these changes a week ago because of that build issue. It should be fixed more properly now (build pending). Please see if it still works for you as well.

StefanBruens added a commit to StefanBruens/libArcus that referenced this pull request Oct 25, 2020
Currently the build uses the CMAKE_SHARED_LINKER_FLAGS for both
the libArcus shared library as well as the Python module. Starting with
Python 3.8 the Python module is no longer linked to libpython, and thus
some symbols stay undefined.

The correct build type for python modules is "MODULE", which uses the
distinct CMAKE_MODULE_LINKER_FLAGS. This allows to keep the
'-Wl,--no-undefined' linker flag for libArcus.

Ultimaker#109
@StefanBruens
Copy link
Contributor

Well it does "break" it indeed, but only because you say "every reference needs to be defined" which is not true for Python extension modules. What is the purpose for using -Wl,--no-undefined?

It is the correct flag for libArcus.so - you don't want undefined symbols in a regular shared library. Using it turns a potential runtime error into a build time error.

For the python module Arcus.so there will be undefined symbols. The fault lies with the add_sip_python_module macro, which uses the correct MODULE library type only on Cygwin or for APPLE, and uses SHARED otherwise. MODULE and SHARED have distinct CMAKE_{MODULE,SHARED}_LINKER_FLAGS, and using MODULE for the python module allows to keep --no-undefined for the regular (SHARED) library.

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.

5 participants