[ENH] Centralize virtual/physical $DISPLAYs

This PR addresses several (related) problems:

  - Makes `$DISPLAY` and the config `display_variable` optional (close #2055)
  - Should fix #1403 since xvfb-run is not used anymore.
  - Relates to #1400:
    - Will reduce its impact because now Xvfb is called only once and only if it is absolutely necessary
    - May make it worse in some cases when Xvfb fails to create a listener (root needed?).

This PR adds a `config.get_display()` which identifies what display should be used, and creates a virtual one if necessary. Also adds a few unit tests to the config object to make sure precedence is fulfilled.

Author: oesteban

nipype/interfaces/base.py

@@ -1010,32 +1010,6 @@ def _check_version_requirements(self, trait_object, raise_exception=True):
                                          version, max_ver))
         return unavailable_traits
 
-    def _run_wrapper(self, runtime):
-        sysdisplay = os.getenv('DISPLAY')
-        if self._redirect_x:
-            try:
-                from xvfbwrapper import Xvfb
-            except ImportError:
-                iflogger.error('Xvfb wrapper could not be imported')
-                raise
-
-            vdisp = Xvfb(nolisten='tcp')
-            vdisp.start()
-            try:
-                vdisp_num = vdisp.new_display
-            except AttributeError:  # outdated version of xvfbwrapper
-                vdisp_num = vdisp.vdisplay_num
-
-            iflogger.info('Redirecting X to :%d' % vdisp_num)
-            runtime.environ['DISPLAY'] = ':%d' % vdisp_num
-
-        runtime = self._run_interface(runtime)
-
-        if self._redirect_x:
-            vdisp.stop()
-
-        return runtime
-
     def _run_interface(self, runtime):
         """ Core function that executes interface
         """
@@ -1071,6 +1045,9 @@ def run(self, **inputs):
 
         # initialize provenance tracking
         env = deepcopy(dict(os.environ))
+        if self._redirect_x:
+            env['DISPLAY'] = config.get_display()
+
         runtime = Bunch(cwd=os.getcwd(),
                         returncode=None,
                         duration=None,
@@ -1080,8 +1057,9 @@ def run(self, **inputs):
                         platform=platform.platform(),
                         hostname=platform.node(),
                         version=self.version)
+
         try:
-            runtime = self._run_wrapper(runtime)
+            runtime = self._run_interface(runtime)
             outputs = self.aggregate_outputs(runtime)
             runtime.endTime = dt.isoformat(dt.utcnow())
             timediff = parseutc(runtime.endTime) - parseutc(runtime.startTime)
@@ -1446,7 +1424,7 @@ def get_max_resources_used(pid, mem_mb, num_threads, pyfunc=False):
     return mem_mb, num_threads
 
 
-def run_command(runtime, output=None, timeout=0.01, redirect_x=False):
+def run_command(runtime, output=None, timeout=0.01):
     """Run a command, read stdout and stderr, prefix with timestamp.
 
     The returned runtime contains a merged stdout+stderr log with timestamps
@@ -1458,13 +1436,6 @@ def run_command(runtime, output=None, timeout=0.01, redirect_x=False):
     # Init variables
     PIPE = subprocess.PIPE
     cmdline = runtime.cmdline
-
-    if redirect_x:
-        exist_xvfb, _ = _exists_in_path('xvfb-run', runtime.environ)
-        if not exist_xvfb:
-            raise RuntimeError('Xvfb was not found, X redirection aborted')
-        cmdline = 'xvfb-run -a ' + cmdline
-
     env = _canonicalize_env(runtime.environ)
 
     default_encoding = locale.getdefaultlocale()[1]
@@ -1727,14 +1698,6 @@ def help(cls, returnhelp=False):
             print(allhelp)
 
     def _get_environ(self):
-        out_environ = {}
-        if not self._redirect_x:
-            try:
-                display_var = config.get('execution', 'display_variable')
-                out_environ = {'DISPLAY': display_var}
-            except NoOptionError:
-                pass
-        iflogger.debug(out_environ)
         if isdefined(self.inputs.environ):
             out_environ.update(self.inputs.environ)
         return out_environ
@@ -1754,10 +1717,6 @@ def version_from_command(self, flag='-v'):
             o, e = proc.communicate()
             return o
 
-    def _run_wrapper(self, runtime):
-        runtime = self._run_interface(runtime)
-        return runtime
-
     def _run_interface(self, runtime, correct_return_codes=(0,)):
         """Execute command via subprocess
 
@@ -1785,8 +1744,7 @@ def _run_interface(self, runtime, correct_return_codes=(0,)):
         setattr(runtime, 'command_path', cmd_path)
         setattr(runtime, 'dependencies', get_dependencies(executable_name,
                                                           runtime.environ))
-        runtime = run_command(runtime, output=self.inputs.terminal_output,
-                              redirect_x=self._redirect_x)
+        runtime = run_command(runtime, output=self.inputs.terminal_output)
         if runtime.returncode is None or \
                 runtime.returncode not in correct_return_codes:
             self.raise_exception(runtime)

nipype/utils/config.py

@@ -44,7 +44,6 @@
 [execution]
 create_report = true
 crashdump_dir = %s
-display_variable = :1
 hash_method = timestamp
 job_finished_timeout = 5
 keep_inputs = false
@@ -82,7 +81,8 @@ def mkdir_p(path):
 
 
 class NipypeConfig(object):
-    """Base nipype config class
+    """
+    Base nipype config class
     """
 
     def __init__(self, *args, **kwargs):
@@ -91,6 +91,7 @@ def __init__(self, *args, **kwargs):
         config_file = os.path.join(config_dir, 'nipype.cfg')
         self.data_file = os.path.join(config_dir, 'nipype.json')
         self._config.readfp(StringIO(default_cfg))
+        self._display = None
         if os.path.exists(config_dir):
             self._config.read([config_file, 'nipype.cfg'])
 
@@ -172,3 +173,50 @@ def update_matplotlib(self):
     def enable_provenance(self):
         self._config.set('execution', 'write_provenance', 'true')
         self._config.set('execution', 'hash_method', 'content')
+
+    def get_display(self):
+        """Returns the first display available"""
+
+        # Check if an Xorg server is listening
+        # import subprocess as sp
+        # if not hasattr(sp, 'DEVNULL'):
+        #     setattr(sp, 'DEVNULL', os.devnull)
+        # x_listening = bool(sp.call('ps au | grep -v grep | grep -i xorg',
+        #                    shell=True, stdout=sp.DEVNULL))
+
+        if self._display is not None:
+            return ':%d' % self._display.vdisplay_num
+
+        sysdisplay = None
+        if self._config.has_option('execution', 'display_variable'):
+            sysdisplay = self._config.get('execution', 'display_variable')
+
+        sysdisplay = sysdisplay or os.getenv('DISPLAY')
+        if sysdisplay:
+            from collections import namedtuple
+            def _mock():
+                pass
+
+            # Store a fake Xvfb object
+            ndisp = int(sysdisplay.split(':')[-1])
+            Xvfb = namedtuple('Xvfb', ['vdisplay_num', 'stop'])
+            self._display = Xvfb(ndisp, _mock)
+            return sysdisplay
+
+        else:
+            try:
+                from xvfbwrapper import Xvfb
+            except ImportError:
+                raise RuntimeError(
+                    'A display server was required, but $DISPLAY is not defined '
+                    ' and Xvfb could not be imported.')
+
+            self._display = Xvfb(nolisten='tcp')
+            self._display.start()
+
+            # Older versions of Xvfb used vdisplay_num
+            if hasattr(self._display, 'new_display'):
+                setattr(self._display, 'vdisplay_num',
+                        self._display.new_display)
+
+            return ':%d' % self._display.vdisplay_num

nipype/utils/tests/test_config.py

@@ -0,0 +1,41 @@
+# -*- coding: utf-8 -*-
+# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*-
+# vi: set ft=python sts=4 ts=4 sw=4 et:
+from __future__ import print_function, division, unicode_literals, absolute_import
+import os
+import pytest
+from nipype import config
+
+@pytest.mark.parametrize('dispnum', range(5))
+def test_display_config(monkeypatch, dispnum):
+    """Check that the display_variable option is used"""
+    config._display = None
+    dispstr = ':%d' % dispnum
+    config.set('execution', 'display_variable', dispstr)
+    monkeypatch.delitem(os.environ, 'DISPLAY', raising=False)
+    assert config.get_display() == config.get('execution', 'display_variable')
+
+@pytest.mark.parametrize('dispnum', range(5))
+def test_display_system(monkeypatch, dispnum):
+    """Check that when only a $DISPLAY is defined, it is used"""
+    config._display = None
+    config._config.remove_option('execution', 'display_variable')
+    dispstr = ':%d' % dispnum
+    monkeypatch.setitem(os.environ, 'DISPLAY', dispstr)
+    assert config.get_display() == dispstr
+
+def test_display_config_and_system(monkeypatch):
+    """Check that when only both config and $DISPLAY are defined, the config takes precedence"""
+    config._display = None
+    dispstr = ':10'
+    config.set('execution', 'display_variable', dispstr)
+    monkeypatch.setitem(os.environ, 'DISPLAY', dispstr)
+    assert config.get_display() == dispstr
+
+def test_display_noconfig_nosystem(monkeypatch):
+    """Check that when no display is specified, a virtual Xvfb is used"""
+    config._display = None
+    if config.has_option('execution', 'display_variable'):
+        config._config.remove_option('execution', 'display_variable')
+    monkeypatch.delitem(os.environ, 'DISPLAY', raising=False)
+    assert int(config.get_display().split(':')[-1]) > 80