python / cpython

The Python programming language
https://www.python.org
Other
63.5k stars 30.41k forks source link

New test_ttk failure on Mac: "bad screen distance" #124378

Closed smontanaro closed 1 month ago

smontanaro commented 1 month ago

Bug report

Bug description:

I've been running with some patches from @ronaldoussoren to the following:

    modified:   Lib/test/support/__init__.py
    modified:   Lib/test/test_ttk/test_style.py
    modified:   Lib/test/test_ttk/test_widgets.py

(See attached diff.) test_ttk.txt

Everything was working fine, but today I began to get a repeatable failure in test_ttk:

======================================================================
FAIL: test_configure_height (test.test_ttk.test_widgets.NotebookTest.test_configure_height)
----------------------------------------------------------------------
_tkinter.TclError: bad screen distance ""

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/skip/src/python/cpython/Lib/test/test_tkinter/widget_tests.py", line 539, in test_configure_height
    self.checkIntegerParam(widget, 'height', 100, -100, 0)
    ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/skip/src/python/cpython/Lib/test/test_tkinter/widget_tests.py", line 81, in checkIntegerParam
    self.checkInvalidParam(widget, name, '', errmsg=errmsg)
    ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/skip/src/python/cpython/Lib/test/test_tkinter/widget_tests.py", line 67, in checkInvalidParam
    with self.assertRaisesRegex(tkinter.TclError, errmsg or ''):
         ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: "\Aexpected integer but got ""\Z" does not match "bad screen distance """

======================================================================
FAIL: test_configure_width (test.test_ttk.test_widgets.NotebookTest.test_configure_width)
----------------------------------------------------------------------
_tkinter.TclError: bad screen distance ""

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/skip/src/python/cpython/Lib/test/test_tkinter/widget_tests.py", line 543, in test_configure_width
    self.checkIntegerParam(widget, 'width', 402, -402, 0)
    ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/skip/src/python/cpython/Lib/test/test_tkinter/widget_tests.py", line 81, in checkIntegerParam
    self.checkInvalidParam(widget, name, '', errmsg=errmsg)
    ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/skip/src/python/cpython/Lib/test/test_tkinter/widget_tests.py", line 67, in checkInvalidParam
    with self.assertRaisesRegex(tkinter.TclError, errmsg or ''):
         ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: "\Aexpected integer but got ""\Z" does not match "bad screen distance """

----------------------------------------------------------------------
Ran 342 tests in 6.364s

I'm putting this out there with no further investigation. I doubt it will be a release blocker for 3.13, but if so, maybe the sprinters can find and fix the problem quickly.

CPython versions tested on:

3.12, 3.13, CPython main branch

Operating systems tested on:

macOS

Linked PRs

terryjreedy commented 1 month ago

The patch only touches the test_ttk files (and a support.init gui check) while the error is in test_tkinter. On my mac Catalina, `python3.13 -n test -ugui test_tkinter' runs without error. test_ttk has 2 or 3 failures different from the above, 1 in tkinter.

zware commented 1 month ago

@smontanaro Did you happen to just update to Tcl/Tk 8.6.15? I'm seeing the same failure on Windows in GH-124449. Unfortunately it does not seem to be addressed by @culler's GH-124156 (which is for gh-124111).

@terryjreedy:

while the error is in test_tkinter.

It is actually a test_ttk.test_widgets failure, in some test methods from test_tkinter.widget_tests.

smontanaro commented 1 month ago

@zware Yes, 8.6.15 via Homebrew, updated Sep 20:

otool -L Modules/_tkinter.cpython-314t-darwin.so
Modules/_tkinter.cpython-314t-darwin.so:
    /opt/homebrew/opt/tcl-tk/lib/libtk8.6.dylib (compatibility version 8.6.0, current version 8.6.15)
    /opt/homebrew/opt/tcl-tk/lib/libtcl8.6.dylib (compatibility version 8.6.0, current version 8.6.15)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2)

I'm doing nothing to specify the Homebrew location. I see nothing which would be Apple-specific. Is that okay? I also a have slightly older version in my Python 3.12 Conda environment. I'll see if I can link to that.

smontanaro commented 1 month ago

Doesn't look like I can compile/link against the miniconda3 stuff:

% gcc -bundle -undefined dynamic_lookup      Modules/_tkinter.o Modules/tkappinit.o -L/Users/skip/miniconda3/envs/python312/lib -ltk8.6 -ltkstub8.6 -ltcl8.6 -ltclstub8.6  -o Modules/_tkinter.cpython-314t-darwin.so

ld: warning: object file (/Users/skip/src/python/cpython/Modules/_tkinter.o) was built for newer 'macOS' version (14.6) than being linked (14.0)
ld: warning: object file (/Users/skip/src/python/cpython/Modules/tkappinit.o) was built for newer 'macOS' version (14.6) than being linked (14.0)

The resulting so file fails to import:

ERROR] _tkinter failed to import: dlopen(/Users/skip/src/python/cpython/build/lib.macosx-14.6-arm64-3.14/_tkinter.cpython-314t-darwin.so, 0x0002): Library not loaded: @rpath/libtk8.6.dylib
  Referenced from: <8EFFE0FA-0FD4-3E70-8AFD-11143C118A56> /Users/skip/src/python/cpython/Modules/_tkinter.cpython-314t-darwin.so
  Reason: no LC_RPATH's found
Following modules built successfully but were removed because they could not be imported:
_tkinter                                                                   
zware commented 1 month ago

That's good news, actually :). Something seems to have changed between 8.6.14 and 8.6.15 in ttk's Notebook, so we just need to adjust our tests.

culler commented 1 month ago

On Tue, Sep 24, 2024 at 3:10 PM Zachary Ware @.***> wrote:

Unfortunately it does not seem to be addressed by @culler https://github.com/culler's GH-124156 https://github.com/python/cpython/pull/124156 (which is for gh-124111 https://github.com/python/cpython/issues/124111).

@zware, when you say that it is not addressed do you mean because GH-124156 is for 3.14? (Because I do recall fixing that error.)

zware commented 1 month ago

@zware, when you say that it is not addressed do you mean because GH-124156 is for 3.14? (Because I do recall fixing that error.)

I'm double checking, but I was still seeing the failure on Windows with 8.6.15 using your GH-124156 branch merged into my GH-124449 branch.

culler commented 1 month ago

I was using the tip of core-8-6-branch, rather than the released 8.6.15, That could account for the difference.

On Tue, Sep 24, 2024 at 4:37 PM Zachary Ware @.***> wrote:

@zware https://github.com/zware, when you say that it is not addressed do you mean because GH-124156 https://github.com/python/cpython/pull/124156 is for 3.14? (Because I do recall fixing that error.)

I'm double checking, but I was still seeing the failure on Windows with 8.6.15 using your GH-124156 https://github.com/python/cpython/pull/124156 branch merged into my GH-124449 https://github.com/python/cpython/pull/124449 branch.

— Reply to this email directly, view it on GitHub https://github.com/python/cpython/issues/124378#issuecomment-2372507927, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJ6CP437Y4QEYJ3CMNF6RDZYHSRNAVCNFSM6AAAAABOW4D572VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZSGUYDOOJSG4 . You are receiving this because you were mentioned.Message ID: @.***>

zware commented 1 month ago

Ah ha. How likely is an 8.6.16 with that fix? :)

culler commented 1 month ago

I think it will be a year before we see 8.6.16. I will switch to the released 8.6.15 and update my PR so it works with that.

On Tue, Sep 24, 2024 at 4:50 PM Zachary Ware @.***> wrote:

Ah ha. How likely is an 8.6.16 with that fix? :)

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

culler commented 1 month ago

Indeed, I can reproduce this test failure in GH-124156 with the tips of the core-8-6-16-rc branches (which were used for the release). I am now fixing it in the PR.

On Tue, Sep 24, 2024 at 5:10 PM Marc Culler @.***> wrote:

I think it will be a year before we see 8.6.16. I will switch to the released 8.6.15 and update my PR so it works with that.

On Tue, Sep 24, 2024 at 4:50 PM Zachary Ware @.***> wrote:

Ah ha. How likely is an 8.6.16 with that fix? :)

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

culler commented 1 month ago

I can explain the cause of this problem. There was a change to the Notebook widget after the release of 8.6.14 (the change was in February 2023) which made the Notebook's width and height become "pixel" options rather than integer options. A pixel option can accept values such as "10c" (i.e 10 centimeters). As far as the tkinter test suite is concerned, this means that checkPixelsParam should be used instead of checkIntegerParam. Making just that change in the test_configure_width and test_configure_height methods of the NotebookTest class makes these failures go away. In my PR I did exactly that, but only for 9.0, not for 8.6.

However, this "solution" has drawbacks. Namely, it does not test whether it actually works to set the width of a Notebook widget to "10c", because those two methods only test with integer values. Testing pixel options causes additional issues because, at the moment, the various widgets treat these options inconsistently. Setting an option to "10c" with configure and retrieving it with cget sometimes produces "10c" and sometimes produces the number of pixels, at the current screen resolution, which are needed to span a distance of 10 cm. The former behavior is correct (since screen resolutions can change any time) but not all widgets have yet been converted to have the correct behavior. That change is happening widget by widget at the moment.

culler commented 1 month ago

I think this is now fixed in GH-124156 (even though the thread sanitizer check seems to have failed - the failure seems unrelated to my changes).

zware commented 1 month ago

@smontanaro, can you confirm that GH-124542 fixed these test_ttk failures for you? I expect it should, but confirmation would be nice before merging the backports :)

smontanaro commented 1 month ago

@smontanaro, can you confirm that GH-124542 fixed these test_ttk failures for you? I expect it should, but confirmation would be nice before merging the backports :)

Confirmed, though it does appear the PR was applied before I got to it.

zware commented 1 month ago

Thanks, Skip. I was confident enough to go ahead with the main merge, but wanted to be sure before merging the backports :)