From 1a35f4425d8d75d8c988041d6a728df5c8a715a9 Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 9 Jan 2018 14:20:32 -0800 Subject: [PATCH 01/10] [FIX] Errors parsing ``$DISPLAY`` The format for ``$DISPLAY`` is ``:[.]``. When screen is specified, the config object fails. This patch fixes #2362 --- nipype/utils/config.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nipype/utils/config.py b/nipype/utils/config.py index c02be71f64..a5346869e4 100644 --- a/nipype/utils/config.py +++ b/nipype/utils/config.py @@ -305,10 +305,10 @@ def get_display(self): def _mock(): pass - # Store a fake Xvfb object - ndisp = int(sysdisplay.split(':')[-1]) + # Store a fake Xvfb object. Format - :[.] + ndisp = sysdisplay.split(':')[-1].split('.')[0] Xvfb = namedtuple('Xvfb', ['new_display', 'stop']) - self._display = Xvfb(ndisp, _mock) + self._display = Xvfb(int(ndisp), _mock) return sysdisplay else: if 'darwin' in sys.platform: From e1ef2e545563f4ff2952e0540553edb0e6e681ef Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 9 Jan 2018 14:30:15 -0800 Subject: [PATCH 02/10] add test --- nipype/utils/tests/test_config.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/nipype/utils/tests/test_config.py b/nipype/utils/tests/test_config.py index 7684bdd55e..20c99b3933 100644 --- a/nipype/utils/tests/test_config.py +++ b/nipype/utils/tests/test_config.py @@ -27,6 +27,17 @@ vdisplay_num=2010) +@pytest.mark.parametrize('disp_var', [':0', 'localhost:0', 'localhost:0.1']) +def test_display_config_and_system(monkeypatch, disp_var): + """Check that when $DISPLAY is defined, the display is correctly parsed""" + config._display = None + dispstr = ':0' + monkeypatch.setitem(os.environ, 'DISPLAY', disp_var) + assert config.get_display() == dispstr + # Test that it was correctly cached + assert config.get_display() == dispstr + + @pytest.mark.parametrize('dispnum', range(5)) def test_display_config(monkeypatch, dispnum): """Check that the display_variable option is used ($DISPLAY not set)""" From 6a9bc463ba8099b16d91dacd8dc7aed4267a2540 Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 9 Jan 2018 14:30:30 -0800 Subject: [PATCH 03/10] Update CHANGES --- CHANGES | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index 01a09b735a..cf057f6522 100644 --- a/CHANGES +++ b/CHANGES @@ -1,7 +1,8 @@ Upcoming release (0.14.1) ================ -* MAINT: Cleaning / simplify ``Node`` (https://github.com/nipy/nipype/pull/#2325) +* FIX: Errors parsing ``$DISPLAY`` (https://github.com/nipy/nipype/pull/2363) +* MAINT: Cleaning / simplify ``Node`` (https://github.com/nipy/nipype/pull/2325) 0.14.0 (November 29, 2017) ========================== From 2383177c27879833451fcb817aaf090089c7b0db Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 9 Jan 2018 16:38:23 -0800 Subject: [PATCH 04/10] fix test name --- nipype/utils/tests/test_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nipype/utils/tests/test_config.py b/nipype/utils/tests/test_config.py index 20c99b3933..3a6bebb0c5 100644 --- a/nipype/utils/tests/test_config.py +++ b/nipype/utils/tests/test_config.py @@ -28,7 +28,7 @@ @pytest.mark.parametrize('disp_var', [':0', 'localhost:0', 'localhost:0.1']) -def test_display_config_and_system(monkeypatch, disp_var): +def test_display_parse(monkeypatch, disp_var): """Check that when $DISPLAY is defined, the display is correctly parsed""" config._display = None dispstr = ':0' From fbac4ee965c17c4e26f7a6fa802a46be06466916 Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 9 Jan 2018 20:52:24 -0800 Subject: [PATCH 05/10] fix implementation of get_display() --- nipype/utils/config.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/nipype/utils/config.py b/nipype/utils/config.py index a5346869e4..99824f510b 100644 --- a/nipype/utils/config.py +++ b/nipype/utils/config.py @@ -309,7 +309,7 @@ def _mock(): ndisp = sysdisplay.split(':')[-1].split('.')[0] Xvfb = namedtuple('Xvfb', ['new_display', 'stop']) self._display = Xvfb(int(ndisp), _mock) - return sysdisplay + return self.get_display() else: if 'darwin' in sys.platform: raise RuntimeError( @@ -336,8 +336,7 @@ def _mock(): if not hasattr(self._display, 'new_display'): setattr(self._display, 'new_display', self._display.vdisplay_num) - - return ':%d' % self._display.new_display + return self.get_display() def stop_display(self): """Closes the display if started""" From 4dc5896ebb5a027c82e79c99622228c9539ec000 Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 9 Jan 2018 21:39:39 -0800 Subject: [PATCH 06/10] use mokeypatch.setenv --- nipype/utils/tests/test_config.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/nipype/utils/tests/test_config.py b/nipype/utils/tests/test_config.py index 3a6bebb0c5..4355239556 100644 --- a/nipype/utils/tests/test_config.py +++ b/nipype/utils/tests/test_config.py @@ -32,7 +32,7 @@ def test_display_parse(monkeypatch, disp_var): """Check that when $DISPLAY is defined, the display is correctly parsed""" config._display = None dispstr = ':0' - monkeypatch.setitem(os.environ, 'DISPLAY', disp_var) + monkeypatch.setenv('DISPLAY', disp_var) assert config.get_display() == dispstr # Test that it was correctly cached assert config.get_display() == dispstr @@ -56,7 +56,7 @@ def test_display_system(monkeypatch, dispnum): config._display = None config._config.remove_option('execution', 'display_variable') dispstr = ':%d' % dispnum - monkeypatch.setitem(os.environ, 'DISPLAY', dispstr) + monkeypatch.setenv('DISPLAY', dispstr) assert config.get_display() == dispstr # Test that it was correctly cached assert config.get_display() == dispstr @@ -67,7 +67,7 @@ def test_display_config_and_system(monkeypatch): config._display = None dispstr = ':10' config.set('execution', 'display_variable', dispstr) - monkeypatch.setitem(os.environ, 'DISPLAY', ':0') + monkeypatch.setenv('DISPLAY', ':0') assert config.get_display() == dispstr # Test that it was correctly cached assert config.get_display() == dispstr @@ -93,7 +93,7 @@ def test_display_empty_patched(monkeypatch): config._display = None if config.has_option('execution', 'display_variable'): config._config.remove_option('execution', 'display_variable') - monkeypatch.setitem(os.environ, 'DISPLAY', '') + monkeypatch.setenv('DISPLAY', '') monkeypatch.setitem(sys.modules, 'xvfbwrapper', xvfbpatch) assert config.get_display() == ':2010' # Test that it was correctly cached @@ -123,7 +123,7 @@ def test_display_empty_patched_oldxvfbwrapper(monkeypatch): config._display = None if config.has_option('execution', 'display_variable'): config._config.remove_option('execution', 'display_variable') - monkeypatch.setitem(os.environ, 'DISPLAY', '') + monkeypatch.setenv('DISPLAY', '') monkeypatch.setitem(sys.modules, 'xvfbwrapper', xvfbpatch_old) assert config.get_display() == ':2010' # Test that it was correctly cached @@ -152,7 +152,7 @@ def test_display_empty_notinstalled(monkeypatch): config._display = None if config.has_option('execution', 'display_variable'): config._config.remove_option('execution', 'display_variable') - monkeypatch.setitem(os.environ, 'DISPLAY', '') + monkeypatch.setenv('DISPLAY', '') monkeypatch.setitem(sys.modules, 'xvfbwrapper', None) with pytest.raises(RuntimeError): config.get_display() @@ -183,7 +183,7 @@ def test_display_empty_installed(monkeypatch): config._display = None if config.has_option('execution', 'display_variable'): config._config.remove_option('execution', 'display_variable') - monkeypatch.setitem(os.environ, 'DISPLAY', '') + monkeypatch.setenv('DISPLAY', '') newdisp = config.get_display() assert int(newdisp.split(':')[-1]) > 1000 # Test that it was correctly cached From 5de504afda048626269c9ed176f9c4e5cc5a405d Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 9 Jan 2018 22:01:06 -0800 Subject: [PATCH 07/10] fix test for Travis and Circle --- nipype/utils/tests/test_config.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/nipype/utils/tests/test_config.py b/nipype/utils/tests/test_config.py index 4355239556..86c0b7b787 100644 --- a/nipype/utils/tests/test_config.py +++ b/nipype/utils/tests/test_config.py @@ -32,10 +32,15 @@ def test_display_parse(monkeypatch, disp_var): """Check that when $DISPLAY is defined, the display is correctly parsed""" config._display = None dispstr = ':0' - monkeypatch.setenv('DISPLAY', disp_var) + old_disp = os.getenv('DISPLAY') + os.environ['DISPLAY'] = disp_var assert config.get_display() == dispstr # Test that it was correctly cached assert config.get_display() == dispstr + if old_disp is not None: + os.environ['DISPLAY'] = old_disp + else: + del os.environ['DISPLAY'] @pytest.mark.parametrize('dispnum', range(5)) From 22c1a2783b02ed594d25dc742645f3de2d5a6067 Mon Sep 17 00:00:00 2001 From: oesteban Date: Thu, 18 Jan 2018 14:41:24 -0800 Subject: [PATCH 08/10] update travis --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index 08d9234675..9fe7b3b080 100644 --- a/.travis.yml +++ b/.travis.yml @@ -13,6 +13,7 @@ env: - INSTALL_DEB_DEPENDECIES=true NIPYPE_EXTRAS="doc,tests,fmri,profiler,duecredit" CI_SKIP_TEST=1 - INSTALL_DEB_DEPENDECIES=true NIPYPE_EXTRAS="doc,tests,fmri,profiler" PIP_FLAGS="--pre" CI_SKIP_TEST=1 before_install: +- unset DISPLAY - function apt_inst { if $INSTALL_DEB_DEPENDECIES; then sudo rm -rf /dev/shm; fi && if $INSTALL_DEB_DEPENDECIES; then sudo ln -s /run/shm /dev/shm; fi && From 9cc40da4b97730fff6db068be54ea854d210ba0b Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Thu, 18 Jan 2018 22:45:19 -0800 Subject: [PATCH 09/10] revise tests --- nipype/utils/tests/test_config.py | 50 ++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 14 deletions(-) diff --git a/nipype/utils/tests/test_config.py b/nipype/utils/tests/test_config.py index a8f9d727d7..65ada4c646 100644 --- a/nipype/utils/tests/test_config.py +++ b/nipype/utils/tests/test_config.py @@ -28,20 +28,15 @@ spec=['vdisplay_num', 'start', 'stop'], vdisplay_num=2010) -@pytest.mark.parametrize('disp_var', [':0', 'localhost:0', 'localhost:0.1']) -def test_display_parse(monkeypatch, disp_var): +@pytest.mark.parametrize('dispvar', [':12', 'localhost:12', 'localhost:12.1']) +def test_display_parse(monkeypatch, dispvar): """Check that when $DISPLAY is defined, the display is correctly parsed""" config._display = None - dispstr = ':0' - old_disp = os.getenv('DISPLAY') - os.environ['DISPLAY'] = disp_var - assert config.get_display() == dispstr + config._config.remove_option('execution', 'display_variable') + monkeypatch.setenv('DISPLAY', dispvar) + assert config.get_display() == ':12' # Test that it was correctly cached - assert config.get_display() == dispstr - if old_disp is not None: - os.environ['DISPLAY'] = old_disp - else: - del os.environ['DISPLAY'] + assert config.get_display() == ':12' @pytest.mark.parametrize('dispnum', range(5)) @@ -88,10 +83,17 @@ def test_display_noconfig_nosystem_patched(monkeypatch): config._config.remove_option('execution', 'display_variable') monkeypatch.delitem(os.environ, 'DISPLAY', raising=False) monkeypatch.setitem(sys.modules, 'xvfbwrapper', xvfbpatch) + monkeypatch.setattr(sys, 'platform', value='linux') assert config.get_display() == ":2010" # Test that it was correctly cached assert config.get_display() == ':2010' + # Check that raises in Mac + config._display = None + monkeypatch.setattr(sys, 'platform', value='darwin') + with pytest.raises(RuntimeError): + config.get_display() + def test_display_empty_patched(monkeypatch): """ @@ -103,10 +105,16 @@ def test_display_empty_patched(monkeypatch): config._config.remove_option('execution', 'display_variable') monkeypatch.setenv('DISPLAY', '') monkeypatch.setitem(sys.modules, 'xvfbwrapper', xvfbpatch) + monkeypatch.setattr(sys, 'platform', value='linux') assert config.get_display() == ':2010' # Test that it was correctly cached assert config.get_display() == ':2010' + # Check that raises in Mac + config._display = None + monkeypatch.setattr(sys, 'platform', value='darwin') + with pytest.raises(RuntimeError): + config.get_display() def test_display_noconfig_nosystem_patched_oldxvfbwrapper(monkeypatch): """ @@ -118,10 +126,16 @@ def test_display_noconfig_nosystem_patched_oldxvfbwrapper(monkeypatch): config._config.remove_option('execution', 'display_variable') monkeypatch.delitem(os.environ, 'DISPLAY', raising=False) monkeypatch.setitem(sys.modules, 'xvfbwrapper', xvfbpatch_old) + monkeypatch.setattr(sys, 'platform', value='linux') assert config.get_display() == ":2010" # Test that it was correctly cached assert config.get_display() == ':2010' + # Check that raises in Mac + config._display = None + monkeypatch.setattr(sys, 'platform', value='darwin') + with pytest.raises(RuntimeError): + config.get_display() def test_display_empty_patched_oldxvfbwrapper(monkeypatch): """ @@ -133,10 +147,16 @@ def test_display_empty_patched_oldxvfbwrapper(monkeypatch): config._config.remove_option('execution', 'display_variable') monkeypatch.setenv('DISPLAY', '') monkeypatch.setitem(sys.modules, 'xvfbwrapper', xvfbpatch_old) + monkeypatch.setattr(sys, 'platform', value='linux') assert config.get_display() == ':2010' # Test that it was correctly cached assert config.get_display() == ':2010' + # Check that raises in Mac + config._display = None + monkeypatch.setattr(sys, 'platform', value='darwin') + with pytest.raises(RuntimeError): + config.get_display() def test_display_noconfig_nosystem_notinstalled(monkeypatch): """ @@ -146,7 +166,7 @@ def test_display_noconfig_nosystem_notinstalled(monkeypatch): config._display = None if config.has_option('execution', 'display_variable'): config._config.remove_option('execution', 'display_variable') - monkeypatch.delitem(os.environ, 'DISPLAY', raising=False) + monkeypatch.delenv('DISPLAY', raising=False) monkeypatch.setitem(sys.modules, 'xvfbwrapper', None) with pytest.raises(RuntimeError): config.get_display() @@ -167,6 +187,7 @@ def test_display_empty_notinstalled(monkeypatch): @pytest.mark.skipif(not has_Xvfb, reason='xvfbwrapper not installed') +@pytest.mark.skipif('darwin' in sys.platform, reason='macosx requires root for Xvfb') def test_display_noconfig_nosystem_installed(monkeypatch): """ Check that actually uses xvfbwrapper when installed (not mocked) @@ -175,7 +196,7 @@ def test_display_noconfig_nosystem_installed(monkeypatch): config._display = None if config.has_option('execution', 'display_variable'): config._config.remove_option('execution', 'display_variable') - monkeypatch.delitem(os.environ, 'DISPLAY', raising=False) + monkeypatch.delenv('DISPLAY', raising=False) newdisp = config.get_display() assert int(newdisp.split(':')[-1]) > 1000 # Test that it was correctly cached @@ -183,6 +204,7 @@ def test_display_noconfig_nosystem_installed(monkeypatch): @pytest.mark.skipif(not has_Xvfb, reason='xvfbwrapper not installed') +@pytest.mark.skipif('darwin' in sys.platform, reason='macosx requires root for Xvfb') def test_display_empty_installed(monkeypatch): """ Check that actually uses xvfbwrapper when installed (not mocked) @@ -207,7 +229,7 @@ def test_display_empty_macosx(monkeypatch): config._display = None if config.has_option('execution', 'display_variable'): config._config.remove_option('execution', 'display_variable') - monkeypatch.delitem(os.environ, 'DISPLAY', '') + monkeypatch.delenv('DISPLAY', '') monkeypatch.setattr(sys, 'platform', 'darwin') with pytest.raises(RuntimeError): From 7d80634b3702525e68748b79550d472bd962fb31 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Thu, 18 Jan 2018 22:58:30 -0800 Subject: [PATCH 10/10] roll back travis setting --- .travis.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 9fe7b3b080..08d9234675 100644 --- a/.travis.yml +++ b/.travis.yml @@ -13,7 +13,6 @@ env: - INSTALL_DEB_DEPENDECIES=true NIPYPE_EXTRAS="doc,tests,fmri,profiler,duecredit" CI_SKIP_TEST=1 - INSTALL_DEB_DEPENDECIES=true NIPYPE_EXTRAS="doc,tests,fmri,profiler" PIP_FLAGS="--pre" CI_SKIP_TEST=1 before_install: -- unset DISPLAY - function apt_inst { if $INSTALL_DEB_DEPENDECIES; then sudo rm -rf /dev/shm; fi && if $INSTALL_DEB_DEPENDECIES; then sudo ln -s /run/shm /dev/shm; fi &&