Skip to content

_tkinter pt. 2 #5640

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

Merged
merged 4 commits into from
Apr 6, 2025
Merged

_tkinter pt. 2 #5640

merged 4 commits into from
Apr 6, 2025

Conversation

arihant2math
Copy link
Collaborator

@arihant2math arihant2math commented Mar 30, 2025

Switches to custom bindgen crates and does some implementation work (GetVar is still broken however).

Also ... building on macos/linux is broken, but that can be fixed on those crates macos/linux require pkg-config now.

@youknowone
Copy link
Member

The existing tk/tcl libraries seem to use inwelling to generate bindings https://github.com/oooutlk/tcltk/blob/main/tcl/src/lib.rs#L150-L159

If we switch to bindings, how do you think we go similar with those approach to avoid common pitfalls and find a shared point with them in future?

@youknowone youknowone requested a review from coolreader18 March 31, 2025 02:35
@arihant2math
Copy link
Collaborator Author

arihant2math commented Mar 31, 2025

The existing tk/tcl libraries seem to use inwelling to generate bindings https://github.com/oooutlk/tcltk/blob/main/tcl/src/lib.rs#L150-L159

I took a look a while back, before writing custom crates. It seems inwelling is simply a manner for upstream crates to send information downstream. clib does the building and is just a thin wrapper around bindgen (https://docs.rs/clib/latest/clib/, and https://docs.rs/crate/clib/latest/source/build.rs).

Currently I have a few problems with this approach.

  1. clib is used as a singular dependency to compile anything, and is depended on twice just by pulling in the tk crate (once for tcl, and the second time for tk). I'm not sure how well cargo handles this, seeing they both call clib with the same version etc.
  2. I don't have raw API access, which I seem to need for much of this.
  3. tk as well as other bindings I have considered all have a bug where Tcl_DecrRefCount and Tcl_IncrRefCount are not handled properly due to a bindgen limitation in terms of function macros.

I blacklist these macros here: https://github.com/arihant2math/tkinter/blob/d9874a8446cd32dfcd3e71b8e891329f304d2a3b/tk-sys/build.rs#L113

and re-implement them here: https://github.com/arihant2math/tkinter/blob/2957564c74fbb4f25b967f6ac1e2289ed04fd0c0/tk-sys/src/lib.rs#L7

Seeing that they crash the linker if included, unless patched, ... I doubt these libraries are handling memory management well.
Seeing that the right Free function isn't called either: https://github.com/search?q=repo%3Aoooutlk%2Ftcltk%20Free&type=code

If we switch to bindings, how do you think we go similar with those approach to avoid common pitfalls and find a shared point with them in future?

I think with bindings we will be able to mirror the cpython implementation well enough that most issues should be avoided. I think the current set of tk libraries aren't ready for something as broad as tkinter.

@arihant2math
Copy link
Collaborator Author

Also I would ideally have contributed to https://github.com/MisterEggnog/tcltk-rs, but it seems to be unmaintained and the author seems to be inactive.

@arihant2math arihant2math changed the title tkinter pt. 2 _tkinter pt. 2 Mar 31, 2025
@youknowone
Copy link
Member

Thank you so much for explanation

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

The changes look good.
Are you going to individually maintain tk bindings? Otherwise could you consider to transfer the repository to RustPython org?

@arihant2math
Copy link
Collaborator Author

I don't have too much of a use for them, so I'll send a transfer request after I clean it up and add basic CI.

@arihant2math
Copy link
Collaborator Author

Also since I'm not an admin of the RustPython org, I'd have to transfer it to you first, and you can transfer it to the org.

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

Thank you for this headache driving work

@youknowone youknowone merged commit be56911 into RustPython:main Apr 6, 2025
11 checks passed
Shakil582 pushed a commit to Shakil582/RustPython that referenced this pull request Apr 6, 2025
@arihant2math arihant2math deleted the tkinter-pt2 branch April 14, 2025 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants