-
-
Notifications
You must be signed in to change notification settings - Fork 211
Build libtcl, libtk, and _tkinter as shared objects, and remove Tix #676
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
Conversation
5adbe11
to
abe2559
Compare
984efb4
to
27eeadb
Compare
2927f0d
to
046fa63
Compare
I think this is ready to review. Seems to work in mild manual testing on macOS and glibc. The formatting check and the failing static musl builds should be quick to fix, I'll do that tomorrow. I want to do a little more extensive testing, including on a dynamic musl distribution, but I expect to be able to undraft this tomorrow hopefully. Note this is split up into commits and might be easier to review commit-by-commit. |
This allows the shared libraries to statically link libz.a. I think we were previously picking up the zlib headers from the OS, and we were previously building static libraries so that was okay - they got linked into CPython itself which had the zlib symbols. But now that we're building shared libraries, they're picking up a dependency on libz.so.1, presumably because we don't have a static zlib installed from the OS.
This should suppress a mismatch leading to an xcb assertion failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title should be updated to indicate this removes Tix too.
In general, if libX.so depends on libY.a, we don't want libY's symbols dynamically exported by libX. We want libX's use of libY to be private. We use two mechanisms for making this happen. -fvisibility=hidden, which works on the build of libY to mark the symbols themselves as hidden in the object file, and -Wl,--exclude-libs=ALL, which works on the build of libX to instruct the linker not to re-export symbols from static libraries. We set -fvisibility=hidden on some platforms but not all. We use -Wl,--exclude-libs=ALL on the build of Python itself on all platforms, which up to this point built our only dynamic objects (bin/python and libpython). Now that libtcl and libtk are dynamic, we need to do the same thing. I'm not sure if -fvisibility=hidden actually does anything useful for us, given the use of -Wl,--exclude-libs=ALL, and I think the fact that it's only set on some platforms is an oversight and demonstrates that it's not needed. See astral-sh#735, which removes it; the builds pass vaildation.
I think I was thinking of doing rebase-and-merge, which would preserve the separate commit removing it and linking to #723, but I can update the title and squash-and-merge. |
Rebase-merge is fine. It seems nice to update the title regardless, but I don't have strong feelings. |
Several important third-party packages, including matplotlib in its
tkagg backend and Pillow, use tkinter as a way of locating libtcl and
libtk and making direct C API calls to those libraries. For more
details, see the analysis in
#129 (comment)
To make these packages work, we need to expose the full libtcl and
libtk dynamic symbol ABI; we can't just statically link them into our
own binary. It seems most robust to also expose these as separate
libraries under their usual filenames to match the behavior of other
Python distributions.
Build shared libraries for the _tkinter module and for libtcl and libtk,
and set up rpaths so we find our copies of them. libX11 continues to be
statically linked, but it's linked into libtk. Just as with the build of
Python itself, use --exclude-libs=ALL to prevent the dependencies'
symbols from being exported.
Stop building Tix because it's broken (#723) and it would need to be
changed to dynamic linking.
Configure libX11 with --disable-loadable-xcursor to fix #146, which I
ran into while running tests.
Add zlib as a build-dep of Tcl/Tk so that they can statically link
libz.a. I think we were previously picking up the zlib headers from the
OS, which wasn't a problem when libtcl and libtk were static libraries -
they got linked into CPython itself which also linked zlib.a. But now
libtcl.so and libtk.so need zlib.a.
Fixes #129
Fixes #533