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.

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.

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.

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] == '.'.

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.

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.

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

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).

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).

Also, I have confirmed that the blind symlink issue in non-English Windows Vista (bpo-3881) is not a problem in Tcl/Tk 8.6. That issue was easy to reproduce in a standard installation of Python 3.3 (with Tcl/Tk 8.5) on German Windows Vista by setting TCL_LIBRARY manually; the same test with a standard installation of Python 3.4 (with Tcl/Tk 8.6) showed no problem. Thus, I don't believe the convert_path() acrobatics from the top of Lib/tkinter/_fix.py are necessary in the C replacement.

TCL_VERSION should be set before call of Tcl_FindExecutable() (for correct Tcl encodings initialization). Tcl_FindExecutable() is called in PyInit__tkinter().

Serhiy Storchaka wrote:

TCL_VERSION should be set before call of Tcl_FindExecutable() (for correct Tcl
encodings initialization). Tcl_FindExecutable() is called in PyInit__tkinter().

I assume you mean TCL_LIBRARY (since TCL_VERSION is #defined in Tcl.h)? You are correct though, I missed that part of the comment at the top of tkinter._fix. However, I don't know what issues arise from not having TCL_LIBRARY set before Tcl_FindExecutable() (I haven't seen any problems, but I live in an ASCII world). Also, I have tried stepping through the Tcl_FindExecutable() call in PyInit__tkinter() with the VS debugger, and didn't see anything that looked like it was trying to read TCL_LIBRARY or hit the filesystem at all, which makes me suspect that it may not actually need TCL_LIBRARY prior to Tcl_FindExecutable(). Can you give me steps to reproduce problems with not having TCL_LIBRARY set that early?

Ok, debugging again with better breakpoints, I see what I missed before, so here's a new patch that does things a little differently. This patch sets the TCL_LIBRARY envvar just before calling Tcl_FindExecutable, and unsets it after the call. The $tcl_library Tcl var is set after interpreter creation, as in the previous patch. Both places do nothing if TCL_LIBRARY envvar is already set or the Tcl library isn't in one of the two expected locations.

I attempted to get things to work by setting Tcl's encoding search path before the call to Tcl_FindExecutable, but doing so seems to require some part of the initialization done by Tcl_FindExecutable. I would much prefer a solution that doesn't mess around with the TCL_LIBRARY envvar at all, but I've had no luck with it.

Ping. I still want to get this in, but not without a proper review.

I want to get this too, but perhaps there are some issues in a code.

1. Py_GetPrefix() returns wchar_t* string with maximal length MAXPATHLEN (defined in Include/osdefs.h as 256 on Windows). Then wcstombs() converts it to char* string. Are you sure that MAX_PATH (defined as 260 on Windows) is enough for converted string? I afraid that for multi-byte encoding it can be 2*MAXPATHLEN or even 3*MAXPATHLEN bytes.

2. After converting _tcl_library contains a path in locale encoding. And _putenv() works with it. I'm not sure, but I afraid that Tcl_SetVar() can expect UTF-8 encoded string. Please test with prefix containing non-ASCII characters (or even better with prefix containing East-Asian characters on East-Asian Windows).

Thank you, Serhiy; those are exactly the kinds of things I don't know enough about and had concerns about. I'll take another stab and see if I can come up with anything better. Suggestions welcome :)

Ok, here's another attempt which should address both points you raised, Serhiy. I don't have an East-Asian Windows install to test on (and wouldn't be able to read anything to do the test, anyway!), but I did test with a prefix containing East-Asian characters on US-English Windows. Command Prompt had issues and MSVC choked completely, so I could not run with the debugger for that test, but Python and Tkinter seemed to work fine.

Can we have (hopefully) a final review and get this committed as that would also allow us to close bpo-10652.

ping.

Here's a new patch that actually applies, and has a couple of improvements.

I'd greatly appreciate some review on this; I'm planning on committing before beta1 anyway.

I afraid about Tix. _fix.py searches not only Tcl/Tk, but Tix too. New patch doesn't set TIX_LIBRARY. Unfortunately there are no Tix tests at all, and we can break the support of Tix without the notice.

There's an MSBuild property that I set for release builds (either BuildForRelease or ReleaseBuild, not Configuration) that should exclude the preprocessor directive so we don't include an unnecessary full path. I can make that change later if you don't want to hold this patch up any longer though, as the rest looks fine to me. I don't know enough about Tix to comment on Serhiy's point.

I committed a presence test for Tix in bpo-21337 that should catch problems with loading Tix after this patch. There shouldn't be any problem though, we actually install Tix into Tcl/Tk whereas TIX_LIBRARY is (to my understanding) for helping Tcl/Tk find a Tix that it can't find itself.

Steve, I think I have BuildForRelease patched in properly; it works with BuildForRelease unset at least :)

Use !='true' rather than =='', but otherwise it's good.

New changeset 951318b651be by Zachary Ware in branch 'default':
Issue bpo-20035: Reimplement tkinter._fix module as a C function.
https://hg.python.org/cpython/rev/951318b651be

Committed, thanks for the reviews, Serhiy and Steve!