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

bpo-45847: Port _tkinter to PY_STDLIB_MOD #31698

Merged
merged 20 commits into from
Mar 31, 2022

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Mar 5, 2022

  • Remove --with-tclk-* options from configure
  • Use pkg-config to detect _tkinter dependencies (Tcl/Tk, X11)
  • Manual override via environment variables TCLTK_CFLAGS and TCLTK_LIBS

https://bugs.python.org/issue45847

Automerge-Triggered-By: GH:tiran

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Mar 6, 2022

Looks like the Ubuntu pkg-config files for Tcl/Tk are buggy; they don't provide the correct --libs flags (-L is wrong).

Resolved in commit a8313a1

Now we've got problems on FreeBSD (14.0): X11/Xlib.h is included by tk.h, X11 include path is not passed toCFLAGS:

$ grep -A5 "cc.*tcl" config.log
configure:12411: cc -o conftest  -I/usr/local/include/tcl8.6 -I/usr/local/include/tk8.6    conftest.c -L/usr/local/lib -ltk86 -ltkstub86 -ltcl86 -ltclstub86   >&5
In file included from conftest.c:111:
/usr/local/include/tk8.6/tk.h:99:13: fatal error: 'X11/Xlib.h' file not found
#   include <X11/Xlib.h>
            ^~~~~~~~~~~~

1 error generated.

$ pkg-config --libs --cflags tcl86 tk86
-I/usr/local/include/tcl8.6 -I/usr/local/include/tk8.6 -L/usr/local/lib -ltk86 -ltkstub86 -ltcl86 -ltclstub86

Proposed solution: add special case for FreeBSD in 72cf592

@erlend-aasland
Copy link
Contributor Author

(I squeezed 46e43dcaa599d3fe906cdb3ebd591151af856372, 5156b5764a368578e5eb55da471b74e80e5aa479, and 8af1b258d04b042038e0cc0e5fccfd35aad659af into 72cf592)

@erlend-aasland erlend-aasland changed the title [WIP] bpo-45847: Port _tkinter to PY_STDLIB_MOD bpo-45847: Port _tkinter to PY_STDLIB_MOD Mar 7, 2022
@erlend-aasland erlend-aasland reopened this Mar 7, 2022
configure.ac Outdated
Comment on lines 3540 to 3544
#if defined(TK_HEX_VERSION)
# if TK_HEX_VERSION < 0x08040200
# error "Tk older than 8.4.2 not supported"
# endif
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop this and keep the ones below?

@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 7, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit ea426ef 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 7, 2022
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Mar 7, 2022

Non-Windows buildbot status embedded below (updating this post at irregular intervals)

TL;DR: macOS and RHEL7 buildbots did no longer build _tkinter (change in behaviour).

There were some other issues which have been addressed:

  • A FreeBSD bot failed because the X11 check lacked a guard. Addressed in 5816799
Buildbot status
Buildbot Post PR Pre PR Status
AMD64 FreeBSD Shared PR error missing 🔴
AMD64 Arch Linux Asan Debug PR yes yes 🟢
AMD64 Arch Linux Asan PR yes yes 🟢
AMD64 Arch Linux TraceRefs PR yes  yes 🟢
AMD64 Arch Linux Usan PR yes  yes 🟢
AMD64 Debian PGO PR yes yes 🟢
AMD64 Debian root PR missing missing 🟢
AMD64 Fedora Stable Clang Installed PR yes  yes 🟢
AMD64 Fedora Stable Clang PR yes yes 🟢
AMD64 Fedora Stable LTO + PGO PR yes yes 🟢
AMD64 Fedora Stable LTO PR yes yes 🟢
AMD64 Fedora Stable PR yes yes 🟢
AMD64 Fedora Stable Refleaks PR yes  yes 🟢
AMD64 RHEL7 LTO + PGO PR missing yes 🔴
AMD64 RHEL7 LTO PR missing yes 🔴
AMD64 RHEL7 PR missing yes 🔴
AMD64 RHEL7 Refleaks PR missing yes 🔴
AMD64 RHEL8 FIPS Only Blake2 Builtin Hash PR yes yes 🟢
AMD64 RHEL8 LTO + PGO PR yes yes 🟢
AMD64 RHEL8 LTO PR yes yes 🟢
AMD64 RHEL8 PR yes yes 🟢
AMD64 RHEL8 Refleaks PR yes  yes 🟢
AMD64 Ubuntu Shared PR missing missing 🟢
ARM64 macOS PR missing (deliberate) yes 🟢
PPC64 Fedora PR missing yes 🔴
PPC64LE Fedora Stable Clang Installed PR yes yes 🟢
PPC64LE Fedora Stable Clang PR yes yes 🟢
PPC64LE Fedora Stable LTO + PGO PR yes yes 🟢
PPC64LE Fedora Stable LTO PR yes yes 🟢
PPC64LE Fedora Stable PR yes yes 🟢
PPC64LE Fedora Stable Refleaks PR yes yes 🟢
PPC64LE RHEL7 LTO + PGO PR missing yes 🔴
PPC64LE RHEL7 LTO PR missing yes 🔴
PPC64LE RHEL7 PR missing  yes 🔴
PPC64LE RHEL7 Refleaks PR missing yes 🔴
PPC64LE RHEL8 LTO + PGO PR yes yes 🟢
PPC64LE RHEL8 LTO PR yes  yes 🟢
PPC64LE RHEL8 PR yes yes 🟢
PPC64LE RHEL8 Refleaks PR yes yes 🟢
aarch64 Fedora Stable Clang Installed PR yes yes 🟢
aarch64 Fedora Stable Clang PR yes yes 🟢
aarch64 Fedora Stable LTO + PGO PR  yes yes 🟢
aarch64 Fedora Stable LTO PR yes yes 🟢
aarch64 Fedora Stable PR yes yes 🟢
aarch64 Fedora Stable Refleaks PR yes yes 🟢
aarch64 RHEL8 LTO + PGO PR yes yes 🟢
aarch64 RHEL8 LTO PR yes yes 🟢
aarch64 RHEL8 PR yes yes 🟢
aarch64 RHEL8 Refleaks PR yes yes 🟢
s390x Debian PR yes yes 🟢
s390x Fedora Clang Installed PR yes  yes 🟢
s390x Fedora Clang PR yes yes 🟢
s390x Fedora LTO + PGO PR yes yes 🟢
s390x Fedora LTO PR yes yes 🟢
s390x Fedora PR yes  yes 🟢
s390x Fedora Refleaks PR yes yes 🟢
s390x RHEL7 LTO + PGO PR missing yes 🔴
s390x RHEL7 LTO PR missing yes 🔴
s390x RHEL7 PR missing yes 🔴
s390x RHEL7 Refleaks PR missing  yes 🔴
s390x RHEL8 LTO + PGO PR yes  yes 🟢
s390x RHEL8 LTO PR yes yes 🟢
s390x RHEL8 PR yes  yes 🟢
s390x RHEL8 Refleaks PR yes yes 🟢
s390x SLES PR  yes  yes 🟢
x86 Gentoo Installed with X PR yes  yes 🟢
x86 Gentoo Non-Debug with X PR yes  yes 🟢
x86-64 macOS PR missing (deliberate)  yes 🟢

@erlend-aasland
Copy link
Contributor Author

Sparked by Ronald's comment on Discourse:

We could add a hack for macOS... I added this on my macOS 12 install, and it works for me. Not sure how it would fare on older installs, though.

--- a/configure.ac
+++ b/configure.ac
@@ -3515,8 +3515,21 @@ for _QUERY in \
 done
 
 AS_VAR_IF([found_tcltk], [no], [
-  TCLTK_CFLAGS=${TCLTK_CFLAGS-""}
-  TCLTK_LIBS=${TCLTK_LIBS-""}
+  AS_CASE([$ac_sys_system],
+    [Darwin*], [
+      TCLTK_CFLAGS=${TCLTK_CFLAGS-"\
+        -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Tcl.framework/Headers \
+        -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Tk.framework/Headers \
+        -F/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks \
+      "}
+      TCLTK_LIBS=${TCLTK_LIBS-"\
+        -Wl,-F,/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks,-framework,Tcl,-framework,Tk
+      "}
+    ], [
+      TCLTK_CFLAGS=${TCLTK_CFLAGS-""}
+      TCLTK_LIBS=${TCLTK_LIBS-""}
+    ]
+  )
 ])
 
 dnl FreeBSD has an X11 dependency which is not implicitly resolved.

I'm not sure if it's worth it adding a more complex hack, or if it's worth it adding this hack in the first place.

cc. @tiran @ronaldoussoren @ned-deily

@erlend-aasland
Copy link
Contributor Author

We could add a hack for macOS... I added this on my macOS 12 install, and it works for me. Not sure how it would fare on older installs, though.

Based on comments from Ned and Ronald, I'm dropping adding any hack for macOS; those building on macOS will have to install MacPorts or Homebrew, and explicitly or implicitly pass this information to configure, for example through PKG_CONFIG_PATH.

@tiran tiran added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 13, 2022
@erlend-aasland
Copy link
Contributor Author

Synced with main to get Serhiy's commits (GH-31839, GH-31938)

@erlend-aasland
Copy link
Contributor Author

@tiran, do you want another round through the buildbots after I pulled Serhiy's changes? Apart from that, are you fine with this PR as it stands?

@ned-deily
Copy link
Member

ned-deily commented Mar 17, 2022

Thanks for the PR! I'd like to review and play with it a bit but I likely won't be able to do so before Monday. If you don't hear anything by then, go ahead.

@erlend-aasland
Copy link
Contributor Author

@ned-deily would you still like to review this?

Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for doing this. Considering the history and complexity, I'm sure some problems will pop up that, at a minimum, will require more documentation, including the devguide with regard to installing dependencies. I did leave a comment about explicitly documenting the removal of the previous configure options. Also FTR, the macOS installer build script at the moment still depends on those current configure options; I will rework that area prior to the next 3.11 alpha.

Doc/whatsnew/3.11.rst Show resolved Hide resolved
@ned-deily
Copy link
Member

Ready to go

@erlend-aasland
Copy link
Contributor Author

Ready to go

Great, thank you for reviewing! @tiran, can you land this? :)

@tiran
Copy link
Member

tiran commented Mar 31, 2022

Love it! Your PR removes a lot of complicated code from setup.py.

@miss-islington miss-islington merged commit b36d222 into python:main Mar 31, 2022
@erlend-aasland erlend-aasland deleted the ac-tkinter branch March 31, 2022 10:35
erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Apr 17, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 9, 2022
(cherry picked from commit 269e726)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@protonmail.com>
erlend-aasland added a commit that referenced this pull request May 9, 2022
miss-islington added a commit that referenced this pull request May 9, 2022
(cherry picked from commit 269e726)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@protonmail.com>
@sharewax
Copy link

files from centos7 (tk-devel-8.5.13-6.el7.x86_64 and tcl-devel-8.5.13-8.el7.x86_64) doesn't contain files for pkg-config, so tcl for debug version of python you get

Wl,--enable-new-dtags -g -L/usr/lib64/openssl11 -L/opt/wargaming/wargaming-python311/root/usr/lib64-Wl,-z,relro -Wl,-rpath,/opt/wargaming/wargaming
-python311/root/usr/lib64 -Wl,--enable-new-dtags -g -L/usr/lib64/openssl11 -L/opt/wargaming/wargaming-python311/root/usr/lib64-Wl,-z,relro -Wl,-rpa
th,/opt/wargaming/wargaming-python311/root/usr/lib64 -Wl,--enable-new-dtags -fno-semantic-interposition -g -L/usr/lib64/openssl11 -flto -fuse-linke
r-plugin -ffat-lto-objects -flto-partition=none -g -L/opt/wargaming/wargaming-python311/root/usr/lib64-Wl,-z,relro -Wl,-rpath,/opt/wargaming/wargam
ing-python311/root/usr/lib64 -Wl,--enable-new-dtags -fno-semantic-interposition -g -L/usr/lib64/openssl11 -L/opt/wargaming/wargaming-python311/root
/usr/lib64-Wl,-z,relro -Wl,-rpath,/opt/wargaming/wargaming-python311/root/usr/lib64 -Wl,--enable-new-dtags -g -L/usr/lib64/openssl11 -I/opt/wargami
ng/wargaming-python311/root/usr/include -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4
-grecord-gcc-switches -m64 -mtune=generic -D_GNU_SOURCE -fPIC -fwrapv -I/usr/include/openssl11 build/temp.linux-x86_64-3.11-pydebug/builddir/build/
BUILD/Python-3.11.0rc1/Modules/_ctypes/_ctypes.o build/temp.linux-x86_64-3.11-pydebug/builddir/build/BUILD/Python-3.11.0rc1/Modules/_ctypes/callbac
ks.o build/temp.linux-x86_64-3.11-pydebug/builddir/build/BUILD/Python-3.11.0rc1/Modules/_ctypes/callproc.o build/temp.linux-x86_64-3.11-pydebug/bui
lddir/build/BUILD/Python-3.11.0rc1/Modules/_ctypes/cfield.o build/temp.linux-x86_64-3.11-pydebug/builddir/build/BUILD/Python-3.11.0rc1/Modules/_cty
pes/stgdict.o -L. -L/opt/wargaming/wargaming-python311/root/usr/lib64 -L/usr/lib64/openssl11 -L/usr/local/lib -lffi -ldl -o build/lib.linux-x86_64-
3.11-pydebug/_ctypes.cpython-311d-x86_64-linux-gnu.so
*** WARNING: renaming "_tkinter" since importing it failed: /builddir/build/BUILD/Python-3.11.0rc1/build/debug/build/lib.linux-x86_64-3.11-pydebug/_tkinter.cpython-311d-x86_64-linux-gnu.so: undefined symbol: Tcl_AddErrorInfo
Following modules built successfully but were removed because they could not be imported:
_tkinter

I'll try to set manually flags TCLTK_CFLAGS and TCLTK_LIBS, but it seems strage. on python3.10 all works well without additional things.

@tiran
Copy link
Member

tiran commented Aug 13, 2022

@sharewax Python 3.11 requires pkg-config to detect and configure tkinter. The CentOS 7 packages do not provide a .pc file for TCL and TK. _tkinter compiles for me with correct TCLTK_LIBS. We should document the correct flags for CentOS7 users. Could you please open a new ticket?

@sharewax
Copy link

sharewax commented Aug 13, 2022

@tiran Sure. Can you provide correct TCLTK_LIBS for centos7? I would like to finish my installation correctly :)
added:
export TCLTK_LIBS="-ltk8.5 -ltcl8.5"
solves the issue if add in into spec file near other export variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants