Opened 3 years ago

Closed 2 years ago

#5676 enhancement closed fixed (fixed)

Support gi.pygtkcompat in gi reactor

Reported by: itamar Owned by: itamar
Priority: normal Milestone:
Component: core Keywords:
Cc: jdahlin, jesstess Branch: branches/pygtkcompat-5676
(diff, github, buildbot, log)
Author: itamar Launchpad Bug:

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:

  1. 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.
  2. Enable the compatibility layer instead of preventing import of gobject and friends.

Change History (13)

comment:1 Changed 3 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/pygtkcompat-5676

(In [34392]) Branching to 'pygtkcompat-5676'

comment:2 Changed 3 years ago by itamar

  • Author changed from itamarst to 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 2 years ago by jesstess

  • Owner set to jesstess

comment:4 Changed 2 years ago by jesstess

  • Cc jesstess added
  • Keywords review removed
  • Owner changed from jesstess to itamar

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 or gtk3reactor:
$ 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 in test_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 2 years ago by itamar

  • 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 2 years ago by jesstess

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 2 years ago by itamar

  • Keywords review added
  • Owner changed from itamar 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 2 years ago by exarkun

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:9 Changed 2 years ago by itamarst

(In [35160]) Use getattr() instead of hasattr(). Refs #5676

comment:10 Changed 2 years ago by jesstess

  • Keywords review removed
  • Owner changed from jesstess to itamar

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 2 years ago by itamar

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 2 years ago by itamar

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 2 years ago by itamarst

  • Resolution set to fixed
  • Status changed from new to closed

(In [35285]) Merge pygtkcompat-5676: Better support for newer and older gi versions.

Author: itamar
Review: jesstess
Fixes: #5676, #5790

Work with old versions of gi that didn't wrap enable_threads(), and with newer versions that have the gi.pygtkcompat compatability layer.

Note: See TracTickets for help on using tickets.