search

Ultimaker / libArcus

Communication library between internal components for Ultimaker software
GNU Lesser General Public License v3.0
75 stars 81 forks source link

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

Closed gferon closed 4 years ago

gferon commented 4 years ago

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: https://github.com/python/cpython/pull/12946

Signed-off-by: Gabriel Féron feron.gabriel@gmail.com

Ghostkeeper commented 4 years ago

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 commented 4 years ago

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:

Not linking to libpython:

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

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

Ghostkeeper commented 4 years ago

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 :smiley: 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.

krop commented 4 years ago

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 commented 4 years ago

Breaks how?

krop commented 4 years ago

The comment above was edited.

hroncok commented 4 years ago

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 commented 4 years ago

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: https://github.com/Ultimaker/libArcus/commit/b6c89c7de172accd10c97eb81384320137b1be74

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 commented 4 years ago

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.