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-124111 Update tkinter to use TclTk 8.6.15 or 9.0.0 #124156

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

culler
Copy link
Contributor

@culler culler commented Sep 17, 2024

This PR updates tkinter, ttk, test.test_tkinter and test.test_ttk to make them work and pass tests when built against either TclTk 8.6.15, which was released today, or TclTk 9.0.0 which currently has a release candidate and will be released soon. The changes to the tests include some re-organization to make it easier to accommodate expected development of Tk 9, particularly with respect to the new TclObject of type "pixels" used in Tk 9 for options whose values represent screen distances.

Please also see the discussion on PR #124112.

Note that, unlike #124112, this PR targets Python 3.14.

@bedevere-app
Copy link

bedevere-app bot commented Sep 17, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link

bedevere-app bot commented Sep 17, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@culler culler requested a review from a team as a code owner September 19, 2024 16:11
Copy link
Member

@zooba zooba left a comment

Choose a reason for hiding this comment

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

Do you know why your generated files look like they're completely changed? It is line endings being bad? (I believe our regen scripts assume you're using autocrlf on Windows.)

PCbuild/tcltk.props Outdated Show resolved Hide resolved
PCbuild/tcltk.props Outdated Show resolved Hide resolved
@culler
Copy link
Contributor Author

culler commented Sep 19, 2024

Do you know why your generated files look like they're completely changed? It is line endings being bad? (I believe our regen scripts assume you're using autocrlf on Windows.)

I have no idea. I committed those by mistake, since I am used to using git commit -a. It has something to do with Windows - those files were not changed by other platforms.

I think I have now restored the autogenerated files that I committed by mistake.

Copy link
Member

@zware zware left a comment

Choose a reason for hiding this comment

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

Looking good! I have some comments that are mostly style nits, and I think we may want to hold off on the changes to PCbuild/. I don't think we'll be able to move the Windows builds for <=3.13 to Tcl/Tk 9 (at least not immediately), so it will make backporting the other changes easier to leave the 3.14 PCbuild/ updates to a separate PR.

Thanks!

Lib/test/test_tkinter/test_misc.py Outdated Show resolved Hide resolved
Lib/test/test_tkinter/test_widgets.py Outdated Show resolved Hide resolved
Lib/test/test_tkinter/widget_tests.py Outdated Show resolved Hide resolved
Lib/test/test_ttk/test_widgets.py Outdated Show resolved Hide resolved
@culler
Copy link
Contributor Author

culler commented Sep 20, 2024

I think I have now addressed all issues raised in the comments. Is there anything else that is needed?

@culler culler requested a review from zware September 20, 2024 16:38
@zooba
Copy link
Member

zooba commented Sep 23, 2024

We don't want example files in the build directory. To "save" the changes for later, they could either be a draft PR, comments in the file (not preferred), or you could add the changes with Condition="$(TclMajorVersion) == '9'" checks so that they automatically apply when building against 9.0 (preferred).1

Footnotes

  1. Number comparisons are a bit wonky in MSBuild, so usually just best to pick the most sensible default. In this case, it's probably actually to check $(TclMajorVersion) == '8' for the old behaviour, so that when 10.0 comes out it'll get the newer behaviours.

@culler
Copy link
Contributor Author

culler commented Sep 23, 2024

We don't want example files in the build directory. To "save" the changes for later, they could either be a draft PR, comments in the file (not preferred), or you could add the changes with Condition="$(TclMajorVersion) == '9'" checks so that they automatically apply when building against 9.0 (preferred).1

Footnotes

1. Number comparisons are a bit wonky in MSBuild, so usually just best to pick the most sensible default. In this case, it's probably actually to check `$(TclMajorVersion) == '8'` for the old behaviour, so that when 10.0 comes out it'll get the newer behaviours. [↩](#user-content-fnref-1-9d9cf4f3af8602fa729bad031ef1ba2c)

Sure. I will attempt to modify _tkinter.vcxproj to allow building against either 8.6.14 or 9.0.0. (Note that 8.6.15 has been released, but the binaries are not available to get_externals.bat yet.)

@culler
Copy link
Contributor Author

culler commented Sep 23, 2024

I have removed the "examples" and modified the build scripts to use the TclVersion environment variable to decide which Tk version gets used for the build. (The version must also be installed in ../externals/tcltk-W.X.Y.Z, which can either be done locally or by the get_externals script. I tested on my machine and was able to build both for Tk 8.6.14 (using the binaries downloaded by get_externals) and for Tk 9.0.0.0 (installed by me, by setting the INSTALLDIR variable in a standaard build of Tcl and Tk with nmake and makefile.vc).

Copy link
Member

@zware zware left a comment

Choose a reason for hiding this comment

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

Some of these changes cause failures for me on Pop_OS! 22.04 (Ubuntu 22.04 derivative) with Tcl/Tk 9.0.0b3. It's possible that I'm getting my configuration wrong, but switching to Tcl/Tk 9.0.0b3 without this PR causes one failure, then adding these changes introduces a bunch of other failures, mostly around rounding as far as I can tell.

add_configure_tests,
AbstractWidgetTest,
StandardOptionsTests,
IntegerSizeTests, PixelSizeTests)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: let's not stop here :)

Suggested change
IntegerSizeTests, PixelSizeTests)
IntegerSizeTests,
PixelSizeTests,
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Lib/test/test_ttk/test_widgets.py Outdated Show resolved Hide resolved
Comment on lines 359 to 362
if tk_version < (9,0):
errmsg = 'font "" doesn\'t exist'
else:
errmsg = 'font "" does not exist'
Copy link
Member

Choose a reason for hiding this comment

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

Nit: checkInvalidParam uses assertRaisesRegex, so we should be able to simplify:

Suggested change
if tk_version < (9,0):
errmsg = 'font "" doesn\'t exist'
else:
errmsg = 'font "" does not exist'
errmsg = 'font "" does ?n[o\']t exist'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Lib/test/test_ttk/test_widgets.py Show resolved Hide resolved
@culler
Copy link
Contributor Author

culler commented Sep 23, 2024

Some of these changes cause failures for me on Pop_OS! 22.04 (Ubuntu 22.04 derivative) with Tcl/Tk 9.0.0b3. It's possible that I'm getting my configuration wrong, but switching to Tcl/Tk 9.0.0b3 without this PR causes one failure, then adding these changes introduces a bunch of other failures, mostly around rounding as far as I can tell.

I am getting no failures on Ubuntu 22.04. I think that you are probably using the released 9.0.b3 while I am using the tips of main branches of the fossil repositories for Tcl and Tk. (I did that because they presumably are closer to the actual 9.0.0 releases.) There have been recent changes to how some of the widgets handle rounding.

I am using commits 8627aad9bd for Tcl and d2f53df7a8 for Tk. (Both from 9/23)

Note: part of what I am trying to do here is to make it easier to adjust the tests as these sorts of changes happen. I think the plan is that eventually all of these options will be rounded. Also more options will be changed to accept pixel values (e.g. 10p or 23m).

@zware
Copy link
Member

zware commented Sep 24, 2024

Ahh, that makes sense. It also makes me think we probably want to sit on this until 9.0 is actually released or has at least had a release candidate, though :-/

@culler
Copy link
Contributor Author

culler commented Sep 24, 2024

Ahh, that makes sense. It also makes me think we probably want to sit on this until 9.0 is actually released or has at least had a release candidate, though :-/

Sure. A release candidate is imminent. The core-9-0-0-rc branch is being finalized right now.

@culler
Copy link
Contributor Author

culler commented Sep 24, 2024

I switched to the tip of the core-9-0-0-rc branches of Tcl and Tk and reran the tkinter and ttk tests on Ubuntu 22.04, I still am seeing no errors.

Addendum 1: Also no errors on Windows 11 with the tips of the core-9-0-0-rc branches.

Addendum2: Also no errors on macOS 14 with the tips of the cre-9-0-0-rc branches.

Addendum3: The third Tcl 9.0.0.0 release candidate is now available here. I expect the final release will happen in the next week or two.

@culler
Copy link
Contributor Author

culler commented Sep 25, 2024

I think this PR is finished. The PR checks test against Tcl/Tk 8.6.14, and they are showing no failures. I have tested against Tcl/Tk 8.6.15 and Tcl/Tk 9.0.0 (rc2) on my own machines, running Ubuntu 22.04, Windows 11, and macOS 14. There are no failures in those 6 cases. The issue reported in #124378 with 8.6.15 has been resolved here.

The release of Tcl/Tk 9.0.0 is expected within a week, and surely will happen by the end of next week.

Copy link
Member

@zware zware left a comment

Choose a reason for hiding this comment

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

This is great, thank you! I'm satisfied with this change overall, but I think we should get a once-over from @serhiy-storchaka for the tkinter changes and a final confirmation from @zooba that the PCbuild/ changes match his expectations.

With your permission, I'd like to extract your ttk Notebook test fixes to a separate PR so that we can go ahead with it and get it backported. (If you would prefer to make that split yourself, that works for me.)

I expect that we will eventually be backporting this whole PR even if we don't start shipping 9.0 with our <3.14 binaries because at some point our non-Windows CI systems will inevitably update to 9.0, but I'm not sure how quickly that will happen and want to be sure we get 8.6.15 supported as soon as we can.

Lib/test/test_tkinter/test_misc.py Outdated Show resolved Hide resolved
@@ -112,6 +113,10 @@ def get_tk_patchlevel(root):
def pixels_conv(value):
return float(value[:-1]) * units[value[-1:]]

pix_re = re.compile(r'[0-9]*\.?[0-9]*[cimp]{1}')
def is_pixel_str(x):
return pix_re.fullmatch(x) != None
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this doesn't seem to be used anymore.

@culler
Copy link
Contributor Author

culler commented Sep 25, 2024

This is great, thank you! I'm satisfied with this change overall, but I think we should get a once-over from @serhiy-storchaka for the tkinter changes and a final confirmation from @zooba that the PCbuild/ changes match his expectations.

Thanks! That sounds good,

With your permission, I'd like to extract your ttk Notebook test fixes to a separate PR so that we can go ahead with it and get it backported. (If you would prefer to make that split yourself, that works for me.)

By all means. I would prefer to have you handle the split.

I expect that we will eventually be backporting this whole PR even if we don't start shipping 9.0 with our <3.14 binaries because at some point our non-Windows CI systems will inevitably update to 9.0, but I'm not sure how quickly that will happen and want to be sure we get 8.6.15 supported as soon as we can.

Great!

@culler
Copy link
Contributor Author

culler commented Sep 25, 2024

According to this email the release of Tcl/Tk 9.0.0.0 will happen tomorrow.

Addendum: Tcl/Tk 9.0.0.0 were in fact released today 2/26/2024 as scheduled.

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.

3 participants