classification
Title: PEP 3121, 384 Refactoring applied to tkinter module
Type: resource usage Stage: resolved
Components: Extension Modules Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: asvetlov Nosy List: Robin.Schreiber, amaury.forgeotdarc, asvetlov, loewis, ned.deily, pitrou, python-dev, serhiy.storchaka
Priority: normal Keywords: pep3121

Created on 2012-08-18 10:53 by Robin.Schreiber, last changed 2016-05-08 18:31 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
_tkinter_pep3121-384_v0.patch Robin.Schreiber, 2012-08-18 10:53
_tkinter_pep3121-384_v1.patch asvetlov, 2012-10-06 15:27 review
_tkinter_pep3121-384_v2.patch asvetlov, 2012-10-06 21:12 review
_tkinter_pep3121-384_v3.patch asvetlov, 2012-10-06 21:36 review
_tkinter_pep3121-384_v4.patch asvetlov, 2012-10-06 22:49 review
_tkinter_pep3121-384_v7.patch asvetlov, 2012-10-07 09:59
_tkinter_pep384_v1.patch asvetlov, 2012-10-07 14:26 review
Messages (24)
msg168504 - (view) Author: Robin Schreiber (Robin.Schreiber) * Date: 2012-08-18 10:53
Changes proposed in PEP3121 and PEP384 have now been applied to the tkinter module!
When running the test form inside Python.exe (tkinter._test()), the litte "test-window" is rendered correctly. However there are still some error messages popping up inside the python shell 
(I am running this on OS X 10.8):

>>> tkinter._test()
2012-08-18 12:46:24.775 python.exe[17410:707] speedup (null)
2012-08-18 12:46:24.776 python.exe[17410:707] ERROR: Unable to find method [_NSFullScreenTransition _startFullScreenTransitionForCGWindow:targetFrame:duration:].
2012-08-18 12:46:24.776 python.exe[17410:707] ERROR: Unable to find method [_NSFullScreenTransition _startEnterFullScreenTransitionForCGWindow:targetFrame:duration:].
2012-08-18 12:46:24.778 python.exe[17410:707] ERROR: Unable to find method [_NSFullScreenTransition startExitFullScreenTransitionForCGWindow:targetFrame:duration:].
[87221 refs]
>>> 

I have no Idea how and where these are triggered.
msg168505 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-08-18 10:58
See

http://www.python.org/download/mac/tcltk/

It may be that using a different build of Tcl/Tk solves that problem.
msg168522 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-08-18 18:52
What version of 10.8 are you seeing those messages with?  And do you see them without the patch applied?
msg172210 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-10-06 15:27
Update patch conforming to current _tkinter code.
msg172213 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-10-06 15:55
I would to have all module state inside _tkinterstate structure.
static variables like tcl_lock, tcl_state, quitMainLoop, errorInCmd etc should be moved into _tkinterstate also.
msg172222 - (view) Author: Robin Schreiber (Robin.Schreiber) * Date: 2012-10-06 17:04
Before I submitted this patch, I used to have these variables inside the modulestate, which caused severe problems. I do not know the exact reason, but my guess is that these variables have to be globally available for every thread (tcl variables are used for thread synchronization arent they?). As the modulestate may change depending on the thread, one can no longer guarantee that every thread within the process is operating on the same variable. This might not be nessecary, however as I mentioned, the naive approach of putting the variables inside the modulestate did not work out for me.
msg172230 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-10-06 18:03
I'm trying to make patch following myself recommendations.
Looks good but not finished yet. Will publish it after all work done.
msg172245 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-10-06 21:12
Submit patch which store own state into module state.
Still not finished yet: need to add checks for "ready state" in every access to fields of module state structure.
msg172248 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-10-06 21:36
Update patch to support TKINTER_PROTECT_LOADTK option.
msg172257 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-10-06 22:49
Upload version which check if _tkinter module was destroyed (for example, if it was call from daemon thread runing Tk app when main thread is exiting with interpreter finalization).
msg172288 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-10-07 09:59
The patch for current default branch.
msg172306 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-10-07 14:26
The patch is huge, then I like to apply it in two steps: first implement pep 384 than 3121.
Attached first patch.
msg172883 - (view) Author: Roundup Robot (python-dev) Date: 2012-10-14 13:54
New changeset bf9d118779f5 by Andrew Svetlov in branch 'default':
Issue #15721: make _tkinter module pep384 compatible.
http://hg.python.org/cpython/rev/bf9d118779f5
msg172969 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-10-15 13:35
Andrew,

I have questions about the following part of commit bf9d118779f5:
+ PyTclObject_Type_slots[3].pfunc = PyObject_GenericGetAttr;

First, the "3" refers to the position of Py_tp_getattro in the array, which is a fragile thing IMO.

Then, this hack was not necessary before; why is it needed now?
IIRC on Windows a static initializer cannot contain dll-imported variables, but function pointers are OK.
Or do you have evidence of the contrary?
msg172985 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-10-15 16:37
Amaury, I completely agree with your objection. 
I've found this code in xxlimited.c and adapted to _tkinter source.
If that weird code can be removed I will do it.

I have no idea how we can catch/reproduce the problem, maybe Martin von Loewis can help as author of xxlimited.c code?

As I know almost nobody make PEP 384 compliant modules for now and maybe there are couple of dark corners at least in the docs.

See also #15650 for another question related to adaptation to pep3121.
msg174226 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-10-30 20:48
Amaury, you are right: 
PyTclObject_Type_slots[3].pfunc = PyObject_GenericGetAttr;
is not required.
msg174227 - (view) Author: Roundup Robot (python-dev) Date: 2012-10-30 20:49
New changeset 4eb6e07e8171 by Andrew Svetlov in branch 'default':
Issue #15721: apply PEP 384 Refactoring to tkinter module.
http://hg.python.org/cpython/rev/4eb6e07e8171
msg187261 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2013-04-18 15:42
Is any more work needed on this issue or can it be closed?
msg187380 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2013-04-19 18:26
No, PEP3121 patch is not ready yet but PEP384 has been applied.
msg194842 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-10 22:18
The first two patches (_tkinter_pep3121-384_v0.patch and _tkinter_pep3121-384_v1.patch) seemed to have the refcounts right, but Andrew's further patches lost the required Py_INCREF in the various new() functions (and the corresponding Py_DECREF in deallocs), which is the cause for the crash in #15667.
msg194843 - (view) Author: Roundup Robot (python-dev) Date: 2013-08-10 22:22
New changeset 4d0c938870bc by Antoine Pitrou in branch 'default':
Fix refcounting issue with extension types in tkinter.
http://hg.python.org/cpython/rev/4d0c938870bc
msg231696 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-11-26 07:48
_tkinter classes are now heap types and they no longer have the __module__ attribute.

>>> import _tkinter
>>> _tkinter.TkappType.__module__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: __module__

See issue20204.
msg254680 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-11-15 08:23
This change causes a crash when create instances of _tkinter classes (issue23815). If this will not be fixed I suggest to revert the patch.
msg265154 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-08 18:31
Fixed in issue23815. Opened issue26979 for documenting the catch.
History
Date User Action Args
2016-05-08 18:31:21serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg265154

stage: patch review -> resolved
2015-11-15 08:23:44serhiy.storchakasetmessages: + msg254680
2014-11-26 07:48:35serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg231696
2014-02-03 17:03:22BreamoreBoysetnosy: - BreamoreBoy
2013-08-10 22:22:39python-devsetmessages: + msg194843
2013-08-10 22:18:03pitrousetnosy: + pitrou
messages: + msg194842
2013-04-19 18:26:06asvetlovsetmessages: + msg187380
2013-04-18 15:42:27BreamoreBoysetnosy: + BreamoreBoy
messages: + msg187261
2012-11-08 13:18:59Robin.Schreibersetkeywords: + pep3121, - patch
2012-10-30 20:49:29python-devsetmessages: + msg174227
2012-10-30 20:48:21asvetlovsetmessages: + msg174226
2012-10-15 16:37:06asvetlovsetmessages: + msg172985
2012-10-15 13:35:47amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg172969
2012-10-14 13:54:47python-devsetnosy: + python-dev
messages: + msg172883
2012-10-07 14:26:46asvetlovsetfiles: + _tkinter_pep384_v1.patch

messages: + msg172306
2012-10-07 09:59:56asvetlovsetfiles: + _tkinter_pep3121-384_v7.patch

messages: + msg172288
2012-10-06 22:49:19asvetlovsetfiles: + _tkinter_pep3121-384_v4.patch

messages: + msg172257
2012-10-06 21:36:39asvetlovsetfiles: + _tkinter_pep3121-384_v3.patch

messages: + msg172248
2012-10-06 21:24:32asvetlovsetstage: patch review
2012-10-06 21:12:40asvetlovsetfiles: + _tkinter_pep3121-384_v2.patch
assignee: asvetlov
messages: + msg172245
2012-10-06 18:03:11asvetlovsetmessages: + msg172230
2012-10-06 17:04:09Robin.Schreibersetmessages: + msg172222
2012-10-06 15:55:16asvetlovsetmessages: + msg172213
2012-10-06 15:27:35asvetlovsetfiles: + _tkinter_pep3121-384_v1.patch

messages: + msg172210
2012-08-27 03:49:13belopolskylinkissue15787 dependencies
2012-08-20 10:42:46asvetlovsetnosy: + asvetlov
2012-08-18 18:52:01ned.deilysetnosy: + ned.deily
messages: + msg168522
2012-08-18 10:58:31loewissetnosy: + loewis
messages: + msg168505
2012-08-18 10:53:44Robin.Schreibercreate