Skip to content
This repository

check for complete pyside presence before trying to import #2757

Merged
merged 1 commit into from over 1 year ago

4 participants

Julian Taylor Min RK Bradley M. Froehle Thomas Kluyver
Julian Taylor
Collaborator

importing pyside partially and then falling back to pyqt causes a crash
in sip (see gh-1431)
To avoid it try to locate all modules before the import and should that
fail print a warning.

debian splits pyside into many small packages so sometimes people end up with incomplete pyside installations.

Julian Taylor check for complete pyside presence before trying to import
importing pyside partially and then falling back to pyqt causes a crash
in sip (see gh-1431)
To avoid it try to locate all modules before the import and should that
fail print a warning.
9106259
Thomas Kluyver takluyver commented on the diff
IPython/external/qt.py
@@ -35,6 +44,11 @@ def prepare_pyqt4():
35 44 prepare_pyqt4()
36 45 import PyQt4
37 46 from PyQt4 import QtCore, QtGui, QtSvg
  47 + if pyside_found:
5
Thomas Kluyver Collaborator

Under what condition does it reach this point with pyside_found=True? It looks like, if it can't find the modules it needs, ImportError is raised while pyside_found is still False.

Julian Taylor Collaborator

find_module does not load the module.
It is possible that one of these module is on the disk but can't be loaded, if the others load it will likely still crash so a warning is printed.

Thomas Kluyver Collaborator

What would prevent them being loaded? Permissions error? How could that arise? I think it's OK to handle these corner cases in the main code base, I just want to work out what it is we're dealing with.

Am I right in thinking that you're handling two different cases here? The section above using find_module avoids loading the PySide modules unless all the ones we need are present, so that we can fall back to PyQt4 if some are missing. That bit I think I follow. This second addition I'm more hazy about.

Julian Taylor Collaborator

its pretty paranoid, but plenty things could happen e.g. undefined symbols due to linking errors, all kind of issues due to binary incompatibilities.
but as qt is a pretty core package if its broken you probably have bigger issues than ipython then, so the warning could be dropped.

Thomas Kluyver Collaborator

That's fair enough. I guess my concern is that if we don't know a likely reason for this result, any suggestions in the message are liable to be unhelpful. But I guess a general "your dodgy PySide installation is a likely culprit" is better than nothing.

I'm happy for this to go in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Julian Taylor
Collaborator

as this is a debian only problem I'm fine with carrying that patch in the package alone (or change the package to depend on the complete python-pyside metapackage)

Min RK
Owner

@takluyver I recommended that @juliantaylor open a PR, even if it only ends up as a debian patch, just so we have a record of it. The disadvantage of merging it is we are taking responsibility for a package-management issue. The advantage is this issue can still come up for IPython installed manually on a debian system with incomplete pyside.

Bradley M. Froehle
Collaborator

I'm indifferent to the proposed patch. Go ahead and merge it if you like.

Thomas Kluyver
Collaborator

We all seem to be ambivalent about merging this. On the plus side, users who install IPython on Debian-based systems outside apt would benefit from it. The downside would be a little extra complexity to handle an odd corner case.

I suggest we merge it. The complexity would exist anyway, just as a Debian patch, and I think it's a good idea to minimise the amount of patches we make distributions apply.

@bfroehle , @minrk : any objections? If not, I'll merge this tomorrow.

Bradley M. Froehle
Collaborator

No, it's fine with me.

Min RK
Owner

Sounds good to me.

Bradley M. Froehle bfroehle merged commit de2743b into from
Min RK minrk referenced this pull request from a commit
Min RK minrk Backport PR #2757: check for complete pyside presence before trying t…
…o import

importing pyside partially and then falling back to pyqt causes a crash
in sip (see gh-1431)
To avoid it try to locate all modules before the import and should that
fail print a warning.

debian splits pyside into many small packages so sometimes people end up with incomplete pyside installations.
b7de089
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Jan 07, 2013
Julian Taylor check for complete pyside presence before trying to import
importing pyside partially and then falling back to pyqt causes a crash
in sip (see gh-1431)
To avoid it try to locate all modules before the import and should that
fail print a warning.
9106259
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 14 additions and 0 deletions. Show diff stats Hide diff stats

  1. +14 0 IPython/external/qt.py
14 IPython/external/qt.py
@@ -23,11 +23,20 @@ def prepare_pyqt4():
23 23 # Select Qt binding, using the QT_API environment variable if available.
24 24 QT_API = os.environ.get('QT_API')
25 25 if QT_API is None:
  26 + pyside_found = False
26 27 try:
27 28 import PySide
28 29 if PySide.__version__ < '1.0.3':
29 30 # old PySide, fallback on PyQt
30 31 raise ImportError
  32 + # we can't import an incomplete pyside and pyqt4
  33 + # this will cause a crash in sip (#1431)
  34 + # check for complete presence before importing
  35 + import imp
  36 + imp.find_module("QtCore", PySide.__path__)
  37 + imp.find_module("QtGui", PySide.__path__)
  38 + imp.find_module("QtSvg", PySide.__path__)
  39 + pyside_found = True
31 40 from PySide import QtCore, QtGui, QtSvg
32 41 QT_API = QT_API_PYSIDE
33 42 except ImportError:
@@ -35,6 +44,11 @@ def prepare_pyqt4():
35 44 prepare_pyqt4()
36 45 import PyQt4
37 46 from PyQt4 import QtCore, QtGui, QtSvg
  47 + if pyside_found:
  48 + print "WARNING: PySide installation incomplete and PyQt4 " \
  49 + "present.\nThis will likely crash, please install " \
  50 + "PySide completely, remove PySide or PyQt4 or set " \
  51 + "the QT_API environment variable to pyqt or pyside"
38 52 if QtCore.PYQT_VERSION_STR < '4.7':
39 53 # PyQt 4.6 has issues with null strings returning as None
40 54 raise ImportError

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.