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

gh-71383: add upstream fix #29

Merged
merged 1 commit into from Dec 7, 2023
Merged

gh-71383: add upstream fix #29

merged 1 commit into from Dec 7, 2023

Conversation

chrstphrchvz
Copy link

@chrstphrchvz chrstphrchvz commented Sep 2, 2023

@chrstphrchvz
Copy link
Author

Note: I have not attempted to build the Windows installer myself.

@chrstphrchvz
Copy link
Author

@ned-deily @zware Not sure who I should ping, but is there interest in getting this included in 3.11.7/3.12.1?

@ned-deily
Copy link
Member

Also @zooba

@terryjreedy
Copy link
Member

@serhiy-storchaka This 8-line upstream patch to 3 tk files is supposed to eliminate Theme-changed warnings, which are a nuisance when testing. I can't review it but would like to see it merged.

@zooba
Copy link
Member

zooba commented Dec 4, 2023

It's missed 3.11.7 already.

When's the next expected release of Tk? We'd obviously prefer to take an actual release, and generally only consider backporting fixes for blocking issues. (If the warnings are causing tests to fail in CI/buildbots, that would be enough IMHO.)

@chrstphrchvz
Copy link
Author

To be clear, the issue this fixes is an error/exception, not a warning; the Tcl language does not have warnings. The issue was encountered not just by tests, but also by a number of users who then try to work around it because they think their program is causing it.

The past several Tcl/Tk 8.6 releases were around this time of year, but I am not aware of an imminent 8.6.14 release. Several in the inner Tcl/Tk circle are quite vocal that they only want Tcl/Tk 9.0 and do not care about 8.7 or even 8.6. And so the only imminent release I am aware of is Tcl/Tk 9.0 beta 1, reportedly to be released this month.

@zooba
Copy link
Member

zooba commented Dec 7, 2023

@chrstphrchvz Are you prepared to come back and add more fixes if this turns out to regress other scenarios? (The "I tend to disappear" in your status message isn't very encouraging 😉 )

We don't have any Tk maintainers on our team, which means if we make changes to our build of it, we have no way to diagnose the problems it may cause. I'm willing to merge your PR and generate new builds, but I'm not willing to take responsibility for following up if it breaks anything at all - we'll simply revert our builds to use the prior version with the issue.

@chrstphrchvz
Copy link
Author

I accept that the change will be reverted if it is believed to have introduced a regression.

@chrstphrchvz
Copy link
Author

…A regression due to this change would likely be an upstream issue. In the event that this happens, I cannot say how quickly I can report it upstream, propose a possible alternative fix if no one else does, or propose backporting the response accepted upstream to Python.

@zooba
Copy link
Member

zooba commented Dec 7, 2023

It wouldn't necessarily be an upstream issue - it could simply be due to us cherry-picking one change, but upstream already including other fixes that have an impact.

Based on what you say, I'm not prepared to risk the regression in 3.12.2 or 3.11.8. I'm okay with taking it for 3.13, as that has more time for testing. If it's looking stable enough by the time 3.13 is getting wide use (late beta), then I'd be willing to consider backporting to 3.12 and (if it's not in security-fix mode) 3.11. You may find another contributor who's less concerned than I am and convince them to backport it sooner - I can't stop that from happening.

macOS is a totally separate question. @ned-deily will have to handle that.

@zooba zooba merged commit a7c4781 into python:tk Dec 7, 2023
@zooba
Copy link
Member

zooba commented Dec 7, 2023

I've merged the fix into our source mirror, but will figure out the build and core updates tomorrow (or next week, if I don't get to it). Currently, there'll be no impact on any CPython builds.

@ned-deily
Copy link
Member

macOS is a totally separate question. @ned-deily will have to handle that.

FWIW, I've already merged @chrstphrchvz's similar patch (and a few others) for the Tk used in the macOS installer and it's in 3.11.7 and now 3.12.1.

@chrstphrchvz
Copy link
Author

The fix was also backported to Homebrew and MacPorts.

I still cannot objectively say that the risk of regressions is low enough such that Tkinter is better off with this fix than without it, because I am biased in favor of saying so. I provided the analysis for this bug and coauthored the upstream fix. I argue that the fix is equally applicable to past Tk releases, and predict that it does not have any possible runtime regressions.

@zooba
Copy link
Member

zooba commented Dec 8, 2023

I'm less concerned about the risk of regressions, and more concerned that someone is around to deal with them if/when they happen (or when someone raises a new issue and we have to figure out whether it's a regression or not).

Right now, yours (@chrstphrchvz) is the handle I'll reach for, and if you've made a good fix then I won't ever need to 😉

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