Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

check for complete pyside presence before trying to import #2757

Merged
merged 1 commit into from

4 participants

@juliantaylor
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.

@juliantaylor juliantaylor 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
@takluyver takluyver commented on the diff
IPython/external/qt.py
@@ -35,6 +44,11 @@ def prepare_pyqt4():
prepare_pyqt4()
import PyQt4
from PyQt4 import QtCore, QtGui, QtSvg
+ if pyside_found:
@takluyver Owner

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.

@juliantaylor 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.

@takluyver Owner

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.

@juliantaylor 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.

@takluyver Owner

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
@juliantaylor
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)

@minrk
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.

@bfroehle

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

@takluyver
Owner

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.

@bfroehle

No, it's fine with me.

@minrk
Owner

Sounds good to me.

@bfroehle bfroehle merged commit de2743b into from
@minrk minrk referenced this pull request from a commit
@minrk 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
Commits on Jan 7, 2013
  1. @juliantaylor

    check for complete pyside presence before trying to import

    juliantaylor authored
    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.
This page is out of date. Refresh to see the latest.
Showing with 14 additions and 0 deletions.
  1. +14 −0 IPython/external/qt.py
View
14 IPython/external/qt.py
@@ -23,11 +23,20 @@ def prepare_pyqt4():
# Select Qt binding, using the QT_API environment variable if available.
QT_API = os.environ.get('QT_API')
if QT_API is None:
+ pyside_found = False
try:
import PySide
if PySide.__version__ < '1.0.3':
# old PySide, fallback on PyQt
raise ImportError
+ # we can't import an incomplete pyside and pyqt4
+ # this will cause a crash in sip (#1431)
+ # check for complete presence before importing
+ import imp
+ imp.find_module("QtCore", PySide.__path__)
+ imp.find_module("QtGui", PySide.__path__)
+ imp.find_module("QtSvg", PySide.__path__)
+ pyside_found = True
from PySide import QtCore, QtGui, QtSvg
QT_API = QT_API_PYSIDE
except ImportError:
@@ -35,6 +44,11 @@ def prepare_pyqt4():
prepare_pyqt4()
import PyQt4
from PyQt4 import QtCore, QtGui, QtSvg
+ if pyside_found:
@takluyver Owner

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.

@juliantaylor 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.

@takluyver Owner

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.

@juliantaylor 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.

@takluyver Owner

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
+ print "WARNING: PySide installation incomplete and PyQt4 " \
+ "present.\nThis will likely crash, please install " \
+ "PySide completely, remove PySide or PyQt4 or set " \
+ "the QT_API environment variable to pyqt or pyside"
if QtCore.PYQT_VERSION_STR < '4.7':
# PyQt 4.6 has issues with null strings returning as None
raise ImportError
Something went wrong with that request. Please try again.