Clean up Tcl library discovery in Tkinter on Windows

[zware]

BPO	20035
Nosy	@terryjreedy, @pfmoore, @vstinner, @tjguk, @bitdancer, @zware, @serhiy-storchaka, @zooba
Files	issue20035.diff: Remove tkinter._fix issue20035.v2.diff issue20035.v3.diff issue20035.v4.diff issue20035.v5.diff issue20035.v6.diff issue20035.v7.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/zware'
closed_at = <Date 2015-05-22.16:39:24.567>
created_at = <Date 2013-12-20.16:30:50.050>
labels = ['type-bug', 'expert-tkinter', 'OS-windows']
title = 'Clean up Tcl library discovery in Tkinter on Windows'
updated_at = <Date 2015-05-22.16:39:24.566>
user = 'https://github.com/zware'

bugs.python.org fields:

activity = <Date 2015-05-22.16:39:24.566>
actor = 'zach.ware'
assignee = 'zach.ware'
closed = True
closed_date = <Date 2015-05-22.16:39:24.567>
closer = 'zach.ware'
components = ['Tkinter', 'Windows']
creation = <Date 2013-12-20.16:30:50.050>
creator = 'zach.ware'
dependencies = []
files = ['34286', '34564', '34662', '34830', '35563', '39401', '39416']
hgrepos = []
issue_num = 20035
keywords = ['patch']
message_count = 26.0
messages = ['206692', '206696', '206697', '212403', '212470', '212734', '214446', '214449', '215148', '215155', '215695', '215725', '216146', '218877', '218880', '218902', '220227', '225168', '241745', '243382', '243384', '243395', '243451', '243474', '243840', '243841']
nosy_count = 10.0
nosy_names = ['terry.reedy', 'paul.moore', 'vstinner', 'tim.golden', 'r.david.murray', 'BreamoreBoy', 'python-dev', 'zach.ware', 'serhiy.storchaka', 'steve.dower']
pr_nums = []
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue20035'
versions = ['Python 3.5']

[zware]

The attached patch refactors tkinter._fix's main logic into a function called 'fix_environ' which is called unconditionally on import, and adds a function 'unfix_environ' to undo the effects of fix_environ. fix/unfix_environ are then used in all test files that import tkinter (test___all__, test_tcl, test_tk, test_ttk_guionly, test_ttk_textonly, test_idle) to ensure that the environment is properly set to allow Tcl to load and to suppress regrtest's warning that os.environ has been modified.

Since tkinter._fix is an implementation detail, I assume this change isn't against the 'no new features' policy of all currently open branches, but if this needs to wait until 3.5, that's ok with me.

[serhiy-storchaka]

This looks fragile. What if some test will import tkinter after other test "unfix" the environment? I suggest unload the tkinter._fix module in unfix_environ(). Then next import should implicitly fix the environment. And explicit fix_environ() will be not needed.

[zware]

I like that idea, though I'm a bit wary of messing around is sys.modules. Is there another way to unload a module that I'm not aware of?

Here's a new patch that does that.

[terryjreedy]

The patch works to suppress the message for test_ttk_guionly. However, unfix_environ does not undo everything that needs to be undone, as evidenced by bpo-20800.

The patch does not fix test_idle. I suspect that this is because test_idle pulls in tests from numerous modules, none of which has the unfix call. However, rather than require a tearDowmnodule for every module that imports tkinter, lets fix the root problem. I thought of having the environment change warning function ignore changes to TCL/TK/TIX_LIBRARY, but even better would be to not change them. Rename _fix to _dirs and redefine its mission as setting and holding three module attributes: tcldir, tkdir, and tixdir. They would be set from either the environment or the alternate search code. Any other modules that need the info should get it from _dirs rather the environment.

I believe this part of _fix is buggy.
+ if "TCL_LIBRARY" not in os.environ:
+ for name in os.listdir(prefix):
+ if name.startswith("tcl"):
+ tcldir = os.path.join(prefix,name)
+ if os.path.isdir(tcldir):
+ os.environ["TCL_LIBRARY"] = tcldir
Both base/tcl and base/../tcktk/lib contain (for 3.4) directories named tcl8 and tcl8.6. Since only tcl8.6 works, the code above depends on listdir presenting it second. The 'if name' clause could be augmented with 'and name[-2] == '.'.

[zware]

Terry J. Reedy added the comment:

However, rather than require a tearDowmnodule for every module that imports
​> ​tkinter, lets fix the root problem.

I agree that that would be the ideal solution.

I thought of having the environment change warning function ignore changes to
​> ​TCL/TK/TIX_LIBRARY, but even better would be to not change them. Rename _fix
to ​_dirs and redefine its mission as setting and holding three module
attributes: ​tcldir, tkdir, and tixdir. They would be set from either the
environment or the ​alternate search code. Any other modules that need the
info should get it from ​_dirs rather the environment.

Unfortunately, environment variables are the easiest way to tell Tcl/Tk where to find init.tcl/tk.tcl. Digging around in tclBasic.c and tkWindow.c, it looks like it should be possible to provide our own values for where to find the libraries, but it doesn't look especially easy. It looks like it has to be done somewhere between Tcl_CreateInterp() and Tcl_Init(), which means it has to be done in _tkinter.c.

I believe this part of _fix is buggy.

•    if "TCL_LIBRARY" not in os.environ:
  

•        for name in os.listdir(prefix):
  

•            if name.startswith("tcl"):
  

•                tcldir = os.path.join(prefix,name)
  

•                if os.path.isdir(tcldir):
  

•                    os.environ["TCL_LIBRARY"] = tcldir
  

Both base/tcl and base/../tcktk/lib contain (for 3.4) directories named tcl8
and​ tcl8.6. Since only tcl8.6 works, the code above depends on listdir
presenting it​ second. The 'if name' clause could be augmented with
'and name[-2] == '.'.

Actually, setting TCL_LIBRARY to either one (full path to tcl8 or tcl8.6) or, in fact, the same path without any version qualifier at all, works (tested by manually setting the variables before calling tkinter.Tk()). It looks like Tcl checks for a folder with the right version number on its own.

Also, from testing with installed 2.7, 3.3, and 3.4rc1, it looks like we only really need to set TCL_LIBRARY anyway: setting TK_LIBRARY and TIX_LIBRARY to '.', properly setting TCL_LIBRARY, and then calling tkinter.Tk(), the expected toplevel window pops up and root.tk.eval('package require Tix') returns the expected version number from Tix.

I'm convinced that my initial patches in this issue are the wrong way to attack the problem. I'll look into changing _tkinter.c to do the job of tkinter._fix without changing environment variables, hopefully we can just do away with _fix entirely.

[zware]

Here's a patch that seems to work; with it applied, I can run PCbuild\python_d.exe -m test -uall -rF test_tcl test_tk test_ttk_textonly test_ttk_guionly test_idle through at least 115 rounds of testing with no warnings or errors aside from a locale warning from test_idle the first time around. Hacking up my rc1 install with a fresh _tkinter.pyd and patched tkinter/init.py, the tests all pass, but I still get the 'can't invoke "event" command: application has been destroyed' message from test_ttk_guionly.

Terry, would you mind checking how this impacts all of the Tkinter test interaction issues you're seeing?

Since this is the first non-superficial semantic C patch I've submitted, I have no doubt that it can use a lot of work :). There are a few comments in places where I'm not really sure how best to do things that I'd really appreciate input on. It seems to work for common cases, though.

[python-dev]

New changeset c12cc78d59c1 by Zachary Ware in branch 'default':
Issue bpo-15968: Temporarily revert change to PCbuild/rt.bat
http://hg.python.org/cpython/rev/c12cc78d59c1

[zware]

Here's a slightly less ugly version of the patch. I would really appreciate some review on this; this should solve a test_idle issue that was exacerbated by issue bpo-15968 (for unknown reasons).

[zware]

Here's an even less ugly new version of the patch; it does everything with multi-byte strings instead of wide-char strings (so that there's just one conversion of prefix from wcs to mbs at the beginning of the block, and TCL_VERSION is used directly). This patch also cleans up the Tkinter tests to remove the previous workarounds and un-reverts the change to PCbuild/rt.bat that I reverted after bpo-15968 in an attempt to avoid test failures (that apparently didn't work).