Opened 5 years ago

Last modified 4 weeks ago

#3977 enhancement new

Reintegrate qt4reactor into twisted

Reported by: jknight Owned by: michaelnt
Priority: normal Milestone:
Component: core Keywords:
Cc: ghtdak, exarkun, thijs Branch: branches/qt4reactor-pyside-3977-2
(diff, github, buildbot, log)
Author: michaelnt, thijs Launchpad Bug:

Description

So, the inevitable has happened, and there are now finally LGPL python bindings for the previously LGPL'd Qt, called "PySide":
http://codeposts.blogspot.com/2009/08/lgpl-python-bindings-for-qt-released.html

Supposedly, PySide is even API compatible with PyQt: just replace every PyQt4 import with a PySide import.

It'd thus be nice to get a qt4reactor like (http://github.com/ghtdak/qtreactor/tree/master) merged back into twisted, but written for PySide instead of PyQt4.

I'd additionally note that if the compatibility is as it is claimed, an end-user who wants to use the designed-for-PySide qtreactor with PyQt4 -- perhaps because they already have PyQt4 installed and do not have PySide installed -- could pretty trivially poke a thing into sys.modules['PySide.QtCore'] first before calling qt4reactor.install(). But of course that's nothing to do with Twisted.

Attachments (5)

qtdemo1.png (44.5 KB) - added by thijs 4 years ago.
qtreactor-3977.patch (14.3 KB) - added by michaelnt 4 years ago.
qtdemo2.png (170.4 KB) - added by thijs 4 years ago.
documentation-3977.patch (1.2 KB) - added by michaelnt 4 years ago.
Documentation patch
15759_15758.diff (2.1 KB) - added by michaelnt 4 years ago.

Download all attachments as: .zip

Change History (40)

comment:1 follow-up: Changed 5 years ago by exarkun

Sounds good.

We need to:

  • Figure out what the license is on qt4reactor. AIUI, a bunch of people have contributed to it, Glenn being the most recent developer. Hopefully they'll all be happy with MIT.
  • Get a buildslave set up with PySide installed to run Qt tests
  • Fix whatever test failures that slave points out (eg, hopefully there'll be one for <http://twistedmatrix.com/pipermail/twisted-python/2009-August/020267.html>, if not we might want to expand our test coverage a bit).
  • Gooooooooo

comment:2 follow-up: Changed 5 years ago by ghtdak

I'm cool with changing to whatever license makes sense. IIRC, the other contributers would include Therve, Itmar Shtull-Trauring and Gabe Rudy

Maintainer: U{Glenn H Tarbox, PhD<mailto:glenn@…>}
Previous maintainer: U{Itamar Shtull-Trauring<mailto:twisted@…>}
Original port to QT4: U{Gabe Rudy<mailto:rudy@…>}

The current repo is under the launchpad tx project. Not sure if any clones are up at github with fixes we'd want included.

comment:3 Changed 5 years ago by glyph

  • Owner changed from glyph to ghtdak

comment:4 in reply to: ↑ 2 Changed 4 years ago by sc

Replying to ghtdak:

I'm cool with changing to whatever license makes sense. IIRC, the other contributers would include Therve, Itmar Shtull-Trauring and Gabe Rudy

Maintainer: U{Glenn H Tarbox, PhD<mailto:glenn@…>}
Previous maintainer: U{Itamar Shtull-Trauring<mailto:twisted@…>}
Original port to QT4: U{Gabe Rudy<mailto:rudy@…>}

Following up on this mailing list thread as to the status of the qt4reactor and its licensing:

http://twistedmatrix.com/pipermail/twisted-python/2010-June/022457.html

It would seem PySide has reached the point where it should be a viable alternative to PyQt, without the license complications.

It is still necessary to attain permission to re-release the qt4reactor code under Twisted's MIT license however.

comment:5 Changed 4 years ago by Killarny

I've followed up a bit on the thread referenced by sc, and it appears that the qt4reactor has already been relicensed to something very permissive.

See: http://twistedmatrix.com/pipermail/twisted-python/2010-June/022470.html and https://github.com/ghtdak/qtreactor/blob/master/LICENSE

It seems that only the testing requirements outlined by exarkun above still remain for this ticket, before the code living at https://github.com/ghtdak/qtreactor can be merged into Twisted.

comment:6 Changed 4 years ago by michaelnt

  • Keywords review added

I've made some updates to the qtreactor at https://github.com/ghtdak/qtreactor and it now passes the complete Twisted test suite with the possible exception of UDP multicast.

It probably needs some more tests, the dpaste link in the email exarkun references can't be viewed anymore.

I've also added a win32event interface so this reactor can be used with serial ports on Windows.

comment:7 Changed 4 years ago by thijs

  • Cc thijs added

comment:8 Changed 4 years ago by michaelnt

I created a branch on github that has commits to

  • undo the removal of the original qtreactor,
  • update qtdemo.py to use Qt4, PySide and a WebKit view rather than plain html text
  • update qtreactor with the latest code from qt4reactor

https://github.com/michaelnt/twisted/commits/qt4reactor

comment:9 Changed 4 years ago by michaelnt

  • Owner ghtdak deleted

Added patch that re-integrates the qtreactor and updates the qtdemo app.

Removed the win32event interface as this might be better in a separate ticket.

comment:10 Changed 4 years ago by thijs

A coupe of coding standard issues to be resolved before this lands:

  • copyright header should be bumped from 2001-2004 to 2001-2011
  • don't add maintainer email addresses to the docstring to prevent spam and outdated info in the source code
  • docstrings on 3 lines instead of 2:
    """
    foo
    """
    

comment:11 follow-up: Changed 4 years ago by michaelnt

Patch update to address review comments from thijs:

  • Updated copyright
  • Tidied up docstrings
  • Removed maintainer names and emails

comment:12 in reply to: ↑ 11 Changed 4 years ago by thijs

Thanks for the updates. I'd like to try this out, but applying it to the trunk throws some errors:

thijs@ubuntu:~/workspaces/opensource/Twisted/trunk$ patch -p0 < qtreactor-3977.patch 
patching file b/doc/core/examples/qtdemo.py
patching file b/twisted/internet/qtreactor.py
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file b/twisted/internet/qtreactor.py.rej
patching file b/twisted/internet/test/reactormixins.py
Hunk #1 FAILED at 51.
1 out of 1 hunk FAILED -- saving rejects to file b/twisted/internet/test/reactormixins.py.rej
The next patch would delete the file a/twisted/plugins/twisted_qtstub.py,
which does not exist!  Assume -R? [n] 
Apply anyway? [n] y
patching file a/twisted/plugins/twisted_qtstub.py
Reversed (or previously applied) patch detected!  Assume -R? [n] 
Apply anyway? [n] 
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file a/twisted/plugins/twisted_qtstub.py.rej
patching file b/twisted/plugins/twisted_reactors.py
Hunk #1 FAILED at 36.
1 out of 1 hunk FAILED -- saving rejects to file b/twisted/plugins/twisted_reactors.py.rej

comment:13 Changed 4 years ago by michaelnt

The patch came from git so it has to be applied as

patch -p1 < qtreactor-3977.patch

comment:14 Changed 4 years ago by michaelnt

Updated the patch and checked that it will apply to trunk as

patch -p0 < qtreactor-3977.patch

comment:15 Changed 4 years ago by thijs

Thanks for the updates. I'm using Ubuntu 10.10 maverick and installed pyside 1.0beta4 in the stock ubuntu python2.6 and it runs fine (with some non-related info printed at the console):

thijs@ubuntu:~/workspaces/opensource/Twisted/trunk$ /usr/bin/python2.6 doc/core/examples/qtdemo.py 
/home/thijs/.gtkrc-2.0:11: Unable to locate image file in pixmap_path: "Arrows/arrow-up_.png"
/home/thijs/.gtkrc-2.0:15: Overlay image options specified without filename
/home/thijs/.gtkrc-2.0:21: Unable to locate image file in pixmap_path: "Arrows/arrow-up_.png"
/home/thijs/.gtkrc-2.0:25: Overlay image options specified without filename

Is it possible to auto-load the twisted site in the webkit panel, because it currently shows a blank page with the url in the addressbar, making it seem that it couldn't be loaded (although it does when you hit enter in the addressbar). See attached screenshot.

Changed 4 years ago by thijs

comment:16 Changed 4 years ago by thijs

pyflakes also reports some unused imports:

$ pyflakes qtdemo.py 
qtdemo.py:13: 'QObject' imported but unused
qtdemo.py:13: 'QCoreApplication' imported but unused
qtdemo.py:13: 'QSocketNotifier' imported but unused
qtdemo.py:13: 'QTimer' imported but unused
qtdemo.py:14: 'QEventLoop' imported but unused
qtdemo.py:15: 'QtCore' imported but unused

Changed 4 years ago by michaelnt

comment:17 follow-up: Changed 4 years ago by michaelnt

Updated patch

  • removed un-needed imports
  • qtdemo now loads default url on start up.

I think the .gtkrc warning are something to do with theming and are presumably wrong on your setup. The demo app doesn't have any icons so we should ignore these warnings.

Changed 4 years ago by thijs

comment:18 in reply to: ↑ 17 Changed 4 years ago by thijs

Replying to michaelnt:

I think the .gtkrc warning are something to do with theming and are presumably wrong on your setup. The demo app doesn't have any icons so we should ignore these warnings.

Totally.

Looks good here:


There's a couple of places that have list of reactors (in docstrings and documentation) that would also need an update.

Changed 4 years ago by michaelnt

Documentation patch

comment:19 follow-up: Changed 4 years ago by michaelnt

Added a patch that updates the documentation.

I searched for epoll as a way to discover reactor documentation or references in the source code and couldn't find anywhere else that needs updating.

comment:20 Changed 4 years ago by <automation>

comment:21 Changed 4 years ago by thijs

  • Author set to thijs
  • Branch set to branches/qt4reactor-pyside-3977

(In [30761]) Branching to 'qt4reactor-pyside-3977'

comment:22 Changed 4 years ago by thijs

(In [30827]) Apply qtreactor-3977.patch, refs #3977

comment:23 Changed 4 years ago by thijs

(In [30828]) Apply documentation-3977.patch, refs #3977

comment:24 Changed 4 years ago by thijs

(In [30829]) Update docs, refs #3977

comment:25 in reply to: ↑ 19 Changed 4 years ago by thijs

  • Author changed from thijs to michaelnt, thijs
  • Keywords review removed
  • Owner set to michaelnt

Replying to michaelnt:

Added a patch that updates the documentation.

I searched for epoll as a way to discover reactor documentation or references in the source code and couldn't find anywhere else that needs updating.

Thanks, I created a branch, applied the tickets and made some cosmetic changes. Could you also add a line to the Reactor Functionality table in the choosing-reactor howto? I'm not sure how stable it is, but tested on ubuntu.

comment:26 in reply to: ↑ 1 Changed 4 years ago by thijs

Replying to exarkun:

  • Get a buildslave set up with PySide installed to run Qt tests

Any chance you could supply a buildslave michaelnt?

comment:27 Changed 4 years ago by michaelnt

Update on progress.

The Good News is I have set up a buildslave

http://buildbot.twistedmatrix.com/builders/windows7-32-py2.7

The bad news is that

  • running trial with the select reactor some of the QtReactor tests fail, but are incorrectly marked as skips by trial.
  • Running trial with the qt reactor causes python to crash, and a dialog to get displayed by windows.

I'll keep looking into this.

Changed 4 years ago by michaelnt

comment:28 Changed 4 years ago by michaelnt

  • Keywords review added
  • Owner michaelnt deleted

All the tests should pass after this diff is applied if run against PySide 1.0 15759_15758.diff

Can someone apply this to the branch.

comment:29 Changed 4 years ago by exarkun

(In [31243]) Apply 15759_15758.diff

refs #3977

comment:30 follow-up: Changed 4 years ago by michaelnt

  • Keywords review removed
  • Owner set to michaelnt

Build results at http://buildbot.twistedmatrix.com/builders/windows7-32-py2.7/builds/79

One failure when running the default reactor, test_qt4reactor failing to import twisted_qtstub. When running the complete test suite using the qt reactor python crashes.

If I re-run the individual tests they pass, I suspect a bug in PySide.

comment:31 in reply to: ↑ 30 Changed 3 years ago by thijs

Replying to michaelnt:

Build results at http://buildbot.twistedmatrix.com/builders/windows7-32-py2.7/builds/79

One failure when running the default reactor, test_qt4reactor failing to import twisted_qtstub. When running the complete test suite using the qt reactor python crashes.

That stub was removed in this branch (as part of [attachment:qtreactor-3977.patch in r30827)? Also fails with the qt reactor..

If I re-run the individual tests they pass, I suspect a bug in PySide.

When running this locally the qt reactor python doesn't crash and the test suite completes with errors:

$ uname --all
Linux aspiration 3.0.0-16-generic #28-Ubuntu SMP Fri Jan 27 17:44:39 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux
$ python -c "from PySide import __version__ as v; print v"
1.1.0
$ trial --reactor=qt twisted
...
Ran 6897 tests in 76.736s

FAILED (skips=767, expectedFailures=18, failures=13, errors=11, successes=6089)

comment:32 Changed 3 years ago by thijs

(In [33611]) Merge forward, refs #3977

comment:33 Changed 3 years ago by thijs

  • Branch changed from branches/qt4reactor-pyside-3977 to branches/qt4reactor-pyside-3977-2

comment:34 follow-up: Changed 3 years ago by thijs

(In [33610]) Branching to 'qt4reactor-pyside-3977-2'

comment:35 in reply to: ↑ 34 Changed 4 weeks ago by ghtdak

Replying to thijs:

(In [33610]) Branching to 'qt4reactor-pyside-3977-2'

I've just done an upgrade / reorganization and it looks pretty stable. It passes all 10,00 + tests although whether the skips are correct will need to be vetted.

The QtReactor has been very thoroughly tested with PyQt4, somewhat thoroughly tested with PySide, and not at all on Qt5. However, there is one project using the reactor and Qt5 so I have high confidence I can get it running and reasonably high confidence it will work. How the reactor nails to Twisted would be, essentially, identical.

If there's interest, I could take this ticket on.

Note: See TracTickets for help on using tickets.