Prevent Qt4 from stopping the interpreter #1905

Merged
merged 5 commits into from Apr 19, 2013

Conversation

Projects
None yet
4 participants
@jschueller
Contributor

jschueller commented Apr 15, 2013

Hi,

This commit prevents Qt4 from stopping the interpreter:
If there's no X server available, or if you export DISPLAY="", and try to initialize a QApp, Qt4 stops:

: cannot connect to X server

It's a non-feature of Qt4 to not use exceptions to be lightweight.
But in python, that's not really the spirit, we'd expect a python backtrace, so here's what the patch does:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/schueller/projects/matplotlib/install/lib/python2.7/site-packages/matplotlib-1.3.x-py2.7-linux-x86_64.egg/matplotlib/pyplot.py", line 348, in figure
    **kwargs)
 ...
    raise RuntimeError( 'Qt4 failed to initialize ' + p.stderr.readline().rstrip() )     
RuntimeError: Qt4 failed to initialize : cannot connect to X server

And it's more like the other backends do:

_tkinter.TclError: couldn't connect to display ""

Regards.

(EDIT by @pelson: Added code blocks)

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Apr 15, 2013

Owner

Wow. That's kind of obnoxious of PyQt and PySide ::.

This should definitely be tested on MS Windows: subprocess can be more picky there given the more limited forking on that platform. (Though on the other hand, it's harder to not have a GUI environment on Windows, so practically we could just skip this new test if on Windows).

I'm not crazy about solving this problem with subprocess: it will increase startup time and memory by a whole second Python interpreter, plus most of matplotlib since it imports qt_compat. Particularly for people who run plot-creating batch scripts this is a problem -- I know they should switch to the Agg backend for that, but many users don't know or remember to do that.

For Linux, at least, we could just check to the existence and validity of the DISPLAY environment variable. Obviously on a Mac, X11 isn't required, but is there a similar failure case there? I just to want to solve a small problem (which I agree is a problem) with a jackhammer if a philips head screwdriver might do.

Owner

mdboom commented Apr 15, 2013

Wow. That's kind of obnoxious of PyQt and PySide ::.

This should definitely be tested on MS Windows: subprocess can be more picky there given the more limited forking on that platform. (Though on the other hand, it's harder to not have a GUI environment on Windows, so practically we could just skip this new test if on Windows).

I'm not crazy about solving this problem with subprocess: it will increase startup time and memory by a whole second Python interpreter, plus most of matplotlib since it imports qt_compat. Particularly for people who run plot-creating batch scripts this is a problem -- I know they should switch to the Agg backend for that, but many users don't know or remember to do that.

For Linux, at least, we could just check to the existence and validity of the DISPLAY environment variable. Obviously on a Mac, X11 isn't required, but is there a similar failure case there? I just to want to solve a small problem (which I agree is a problem) with a jackhammer if a philips head screwdriver might do.

@jschueller

This comment has been minimized.

Show comment Hide comment
@jschueller

jschueller Apr 16, 2013

Contributor

I agree, starting a process is a bit heavy.
I modified the code so as to just check that DISPLAY exists and contains a ":".
For non-linux platforms I assumed the problem existed but I did not verify.

Contributor

jschueller commented Apr 16, 2013

I agree, starting a process is a bit heavy.
I modified the code so as to just check that DISPLAY exists and contains a ":".
For non-linux platforms I assumed the problem existed but I did not verify.

@mdboom

View changes

lib/matplotlib/backends/backend_qt4.py
@@ -23,6 +23,8 @@
from qt4_compat import QtCore, QtGui, _getSaveFileName, __version__
+import subprocess

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Apr 16, 2013

Owner

Minor point -- this can probably be removed since it's no longer being used.

@mdboom

mdboom Apr 16, 2013

Owner

Minor point -- this can probably be removed since it's no longer being used.

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Apr 16, 2013

Owner

This is much better. Thanks for your patience on this -- I have one more suggestion.

I think that the sys.platform == 'linux' is not exactly the right check. You could be using the framebuffer build of Qt on Linux, for example, in which case there would be no DISPLAY, but we would expect it to work. Likewise, we could be using the X11 build of Qt on a Mac. I think what we really want to check for is what build of Qt is installed. I couldn't find any way to obtain this information, but there are certain members that only exist in an X11 build, for example:

https://deptinfo-ensip.univ-poitiers.fr/ENS/pyside-docs/PySide/QtGui/QX11Info.html

Maybe we check for the existence of that, and then do the DISPLAY env variable check? (Or maybe there's a better way and my Google-foo isn't strong enough today).

Owner

mdboom commented Apr 16, 2013

This is much better. Thanks for your patience on this -- I have one more suggestion.

I think that the sys.platform == 'linux' is not exactly the right check. You could be using the framebuffer build of Qt on Linux, for example, in which case there would be no DISPLAY, but we would expect it to work. Likewise, we could be using the X11 build of Qt on a Mac. I think what we really want to check for is what build of Qt is installed. I couldn't find any way to obtain this information, but there are certain members that only exist in an X11 build, for example:

https://deptinfo-ensip.univ-poitiers.fr/ENS/pyside-docs/PySide/QtGui/QX11Info.html

Maybe we check for the existence of that, and then do the DISPLAY env variable check? (Or maybe there's a better way and my Google-foo isn't strong enough today).

@jschueller

This comment has been minimized.

Show comment Hide comment
@jschueller

jschueller Apr 17, 2013

Contributor

Hi,

I tried this;

            # check for DISPLAY env variable on X11 build of Qt
            if hasattr(QtGui, "QX11Info"):
                display = os.environ.get('DISPLAY')
                if (display is None) or (not ':' in display):
                    raise RuntimeError('Invalid DISPLAY variable')

I also removed the subprocess import.
It still needs some testing on non-linux platforms
What do you think ?

J.

Contributor

jschueller commented Apr 17, 2013

Hi,

I tried this;

            # check for DISPLAY env variable on X11 build of Qt
            if hasattr(QtGui, "QX11Info"):
                display = os.environ.get('DISPLAY')
                if (display is None) or (not ':' in display):
                    raise RuntimeError('Invalid DISPLAY variable')

I also removed the subprocess import.
It still needs some testing on non-linux platforms
What do you think ?

J.

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Apr 17, 2013

Owner

Looks great. @mdehoon, @pelson, @cgohlke: Any takers to test this on Windows and Mac OS-X?

Owner

mdboom commented Apr 17, 2013

Looks great. @mdehoon, @pelson, @cgohlke: Any takers to test this on Windows and Mac OS-X?

@jschueller

This comment has been minimized.

Show comment Hide comment
@jschueller

jschueller Apr 17, 2013

Contributor

Hi,

I used a regex to verify DISPLAY:

if display is None or not re.search(':\d', display):

since:
http://unixhelp.ed.ac.uk/CGI/man-cgi?ssh+1

J.

Contributor

jschueller commented Apr 17, 2013

Hi,

I used a regex to verify DISPLAY:

if display is None or not re.search(':\d', display):

since:
http://unixhelp.ed.ac.uk/CGI/man-cgi?ssh+1

J.

@mdehoon

This comment has been minimized.

Show comment Hide comment
@mdehoon

mdehoon Apr 17, 2013

Contributor

Sorry, I only have a non-X11 build of Qt4 on my Mac, so I can't test this.

Contributor

mdehoon commented Apr 17, 2013

Sorry, I only have a non-X11 build of Qt4 on my Mac, so I can't test this.

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Apr 18, 2013

Owner

@mdehoon: It actually would be more valuable if you could test this with the native build of Qt4 on your Mac. In particular, it's an open question as to whether the method used here to determine if we're using an X11 Qt4 is correct or not. So if you wouldn't mind trying it with a native Qt4 build, that would be very helpful.

Owner

mdboom commented Apr 18, 2013

@mdehoon: It actually would be more valuable if you could test this with the native build of Qt4 on your Mac. In particular, it's an open question as to whether the method used here to determine if we're using an X11 Qt4 is correct or not. So if you wouldn't mind trying it with a native Qt4 build, that would be very helpful.

@mdehoon

This comment has been minimized.

Show comment Hide comment
@mdehoon

mdehoon Apr 18, 2013

Contributor

It seems to work fine: With the native Qt4 build on my Mac,

from PyQt4 import QtGui
hasattr(QtGui, "QX11Info")

returns False.

Contributor

mdehoon commented Apr 18, 2013

It seems to work fine: With the native Qt4 build on my Mac,

from PyQt4 import QtGui
hasattr(QtGui, "QX11Info")

returns False.

@jschueller

This comment has been minimized.

Show comment Hide comment
@jschueller

jschueller Apr 19, 2013

Contributor

Hi,

I just tried hasattr(QtGui, "QX11Info") on windows with PyQt4, and it's OK too.

J.

Contributor

jschueller commented Apr 19, 2013

Hi,

I just tried hasattr(QtGui, "QX11Info") on windows with PyQt4, and it's OK too.

J.

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Apr 19, 2013

Owner

Great. Merging. Thanks for the patch.

Owner

mdboom commented Apr 19, 2013

Great. Merging. Thanks for the patch.

mdboom added a commit that referenced this pull request Apr 19, 2013

Merge pull request #1905 from jschueller/master
Prevent Qt4 from stopping the interpreter

@mdboom mdboom merged commit 091776d into matplotlib:master Apr 19, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment