Re: [Buildroot] [PATCH] package/python3: add tk as core module

["Yann E. MORIN" <yann.morin.1998@free.fr>]

John, All,

I see that Thomas already sent a review, and you had further questions,
so let me try and address them here, as a review of your patch; it's
going to be basically the same comments as Thomas already provided, but
I'll try to expand on those.

Also, please do not top-post; instead, reply in-line

On 2023-12-20 08:48 -0500, J. Langholz spake thusly:
> ---

First, your commit needs a commit log; here it was empty. A commit log
explains what the situation is, why it needs to change, and how it is
changed. The "how" part is not a description of the patch; it is an
explanation. It can all be very brief in the most basic cases, but here
you may have to explain why the Tk module in python is important.

Then, your commit will have to be signed-off. The sign-off is described
in the manual:
    https://buildroot.org/downloads/manual/manual.html#submitting-patches

Quoting:

    Finally, the patch should be signed off. This is done by adding
    Signed-off-by: Your Real Name <your@email.address> at the end of the
    commit message. [...] See the Developer Certificate of Origin [0]
    for details.

    [0] http://developercertificate.org/

>  package/python3/Config.in  | 6 ++++++
>  package/python3/python3.mk | 9 +++++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/package/python3/Config.in b/package/python3/Config.in
> index 38f0580aa4..65afd4e6e2 100644
> --- a/package/python3/Config.in
> +++ b/package/python3/Config.in
> @@ -108,6 +108,12 @@ config BR2_PACKAGE_PYTHON3_SQLITE
>  	help
>  	  SQLite database support
>  
> +config BR2_PACKAGE_PYTHON3_TK
> +	bool "tk module"
> +	select BR2_PACKAGE_TK
> +	help
> +	  tk (a.k.a. tkinter) module support

This new option is in the section dedicated to the python that will run
on the target. That is, it does not impact the python interpreter used
at build time.

Since you "select TK", you have to reproduce its dependencies. In this
case, all but one of the dependencies for TK are already dependencies
of Python3, and the probability python loses any of those dependencies
is extremely low, so we can just assume that will never happen, so we
don't need to expressly propagate them.

Except for the dependency on Xorg. That one you must propagate, because
Kconfig does not do it automatically. That then needs a little comment
to inform the user about the missing dependency.

So, ultimately, this new option may look something like:

    config BR2_PACKAGE_PYTHON3_TK
        bool "tk module"
        # All other tk dependencies already python dependencies
        depends on BR2_PACKAGE_XORG7 # tk
        select BR2_PACKAGE_TK
        help
          tk (a.k.a. tkinter) module support

    comment "Tk module needs Xorg"
        depends on !BR2_PACKAGE_XORG7

>  config BR2_PACKAGE_PYTHON3_PYEXPAT
>  	bool "xml module"
>  	select BR2_PACKAGE_EXPAT
> diff --git a/package/python3/python3.mk b/package/python3/python3.mk
> index b9c5054a21..ca797f8cda 100644
> --- a/package/python3/python3.mk
> +++ b/package/python3/python3.mk
> @@ -20,7 +20,6 @@ HOST_PYTHON3_CONF_OPTS += \
>  	--without-ensurepip \
>  	--without-cxx-main \
>  	--disable-sqlite3 \
> -	--disable-tk \

[1] This part of the change affects the python interpreter that is built
for the build machine, not the one that runs on the target, i.e. not the
one the option defined above applies to.

See later for a further discussion on that matter.

>  	--with-expat=system \
>  	--disable-curses \
>  	--disable-codecs-cjk \
> @@ -114,6 +113,13 @@ else
>  PYTHON3_CONF_OPTS += --disable-openssl
>  endif
>  
> +ifeq ($(BR2_PACKAGE_PYTHON3_TK),y)
> +PYTHON3_DEPENDENCIES += tk
> +PYTHON3_CONF_OPTS += --enable-tk
> +else
> +PYTHON3_CONF_OPTS += --disable-tk
> +endif

This change, and...

>  ifneq ($(BR2_PACKAGE_PYTHON3_CODECSCJK),y)
>  PYTHON3_CONF_OPTS += --disable-codecs-cjk
>  endif
> @@ -184,7 +190,6 @@ PYTHON3_CONF_OPTS += \
>  	--with-system-ffi \
>  	--disable-pydoc \
>  	--disable-test-modules \
> -	--disable-tk \

... this change, do apply to the target python, so they are correct.

So, if all you wanted was the Tk module on the target, what you did was
almost correct, except for the part [1], and for a non-empty commit log.
In this case, just fix those little nits, and respin a v2 of your patch.

Now, if you (also?) wanted the Tk module in the host python interpreter,
this is going to be a bit more involved. In this case, you'll hit a few
more issues.

First, we do not have ahost variant of Tk, so you would first need to
introduce it. We do have a variant of tcl, though, but you'll have to
see if it is fit (I noticed host-tcl has no dependency on host-zlib;
that's probably an oversight, and may also need to be fixed).

Then, Tk depends on xlib_libXft, so host-tk would probably also need to
depend on host-xlib_libXft, but we do not have a host variant of it, so
you'd need to introduce that as well. And that in turn, has a lot of
dependencies: fontconfig, freetype, xlib_libX11, xlib_libXext,
xlib_libXrender, and xorgproto, and you'd need you have a host variant
for each of them and so on, recursively...

Sine this is going to be a big stack of dependencies, we do not want to
enable it systematically, so we'd need an option to enable Tk, in a
similar manner as for the target variant, something like the following,
in package/python3/Config.in.host:

    config BR2_PACKAGE_HOST_PYTHON3_SSL
        bool "tk"
        help
          Tk module for host python3.

and in package/python3/python3.mk:

    ifeq ($(BR2_PACKAGE_HOST_PYTHON3_TK),y)
    HOST_PYTHON3_DEPENDENCIES += host-tk
    HOST_PYTHON3_CONF_OPTS += --enable-tk
    else
    HOST_PYTHON3_CONF_OPTS += --disable-tk
    endif

I am not sure what you explained in your other reply to Thomas, but if
the underlying explanation was "that will rely on the Tk library as
provided on the build machine distribution", then this is not going to
be OK, because we don't require that to be available. So, all the
dependencies will have to be available in Buildroot if you want to add
the Tk module in the host python interpreter.

And finally, if you need to have the Tk module in the host python
interpreter, you'll have to provide a commit log that explains the
use-case.

As Thomas noted: do we need the host python to have Tk support, so that
we can build the target python with tk support?

So, if you don't need the Tk module on the host python interpreter, then
do not add it just for the sake of adding it.

I hope that the above will answer your questions on Thomas' reply.

Do not hesitate to reply further if that was not clear enough.

In the meantime, I'll mark your patch as "changes requested" in
patchwork.

Regards,
Yann E. MORIN.

>  	--disable-nis \
>  	--disable-idle3 \
>  	--disable-pyc-build
> -- 
> 2.34.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot