Opened 7 years ago
Closed 6 years ago
#5676 enhancement closed fixed (fixed)
Support gi.pygtkcompat in gi reactor
Reported by: | Itamar Turner-Trauring | Owned by: | Itamar Turner-Trauring |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | |
Cc: | jdahlin, jesstess | Branch: |
branches/pygtkcompat-5676
branch-diff, diff-cov, branch-cov, buildbot |
Author: | itamar |
Description
Newer versions of gi provide a compatibility layer for the older pygtk API. Currently, if pygtk has been loaded we don't allow loading gi reactor, since this is incompatible and cause segfaults. Likewise, once gi reactor is loaded we prevent imports of pygtk modules like gobject. However, if gi.pygtkcompat
is available, we should instead:
- If we can figure out that pygtkcompat was enabled, don't prevent import of gi reactor even if we see pygtk loaded, since we know it's not actually the real pygtk.
- Enable the compatibility layer instead of preventing import of gobject and friends.
Change History (13)
comment:1 Changed 7 years ago by
Author: | → itamarst |
---|---|
Branch: | → branches/pygtkcompat-5676 |
comment:2 Changed 7 years ago by
Author: | itamarst → itamar |
---|---|
Cc: | jdahlin added |
Keywords: | review added |
OK, ready for review. It'd be nice to get this into 12.1. Buildbot is in progress, I'll check status in a bit:
http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/pygtkcompat-5676
comment:3 Changed 7 years ago by
Owner: | set to jesstess |
---|
comment:4 Changed 7 years ago by
Cc: | jesstess added |
---|---|
Keywords: | review removed |
Owner: | changed from jesstess to Itamar Turner-Trauring |
Thanks for working on this, itamar. Your changes look good; a couple of comments:
- Using both this branch and the 12.1.0 release, this is what happens when I try to import
gireactor
orgtk3reactor
:
$ python -c "import twisted; print twisted.__version__; from twisted.internet import gtk3reactor" 12.0.0+r34756 Traceback (most recent call last): File "<string>", line 1, in <module> File "twisted/internet/gtk3reactor.py", line 22, in <module> from twisted.internet import gireactor File "twisted/internet/gireactor.py", line 53, in <module> _oldGiInit() File "twisted/internet/gireactor.py", line 42, in _oldGiInit GLib.threads_init() File "/usr/lib/python2.7/dist-packages/gi/module.py", line 268, in __getattr__ return getattr(self._introspection_module, name) File "/usr/lib/python2.7/dist-packages/gi/module.py", line 101, in __getattr__ self.__name__, name)) AttributeError: 'gi.repository.GLib' object has no attribute 'threads_init' $ python -c "import twisted; print twisted.__version__; from twisted.internet import gireactor" 12.0.0+r34756 Traceback (most recent call last): File "<string>", line 1, in <module> File "twisted/internet/gireactor.py", line 53, in <module> _oldGiInit() File "twisted/internet/gireactor.py", line 42, in _oldGiInit GLib.threads_init() File "/usr/lib/python2.7/dist-packages/gi/module.py", line 268, in __getattr__ return getattr(self._introspection_module, name) File "/usr/lib/python2.7/dist-packages/gi/module.py", line 101, in __getattr__ self.__name__, name)) AttributeError: 'gi.repository.GLib' object has no attribute 'threads_init'
The tests know to skip when they don't have the dependencies to support these reactors (http://twistedmatrix.com/trac/ticket/5494), but this is a pretty bad experience for users. Can you open a ticket for addressing this and printing out a nice error message instead of throwing a traceback?
- We now have this list of glib/gtk libraries ("gobject", "glib", "gio", "gtk") that we do various tests on in 3 places, twice in
gireactor.py
and once intest_gireactor.py
. Is it time to put these in a variable, to avoid the risk of accidentally updating some instances and not others?
- The patch has created an unused import:
$ pyflakes twisted/internet/gireactor.py twisted/internet/gireactor.py:21: 'sys' imported but unused
- We need a build slave to test these configurations. Can you open a task ticket to track this and hopefully incentivize someone to do it (perhaps compelled by the high scores list :) )
Once these things have been addressed, I think this is ready to merge.
comment:5 Changed 7 years ago by
- I've created a new ticket, #5790.
- Done.
- Done.
I'm concerned about requiring build slave; on the one hand the need is obvious, on the other hand it can keep us from deploying fixes for code we've already released, and it may take a while until one materializes. Hopefully not too long, since we've been offered a bunch of VMs recently.
If I got #5790 fixed would you be willing to rely on tests passing on my machine and yours?
comment:6 Changed 7 years ago by
I didn't mean to imply that we needed a build slave to merge this -- I just want us to at least track the desire to have a build slave configured to be able to exercise these tests.
So yes, with the resolution of #5790 I'd be happy to consider this ready to merge.
comment:7 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | changed from Itamar Turner-Trauring to jesstess |
I've included a fix for #5790; please let me know if this solves the problem for you on your machine (I couldn't think of a reasonable way to test this, unfortunately.)
comment:8 Changed 7 years ago by
You are aware, of course, that no Python program should ever use hasattr
, as it is a broken interface which hides arbitrary exceptions by design. getattr(obj, name, sentinel) is not sentinel
is the safe way to spell this check (as is catching the AttributeError
from the attribute lookup (not from the call)).
comment:10 Changed 6 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from jesstess to Itamar Turner-Trauring |
Thanks for seeing this through, itamar; this fixes the issue on my machine, and build results look good. I think this is ready to merge!
comment:11 Changed 6 years ago by
Now I'm wondering if we should be enabling gi.pygktcompat
at all. Possibly just checking for its existence and then relying on gi and the old python wrappers to interact correctly on their own may be sufficient, and is less side-effecty.
comment:12 Changed 6 years ago by
Ah yes, I decided to enable it because if you disable it you get this:
$ trial -r gi twisted /usr/lib/python2.7/dist-packages/gobject/constants.py:24: Warning: g_boxed_type_register_static: assertion `g_type_from_name (name) == 0' failed import gobject._gobject /usr/lib/python2.7/dist-packages/gtk-2.0/gtk/__init__.py:40: Warning: specified class size for type `PyGtkGenericCellRenderer' is smaller than the parent type's `GtkCellRenderer' class size from gtk import _gtk /usr/lib/python2.7/dist-packages/gtk-2.0/gtk/__init__.py:40: Warning: g_type_get_qdata: assertion `node != NULL' failed from gtk import _gtk Segmentation fault (core dumped)
comment:13 Changed 6 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [34392]) Branching to 'pygtkcompat-5676'