GTK3 reactor for gobject-introspected GTK3

[kkszysiu]

Hello. As you may know pyGTK will soon be deprecated and gtk2reactor will be no longer needed but a new one so Ill write it. It's based on a gtk2reactor but uses gobject-oriented GTK and GObject. Works well. If you want any examples I can attach it too.

[kkszysiu]

reactor

[thijs]

thanks kkszysiu. Could you provide a diff against the trunk and add unit tests? Once you're ready add the 'review' keyword to this ticket, as described on TwistedDevelopment.

[glyph]

Please don't add milestones to tickets unless you are clear on the ramifications for the Twisted release workflow. (This field is mostly for core developers.) Looking forward to seeing the patch though :).

[itamar]

We don't really need extra tests for this, nor does it have to be a patch given it's just a new module; running trial is sufficient. May need to add it to list of reactors ReactorBuilder uses, though. I will try to hook it up in a branch and see how it goes.

[itamarst]

(In [33234]) Branching to 'gtk3-4558'

[itamar]

Dobey claims this is impossible; I'm hoping for some better feedback. Also, Python segfaults if both pygtk and python-gi are imported (see  https://bugzilla.gnome.org/show_bug.cgi?id=664871). Segfaulting means this isn't really easily usable as part of our test suite, so we can't proceed until we get some clarity/bug fixes there.

It's possible both of these are due to Ubuntu's python-gobject-2 package, which has "deprecated static Python bindings for the GObject library". But maybe not.

[dobey]

Not impossible, but it really shouldn't work, and simply copying the gtk2reactor as gtk3reactor and changing the imports and class names is a recipe for disaster.

I filed  https://bugzilla.gnome.org/show_bug.cgi?id=665050 about the API being there, that shouldn't be there. We shouldn't continue using that API, and should move to using the correct API at the correct entry points (some of which is currently not fully bound by gobject-introspection).

I have an in-progress branch on Launchpad for this, but currently only using a PortableGtkReactor-based solution, which seems to be working quite well in my tests at the moment.

[itamar]

Using gobject-introspection and pygtk2 within the same process is apparently impossible, so this is going to make automated testing difficult.

[dobey]

It's not impossible, but it is very likely and plausible for it to crash arbitrarily if you do. The real big issue is mixing gtk2 and gtk3 symbols from the C libraries. Pure gobject/glib pieces should more or less work reasonably OK, but can also result in problems. Either way, it is something we do have to work around in some manner though, yes.

[dobey]

I just realized I hadn't mentioned already, but  https://code.launchpad.net/~dobey/twisted/gi-support is where I am working on a better solution to this.

[dobey]

Patch to add gireactor.py

[dobey]

I've attached a patch which adds a gireactor.py which supports using both gobject-introspection and static bindings, with the glib main loop, or the gtk+ one. To use static bindings support, one needs to import gobject first before importing gireactor. The tests work and pass (or get appropriately skipped) for me, when using trial -r gi and trial -r gtk2 both.

[itamar]

Can you briefly explain the difference between gobject-introspection and static bindings?

Am I correct in thinking that the code is virtually identical to the new gtk2reactor, except with different method names in a few key points?

[dobey]

Introspected bindings are used in python by means of "from gi.repository import Foo" while static bindings are "import foo." The newer versions of python-gobject (now python-gi) complain if you try to import introspected bindings, while already using static bindings. And may crash if importing static bindings, having already imported the introspected ones.

The code is nearly identical to the gtk2reactor, yes, save for a few key points. And unfortunately the two can't be reliably combined, due to the aforementioned issues with static vs. introspected bindings, as well as some features in twisted itself that can cause some problems when trying to make code be a bit smarter about things.

[itamar]

Thanks for implementing this, it'll be good to support the latest and great GTK/glib event loops.

1. Sharing seems possible (and useful) to me. It could be done by a module (e.g. twisted.internet._glibbase.py) that would have all the shared code, and which wouldn't import either version of the glib bindings so it could be imported by either reactor for subclassing.
2. We'd want a gi-gtk plugin as well, for the useGtk=True variant (see how gtk2reactor.py does it.)
3. I dislike the way it switches back to static bindings-based reactor if someone has already imported the old modules. If this is the gi reactor, it should always be the gi reactor. The new reactor could instead do something like:

   if 'gobject' in sys.modules:
      raise ImportError("Can't use gi reactor if old glib API is used; please switch to gi.repository, or use twisted.internet.glib2reactor instead of this reactor")
   

4. The old glib reactor should disable the imports of the new gi-bindings, much like the new reactor disables static modules.

[exarkun]

Did we come up with a plan for testing this?

[itamar]

Running tests with 'trial -r gi twisted' should (a) ensure glib isn't loaded, and therefore no glib reactor and (b) test it. So we'd need a buildslave configured to do that. This isn't ideal, the ideal is that we run all reactors, but not terrible (I can't run Windows reactors during personal development, for example). Especially since glib and gi reactors will share so much code once we're done refactoring.

[glyph]

Replying to itamar:

Running tests with 'trial -r gi twisted' should (a) ensure glib isn't loaded, and therefore no glib reactor and (b) test it. So we'd need a buildslave configured to do that. This isn't ideal, the ideal is that we run all reactors, but not terrible (I can't run Windows reactors during personal development, for example). Especially since glib and gi reactors will share so much code once we're done refactoring.

This situation is not without precedent either; the CoreFoundation reactor has basically the opposite issue (you can't run the ReactorBuilder tests under -r cf due to the non-reentrant nature of that event loop API; basically it's too similar to Twisted itself :)).

So I think this is fine; we have coverage, if we run the tests in all configurations, which we already have to do.

[exarkun]

We will need more hardware.

[dobey]

Updated patch per review comments

[dobey]

I've attached an updated patch. It implements the changes requested by itamar in comment #15. The tests work correctly here using all 4 of the affected reactors (glib2, gtk2, gi, gtk3). There is a tiny bit of an issue running the tests under the gtk3 reactor on Ubuntu Oneiric 11.10. It seems to probably be a slight timing issue in GTK+ 3.2, or the introspected bindings for it, which causes a hang when reaching the testControlC for the manhole stdio tests. However, this test works fine on Ubuntu Precise 12.04 with GTK+ 3.3, and works fine on Ubuntu Oneiric 11.10, when running the test suite under strace. I'm not entirely sure how to work around this (or what the appropriate fix is in GTK+ itself), yet. The code in this patch is sound though, and a review would be greatly appreciated. Thanks.

[glyph]

Thanks, dobey, for your continued efforts on this subtle and tricky issue.

[itamar]

At first glance looks pretty good. I love not having duplicate code :) I'll try to do more in depth look soon.

[itamarst]

(In [33422]) Branching to 'gtk3-4558-2'

[itamar]

Looking pretty good, almost ready for merge. I cleaned the code up a bit to follow the code standard, have docstrings in correct places, etc. Future patches should go against the branch.

1. Switching portable gtk reactor to idle sounds like a good idea, but I suggest doing that in a separate ticket. If we're lucky, we can use *only* idle callbacks (perhaps with iterate(0.01)) instead of the polling we're doing now. I'll add that ticket when this one is closed. So unless you had a particular reason to start that, no actual work to do.
2. We need to open a ticket (or add to this one if you wish) for documenting new reactor in the choosing reactors howto once this ticket is closed.
3. There's still some code duplication in the reactor's __init__ and input_add methods, which could be fixed by having the base class take glib and gtk modules as arguments to its __init__. That way all the code can be in the base class.
4. When running under gi reactor, twisted.internet.test.test_core.SystemEventTestsBuilder_Gtk3Reactor and an additional test elsewhere I didn't track down output the following warning: (trial:31329): Gtk-CRITICAL **: gtk_main_quit: assertion main_loops != NULL' failed` - any idea what that means, or if we can prevent it?
5. I started a buildbot run here:  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/gtk3-4558-2 -- all tests should pass, or at least the Windows gtk ones should be no worse.
6. The test suite should be run with the gtk3 reactor on Windows if at all possible (if you don't have access to one I can manage it probably).

[itamar]

Oh, and, you should update the choosing reactors howto to talk about this, so people know about it (making clear that "gtk3" really means gobject-introspection gtk3).

[dobey]

The buildbot tests on Windows seem to be at least as well as trunk was. If you merge current trunk into the branch, it looks like the failing tests would be fixed. I don't really have a setup to run any tests on Windows, so I'd greatly appreciate it if you could run the gtk3reactor tests there.

How many versions of pygtk do we need to continue to support here, as well? There's a lot of cruft in the current reactor that I am hoping I don't have to keep around, especially with having the _glibbase.py classes which would work with both static or gi bindings, depending on which module path is passed in to use.

[dobey]

Updated patch against gtk3-4558-2 branch

[dobey]

Here is the latest patch, this time against the gtk3-4558-2 svn branch. This minimizes the derived versions of the reactor further as requested. The PortableGtkReactor is still using the timeout with 1010 and the min() call. The only difference in the previous patch was that it was changed to 1000 instead of 1010 for the timeout. I changed it back to 1010. I am not sure what the gtk_main_quit warnings were exactly. I think they were related to my removal of the main_quit wrapper which checked gtk_main_level(). A similar problem happened in the gtk2reactor tests when working on this latest patch. Adding the wrapper back fixed the issue, and I don't see the Gtk-CRITICAL about gtk_main_quit either.

Where exactly is the "choosing a reactor" documentation? Can we do that in another patch? Thanks.

[itamar]

Correcting author.

[itamar]

Note, BTW, that I'm leaving out Canonical from the module-specific copyright; our default to say "Twisted Matrix Labs" and then list everyone in LICENSE, where Canonical is already listed.

[itamar]

So far looking good, although I don't yet have time for a full review this morning.

Why are you doing "self._crash = lambda: self._glib.idle_add(self.loop.quit)" in the glib version, but just "self._crash = _mainquit" in the gtk version? I.e. why the use of idle in one but not the other?

The choosing reactors howto is this:  http://twistedmatrix.com/documents/current/core/howto/choosing-reactor.html

The change to it could be done in another ticket.

I am considering renaming gtk3 to gi-gtk, since it's more accurate (the current windows installers are actually still gtk2, in fact).

[dobey]

So, I wouldn't say "gi-gtk" is more accurate. Gtk3 is Gtk3, and it is not usable from Python, without gobject-introspection. Implying that it is, by calling it gi-gtk instead, doesn't really help. It is an upstream change. There aren't now, nor in the future, going to be official static bindings of Gtk3 for Python. It would also complicate matters when GTK+ decides to break API again, and Gtk4 comes out, and extra work would have to be done to specify which version to use exactly.

The idle_add was used as there appeared to be a timing issue, in testing, which the use of idle_add corrects, there. I wasn't seeing the same issue with the gtk version of the reactor. I am not entirely sure why.

[itamar]

Ready for merge, I think.

[itamarst]

(In [33490]) Merge gtk3-4558-2: New gtk3 and gobject-introspection reactors.

Author: dobey Reviewer: itamar Fixes: #4558

Add new gtk3 and gobject introspection reactors.