diff mbox series

[1/1] package/kmod: fix build with python 3.8

Message ID 20191020193818.85518-1-james.hilliard1@gmail.com
State Changes Requested
Headers show
Series [1/1] package/kmod: fix build with python 3.8 | expand

Commit Message

James Hilliard Oct. 20, 2019, 7:38 p.m. UTC
Fixes:
 - http://autobuild.buildroot.net/results/71bf937339705a520e047d12c146229ec42a76f2

Details:
 - https://bugs.python.org/issue36721

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 package/kmod/kmod.mk | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Thomas Petazzoni Oct. 21, 2019, 6:56 p.m. UTC | #1
On Sun, 20 Oct 2019 13:38:18 -0600
James Hilliard <james.hilliard1@gmail.com> wrote:

> Fixes:
>  - http://autobuild.buildroot.net/results/71bf937339705a520e047d12c146229ec42a76f2
> 
> Details:
>  - https://bugs.python.org/issue36721

Reading this, I am not sure your fix is correct (though I am not sure
what the correct fix is).

Indeed, in this bug report, Victor Stinner explains:

"""
There are two use cases for libpython:

* Build a C extension and load it in Python: use case called "pyext" by waf
* Embed Python into an application: use case called "pyembed" by waf
"""

The first case should not require -lpython3.8, and therefore
python-config no longer returns -lpython3.8, unless you pass --embed
(which means you are in the second case: you embed Python into an
application).

But kmod is only providing Python bindings: it's a C extension to be
loaded in Python, so we are in the first case, and we shouldn't need
--embed.

What do you think ?

Thomas
James Hilliard Oct. 21, 2019, 7:12 p.m. UTC | #2
On Mon, Oct 21, 2019 at 8:56 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Sun, 20 Oct 2019 13:38:18 -0600
> James Hilliard <james.hilliard1@gmail.com> wrote:
>
> > Fixes:
> >  - http://autobuild.buildroot.net/results/71bf937339705a520e047d12c146229ec42a76f2
> >
> > Details:
> >  - https://bugs.python.org/issue36721
>
> Reading this, I am not sure your fix is correct (though I am not sure
> what the correct fix is).
Yeah, not entirely sure but I saw other projects use this same fix effectively.
>
> Indeed, in this bug report, Victor Stinner explains:
>
> """
> There are two use cases for libpython:
>
> * Build a C extension and load it in Python: use case called "pyext" by waf
> * Embed Python into an application: use case called "pyembed" by waf
> """
>
> The first case should not require -lpython3.8, and therefore
> python-config no longer returns -lpython3.8, unless you pass --embed
> (which means you are in the second case: you embed Python into an
> application).
>
> But kmod is only providing Python bindings: it's a C extension to be
> loaded in Python, so we are in the first case, and we shouldn't need
> --embed.
>
> What do you think ?
Yeah, it's not very clear, this fix is the same as these essentially however:
https://gitlab.freedesktop.org/gstreamer/gst-python/merge_requests/14/diffs
https://gitlab.com/ita1024/waf/merge_requests/2236/diffs
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Thomas Petazzoni Oct. 21, 2019, 7:23 p.m. UTC | #3
On Mon, 21 Oct 2019 21:12:44 +0200
James Hilliard <james.hilliard1@gmail.com> wrote:

> > Reading this, I am not sure your fix is correct (though I am not sure
> > what the correct fix is).  
> Yeah, not entirely sure but I saw other projects use this same fix effectively.

Yes, it "works", but it's quite weird, because the whole point of
Victor Stinner's work was to make sure Python extensions were not
linked with libpython.

> > What do you think ?  
> Yeah, it's not very clear, this fix is the same as these essentially however:
> https://gitlab.freedesktop.org/gstreamer/gst-python/merge_requests/14/diffs

This is indeed a Python binding.

> https://gitlab.com/ita1024/waf/merge_requests/2236/diffs

This is a generic build system, and it only adds --embed when the
"pyembed" flag is used. It could simply be that this flag is used for
applications/libraries than embed Python, and not for Python
extensions. Indeed, the original entry in the Python issue tracker
mentioned that waf had "pyext" for Python extensions and "pyembed" for
apps/libs embedding Python.

It turns out that Victor Stinner (the author of the changes) is a
former colleague of mine from university times. I'll try to get some
feedback from him about this.

Best regards,

Thomas
Thomas Petazzoni Oct. 22, 2019, 9:02 a.m. UTC | #4
Hello,

On Sun, 20 Oct 2019 13:38:18 -0600
James Hilliard <james.hilliard1@gmail.com> wrote:

> +ifeq ($(BR2_PACKAGE_PYTHON3),y)
> +KMOD_CONF_ENV += \
> +	PYTHON_LIBS="`$(STAGING_DIR)/usr/bin/python3-config --libs --embed`"
> +endif

For kmod, the problem is the same as libselinux, just not the same
option is used. libselinux is passing "-z defs", while kmod is passing
--no-undefined, but they are exactly the same option.

So, for kmod, I believe the proper fix is:

diff --git a/Makefile.am b/Makefile.am
index c5c2f06..8e9c90d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -173,7 +173,7 @@ CPYTHON_MODULE_CFLAGS = \
        $(AM_CFLAGS) -DCPYTHON_COMPILING_IN_PYPY=0 \
        $(PYTHON_NOWARN) $(PYTHON_CFLAGS) \
        -fvisibility=default
-CPYTHON_MODULE_LDFLAGS = $(AM_LDFLAGS) -module -avoid-version -shared
+CPYTHON_MODULE_LDFLAGS = $(AM_LDFLAGS) -module -avoid-version -shared -Wl,-z,undefs
 
 if BUILD_PYTHON
 pkgpyexec_LTLIBRARIES = \

Indeed, kmod globally uses -Wl,--no-undefined when linking, but
specifically for building the Python extension, we want to disable that
and allow undefined symbols to exist, so we override it with
-Wl,-z,undefs.

Best regards,

Thomas Petazzoni
diff mbox series

Patch

diff --git a/package/kmod/kmod.mk b/package/kmod/kmod.mk
index a5bcf2f2d6..a63fe99d91 100644
--- a/package/kmod/kmod.mk
+++ b/package/kmod/kmod.mk
@@ -45,6 +45,10 @@  endif
 ifeq ($(BR2_PACKAGE_PYTHON)$(BR2_PACKAGE_PYTHON3),y)
 KMOD_DEPENDENCIES += $(if $(BR2_PACKAGE_PYTHON),python,python3)
 KMOD_CONF_OPTS += --enable-python
+ifeq ($(BR2_PACKAGE_PYTHON3),y)
+KMOD_CONF_ENV += \
+	PYTHON_LIBS="`$(STAGING_DIR)/usr/bin/python3-config --libs --embed`"
+endif
 endif
 
 ifeq ($(BR2_PACKAGE_KMOD_TOOLS),y)