From a2952e13a896fffbb16bc31549d8919bbd4f40e8 Mon Sep 17 00:00:00 2001 From: Jay Kamat Date: Fri, 15 Sep 2017 20:43:17 -0400 Subject: [PATCH 1/6] Add qutebrowser config directory to python path This is done so config.py can import other python files in the config directory. For example, config.py can 'import theme' which would load a theme.py. The previous path is restored at the end of this function, to avoid tainting qutebrowser's path --- qutebrowser/config/configfiles.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/qutebrowser/config/configfiles.py b/qutebrowser/config/configfiles.py index a155726a8..155a2c99f 100644 --- a/qutebrowser/config/configfiles.py +++ b/qutebrowser/config/configfiles.py @@ -21,6 +21,7 @@ import types import os.path +import sys import textwrap import traceback import configparser @@ -222,6 +223,12 @@ def read_config_py(filename=None): if not os.path.exists(filename): return api + # Add config directory to python path, so config.py can import other files + # in logical places + old_path = sys.path.copy() + if standarddir.config() not in sys.path: + sys.path.insert(0, standarddir.config()) + container = config.ConfigContainer(config.instance, configapi=api) basename = os.path.basename(filename) @@ -256,6 +263,9 @@ def read_config_py(filename=None): "Unhandled exception", exception=e, traceback=traceback.format_exc())) + # Restore previous path, to protect qutebrowser's imports + sys.path = old_path + api.finalize() return api From 0332dce458e249e73efd61cdf556d60f1062406c Mon Sep 17 00:00:00 2001 From: Jay Kamat Date: Sat, 16 Sep 2017 01:07:13 -0400 Subject: [PATCH 2/6] Get config path from config.py location, rather than standarddir --- qutebrowser/config/configfiles.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/qutebrowser/config/configfiles.py b/qutebrowser/config/configfiles.py index 155a2c99f..450b7b644 100644 --- a/qutebrowser/config/configfiles.py +++ b/qutebrowser/config/configfiles.py @@ -226,8 +226,9 @@ def read_config_py(filename=None): # Add config directory to python path, so config.py can import other files # in logical places old_path = sys.path.copy() - if standarddir.config() not in sys.path: - sys.path.insert(0, standarddir.config()) + config_dir = os.path.dirname(filename) + if config_dir not in sys.path: + sys.path.insert(0, config_dir) container = config.ConfigContainer(config.instance, configapi=api) basename = os.path.basename(filename) From 333c0d848b9564ed000b66774b63e3014277f451 Mon Sep 17 00:00:00 2001 From: Jay Kamat Date: Sat, 16 Sep 2017 23:22:19 -0400 Subject: [PATCH 3/6] Restructure save/load of state to be more extensible Also save/load sys.modules as well - This is a little rough, but I can't find a better way... --- qutebrowser/config/configfiles.py | 33 +++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/qutebrowser/config/configfiles.py b/qutebrowser/config/configfiles.py index 450b7b644..57d6e851b 100644 --- a/qutebrowser/config/configfiles.py +++ b/qutebrowser/config/configfiles.py @@ -225,10 +225,10 @@ def read_config_py(filename=None): # Add config directory to python path, so config.py can import other files # in logical places - old_path = sys.path.copy() + old_state = _pre_config_save() config_dir = os.path.dirname(filename) if config_dir not in sys.path: - sys.path.insert(0, config_dir) + sys.path = [config_dir] + sys.path container = config.ConfigContainer(config.instance, configapi=api) basename = os.path.basename(filename) @@ -238,6 +238,20 @@ def read_config_py(filename=None): module.c = container module.__file__ = filename + try: + _run_python_config_helper(filename, basename, api, module) + except: + _post_config_load(old_state) + raise + + # Restore previous path, to protect qutebrowser's imports + _post_config_load(old_state) + + api.finalize() + return api + + +def _run_python_config_helper(filename, basename, api, module): try: with open(filename, mode='rb') as f: source = f.read() @@ -264,11 +278,18 @@ def read_config_py(filename=None): "Unhandled exception", exception=e, traceback=traceback.format_exc())) - # Restore previous path, to protect qutebrowser's imports - sys.path = old_path - api.finalize() - return api +def _pre_config_save(): + old_path = sys.path + old_modules = sys.modules.copy() + return (old_path, old_modules) + + +def _post_config_load(save_tuple): + sys.path = save_tuple[0] + for module in set(sys.modules).difference(save_tuple[1]): + del sys.modules[module] + pass def init(): From 7ddde334dacaa21661f41de893864319b2245b8a Mon Sep 17 00:00:00 2001 From: Jay Kamat Date: Sun, 17 Sep 2017 01:18:20 -0400 Subject: [PATCH 4/6] Add tests for module/path isolation --- qutebrowser/config/configfiles.py | 1 - tests/unit/config/test_configfiles.py | 138 ++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 1 deletion(-) diff --git a/qutebrowser/config/configfiles.py b/qutebrowser/config/configfiles.py index 57d6e851b..91dafe172 100644 --- a/qutebrowser/config/configfiles.py +++ b/qutebrowser/config/configfiles.py @@ -289,7 +289,6 @@ def _post_config_load(save_tuple): sys.path = save_tuple[0] for module in set(sys.modules).difference(save_tuple[1]): del sys.modules[module] - pass def init(): diff --git a/tests/unit/config/test_configfiles.py b/tests/unit/config/test_configfiles.py index 975e9c46a..0e782efd7 100644 --- a/tests/unit/config/test_configfiles.py +++ b/tests/unit/config/test_configfiles.py @@ -207,6 +207,144 @@ class TestYaml: assert error.traceback is None +class TestConfigPyModules: + + """Test for ConfigPy Modules.""" + + pytestmark = pytest.mark.usefixtures('config_stub', 'key_config_stub') + _old_sys_path = sys.path.copy() + + class ConfPy: + + """Helper class to get a confpy fixture.""" + + def __init__(self, tmpdir): + self._confpy = tmpdir / 'config.py' + self.filename = str(self._confpy) + + def write(self, *lines): + text = '\n'.join(lines) + self._confpy.write_text(text, 'utf-8', ensure=True) + + class QbModulePy: + + """Helper class to get a QbModulePy fixture.""" + + def __init__(self, tmpdir): + self._qbmodulepy = tmpdir / 'qbmodule.py' + self.filename = str(self._qbmodulepy) + + def write(self, *lines): + text = '\n'.join(lines) + self._qbmodulepy.write_text(text, 'utf-8', ensure=True) + + @pytest.fixture + def confpy(self, tmpdir): + return self.ConfPy(tmpdir) + + @pytest.fixture + def qbmodulepy(self, tmpdir): + return self.QbModulePy(tmpdir) + + def setup_method(self, method): + # If we plan to add tests that modify modules themselves, that should + # be saved as well + TestConfigPyModules._old_sys_path = sys.path.copy() + + def teardown_method(self, method): + # Restore path to save the rest of the tests + sys.path = TestConfigPyModules._old_sys_path + + def test_bind_in_module(self, confpy, qbmodulepy): + qbmodulepy.write("""def run(config): + config.bind(",a", "message-info foo", mode="normal")""") + confpy.write("""import qbmodule +qbmodule.run(config)""") + api = configfiles.read_config_py(confpy.filename) + expected = {'normal': {',a': 'message-info foo'}} + assert len(api.errors) == 0 + assert config.instance._values['bindings.commands'] == expected + + def test_clear_path(self, confpy, qbmodulepy, tmpdir): + qbmodulepy.write("""def run(config): + config.bind(",a", "message-info foo", mode="normal")""") + confpy.write("""import qbmodule +qbmodule.run(config)""") + api = configfiles.read_config_py(confpy.filename) + assert len(api.errors) == 0 + assert tmpdir not in sys.path + + def test_clear_modules(self, confpy, qbmodulepy): + qbmodulepy.write("""def run(config): + config.bind(",a", "message-info foo", mode="normal")""") + confpy.write("""import qbmodule +qbmodule.run(config)""") + api = configfiles.read_config_py(confpy.filename) + assert len(api.errors) == 0 + assert "qbmodule" not in sys.modules.keys() + + def test_clear_modules_on_err(self, confpy, qbmodulepy): + qbmodulepy.write("""def run(config): + 1/0""") + confpy.write("""import qbmodule +qbmodule.run(config)""") + api = configfiles.read_config_py(confpy.filename) + + assert len(api.errors) == 1 + error = api.errors[0] + assert error.text == "Unhandled exception" + assert isinstance(error.exception, ZeroDivisionError) + + tblines = error.traceback.strip().splitlines() + assert tblines[0] == "Traceback (most recent call last):" + assert tblines[-1] == "ZeroDivisionError: division by zero" + assert " 1/0" in tblines + assert "qbmodule" not in sys.modules.keys() + + def test_clear_path_on_err(self, confpy, qbmodulepy, tmpdir): + qbmodulepy.write("""def run(config): + 1/0""") + confpy.write("""import qbmodule +qbmodule.run(config)""") + api = configfiles.read_config_py(confpy.filename) + + assert len(api.errors) == 1 + error = api.errors[0] + assert error.text == "Unhandled exception" + assert isinstance(error.exception, ZeroDivisionError) + + tblines = error.traceback.strip().splitlines() + assert tblines[0] == "Traceback (most recent call last):" + assert tblines[-1] == "ZeroDivisionError: division by zero" + assert " 1/0" in tblines + assert tmpdir not in sys.path + + def test_fail_on_nonexistent_module(self, confpy, qbmodulepy, tmpdir): + qbmodulepy.write("""def run(config): + pass""") + confpy.write("""import foobar +foobar.run(config)""") + api = configfiles.read_config_py(confpy.filename) + + assert len(api.errors) == 1 + error = api.errors[0] + assert error.text == "Unhandled exception" + assert isinstance(error.exception, ImportError) + + tblines = error.traceback.strip().splitlines() + assert tblines[0] == "Traceback (most recent call last):" + assert tblines[-1].endswith("Error: No module named 'foobar'") + + def test_no_double_if_path_exists(self, confpy, qbmodulepy, tmpdir): + sys.path.insert(0, tmpdir) + confpy.write("""import sys +if sys.path[1:].count(sys.path[0]) != 0: + raise Exception('Path not expected')""") + api = configfiles.read_config_py(confpy.filename) + assert len(api.errors) == 0 + assert sys.path.count(tmpdir) == 1 + + class TestConfigPy: """Tests for ConfigAPI and read_config_py().""" From 4e22b4666d79b6b7f4f9b56aff12d72dae42217d Mon Sep 17 00:00:00 2001 From: Jay Kamat Date: Wed, 20 Sep 2017 21:26:56 -0400 Subject: [PATCH 5/6] Convert save-restore of sys to a context-manager Also improve and simplify tests for save/load of sys.module and sys.path --- qutebrowser/config/configfiles.py | 53 +++++------ tests/unit/config/test_configfiles.py | 130 ++++++-------------------- 2 files changed, 54 insertions(+), 129 deletions(-) diff --git a/qutebrowser/config/configfiles.py b/qutebrowser/config/configfiles.py index 91dafe172..561eec0eb 100644 --- a/qutebrowser/config/configfiles.py +++ b/qutebrowser/config/configfiles.py @@ -223,13 +223,6 @@ def read_config_py(filename=None): if not os.path.exists(filename): return api - # Add config directory to python path, so config.py can import other files - # in logical places - old_state = _pre_config_save() - config_dir = os.path.dirname(filename) - if config_dir not in sys.path: - sys.path = [config_dir] + sys.path - container = config.ConfigContainer(config.instance, configapi=api) basename = os.path.basename(filename) @@ -238,20 +231,6 @@ def read_config_py(filename=None): module.c = container module.__file__ = filename - try: - _run_python_config_helper(filename, basename, api, module) - except: - _post_config_load(old_state) - raise - - # Restore previous path, to protect qutebrowser's imports - _post_config_load(old_state) - - api.finalize() - return api - - -def _run_python_config_helper(filename, basename, api, module): try: with open(filename, mode='rb') as f: source = f.read() @@ -268,27 +247,41 @@ def _run_python_config_helper(filename, basename, api, module): raise configexc.ConfigFileErrors(basename, [desc]) except SyntaxError as e: desc = configexc.ConfigErrorDesc("Syntax Error", e, - traceback=traceback.format_exc()) + traceback=traceback.format_exc()) raise configexc.ConfigFileErrors(basename, [desc]) try: - exec(code, module.__dict__) + # Save and restore sys variables + with saved_sys_properties(): + # Add config directory to python path, so config.py can import + # other files in logical places + config_dir = os.path.dirname(filename) + if config_dir not in sys.path: + sys.path = [config_dir] + sys.path + + exec(code, module.__dict__) except Exception as e: api.errors.append(configexc.ConfigErrorDesc( "Unhandled exception", exception=e, traceback=traceback.format_exc())) + api.finalize() + return api -def _pre_config_save(): +@contextlib.contextmanager +def saved_sys_properties(): + """Save various sys properties such as sys.path and sys.modules.""" old_path = sys.path old_modules = sys.modules.copy() - return (old_path, old_modules) - -def _post_config_load(save_tuple): - sys.path = save_tuple[0] - for module in set(sys.modules).difference(save_tuple[1]): - del sys.modules[module] + try: + yield + except: + raise + finally: + sys.path = old_path + for module in set(sys.modules).difference(old_modules): + del sys.modules[module] def init(): diff --git a/tests/unit/config/test_configfiles.py b/tests/unit/config/test_configfiles.py index 0e782efd7..2d8d4cd94 100644 --- a/tests/unit/config/test_configfiles.py +++ b/tests/unit/config/test_configfiles.py @@ -209,121 +209,50 @@ class TestYaml: class TestConfigPyModules: - """Test for ConfigPy Modules.""" - pytestmark = pytest.mark.usefixtures('config_stub', 'key_config_stub') - _old_sys_path = sys.path.copy() - - class ConfPy: - - """Helper class to get a confpy fixture.""" - - def __init__(self, tmpdir): - self._confpy = tmpdir / 'config.py' - self.filename = str(self._confpy) - - def write(self, *lines): - text = '\n'.join(lines) - self._confpy.write_text(text, 'utf-8', ensure=True) - - class QbModulePy: - - """Helper class to get a QbModulePy fixture.""" - - def __init__(self, tmpdir): - self._qbmodulepy = tmpdir / 'qbmodule.py' - self.filename = str(self._qbmodulepy) - - def write(self, *lines): - text = '\n'.join(lines) - self._qbmodulepy.write_text(text, 'utf-8', ensure=True) @pytest.fixture def confpy(self, tmpdir): - return self.ConfPy(tmpdir) + return TestConfigPy.ConfPy(tmpdir) @pytest.fixture def qbmodulepy(self, tmpdir): - return self.QbModulePy(tmpdir) + return TestConfigPy.ConfPy(tmpdir, filename="qbmodule.py") - def setup_method(self, method): - # If we plan to add tests that modify modules themselves, that should - # be saved as well - TestConfigPyModules._old_sys_path = sys.path.copy() + @pytest.fixture(autouse=True) + def restore_sys_path(self): + old_path = sys.path.copy() + yield + sys.path = old_path - def teardown_method(self, method): - # Restore path to save the rest of the tests - sys.path = TestConfigPyModules._old_sys_path - - def test_bind_in_module(self, confpy, qbmodulepy): - qbmodulepy.write("""def run(config): - config.bind(",a", "message-info foo", mode="normal")""") - confpy.write("""import qbmodule -qbmodule.run(config)""") - api = configfiles.read_config_py(confpy.filename) + def test_bind_in_module(self, confpy, qbmodulepy, tmpdir): + qbmodulepy.write('def run(config):', + ' config.bind(",a", "message-info foo", mode="normal")') + confpy.write_qbmodule() + confpy.read() expected = {'normal': {',a': 'message-info foo'}} - assert len(api.errors) == 0 assert config.instance._values['bindings.commands'] == expected - - def test_clear_path(self, confpy, qbmodulepy, tmpdir): - qbmodulepy.write("""def run(config): - config.bind(",a", "message-info foo", mode="normal")""") - confpy.write("""import qbmodule -qbmodule.run(config)""") - api = configfiles.read_config_py(confpy.filename) - assert len(api.errors) == 0 + assert "qbmodule" not in sys.modules.keys() assert tmpdir not in sys.path - def test_clear_modules(self, confpy, qbmodulepy): - qbmodulepy.write("""def run(config): - config.bind(",a", "message-info foo", mode="normal")""") - confpy.write("""import qbmodule -qbmodule.run(config)""") - api = configfiles.read_config_py(confpy.filename) - assert len(api.errors) == 0 - assert "qbmodule" not in sys.modules.keys() - - def test_clear_modules_on_err(self, confpy, qbmodulepy): - qbmodulepy.write("""def run(config): - 1/0""") - confpy.write("""import qbmodule -qbmodule.run(config)""") + def test_restore_sys_on_err(self, confpy, qbmodulepy, tmpdir): + confpy.write_qbmodule() + qbmodulepy.write('def run(config):', + ' 1/0') api = configfiles.read_config_py(confpy.filename) assert len(api.errors) == 1 error = api.errors[0] assert error.text == "Unhandled exception" assert isinstance(error.exception, ZeroDivisionError) - - tblines = error.traceback.strip().splitlines() - assert tblines[0] == "Traceback (most recent call last):" - assert tblines[-1] == "ZeroDivisionError: division by zero" - assert " 1/0" in tblines assert "qbmodule" not in sys.modules.keys() - - def test_clear_path_on_err(self, confpy, qbmodulepy, tmpdir): - qbmodulepy.write("""def run(config): - 1/0""") - confpy.write("""import qbmodule -qbmodule.run(config)""") - api = configfiles.read_config_py(confpy.filename) - - assert len(api.errors) == 1 - error = api.errors[0] - assert error.text == "Unhandled exception" - assert isinstance(error.exception, ZeroDivisionError) - - tblines = error.traceback.strip().splitlines() - assert tblines[0] == "Traceback (most recent call last):" - assert tblines[-1] == "ZeroDivisionError: division by zero" - assert " 1/0" in tblines assert tmpdir not in sys.path def test_fail_on_nonexistent_module(self, confpy, qbmodulepy, tmpdir): - qbmodulepy.write("""def run(config): - pass""") - confpy.write("""import foobar -foobar.run(config)""") + qbmodulepy.write('def run(config):', + ' pass') + confpy.write('import foobar', + 'foobar.run(config)') api = configfiles.read_config_py(confpy.filename) assert len(api.errors) == 1 @@ -337,11 +266,10 @@ foobar.run(config)""") def test_no_double_if_path_exists(self, confpy, qbmodulepy, tmpdir): sys.path.insert(0, tmpdir) - confpy.write("""import sys -if sys.path[1:].count(sys.path[0]) != 0: - raise Exception('Path not expected')""") - api = configfiles.read_config_py(confpy.filename) - assert len(api.errors) == 0 + confpy.write('import sys', + 'if sys.path[0] in sys.path[1:]:', + ' raise Exception("Path not expected")') + confpy.read() assert sys.path.count(tmpdir) == 1 @@ -355,8 +283,8 @@ class TestConfigPy: """Helper class to get a confpy fixture.""" - def __init__(self, tmpdir): - self._confpy = tmpdir / 'config.py' + def __init__(self, tmpdir, filename: str = "config.py"): + self._confpy = tmpdir / filename self.filename = str(self._confpy) def write(self, *lines): @@ -368,6 +296,10 @@ class TestConfigPy: api = configfiles.read_config_py(self.filename) assert not api.errors + def write_qbmodule(self): + self.write('import qbmodule', + 'qbmodule.run(config)') + @pytest.fixture def confpy(self, tmpdir): return self.ConfPy(tmpdir) From 43ce10efc34188825421f1db94b60117b46b57b3 Mon Sep 17 00:00:00 2001 From: Jay Kamat Date: Thu, 21 Sep 2017 10:17:25 -0400 Subject: [PATCH 6/6] Simplify and reorganize configfile tests Also make save/load of sys.path a little more robust --- qutebrowser/config/configfiles.py | 9 +++-- tests/unit/config/test_configfiles.py | 50 ++++++++++++++------------- 2 files changed, 30 insertions(+), 29 deletions(-) diff --git a/qutebrowser/config/configfiles.py b/qutebrowser/config/configfiles.py index 561eec0eb..2ee973730 100644 --- a/qutebrowser/config/configfiles.py +++ b/qutebrowser/config/configfiles.py @@ -247,7 +247,7 @@ def read_config_py(filename=None): raise configexc.ConfigFileErrors(basename, [desc]) except SyntaxError as e: desc = configexc.ConfigErrorDesc("Syntax Error", e, - traceback=traceback.format_exc()) + traceback=traceback.format_exc()) raise configexc.ConfigFileErrors(basename, [desc]) try: @@ -257,13 +257,14 @@ def read_config_py(filename=None): # other files in logical places config_dir = os.path.dirname(filename) if config_dir not in sys.path: - sys.path = [config_dir] + sys.path + sys.path.insert(0, config_dir) exec(code, module.__dict__) except Exception as e: api.errors.append(configexc.ConfigErrorDesc( "Unhandled exception", exception=e, traceback=traceback.format_exc())) + api.finalize() return api @@ -271,13 +272,11 @@ def read_config_py(filename=None): @contextlib.contextmanager def saved_sys_properties(): """Save various sys properties such as sys.path and sys.modules.""" - old_path = sys.path + old_path = sys.path.copy() old_modules = sys.modules.copy() try: yield - except: - raise finally: sys.path = old_path for module in set(sys.modules).difference(old_modules): diff --git a/tests/unit/config/test_configfiles.py b/tests/unit/config/test_configfiles.py index 2d8d4cd94..bf214fed3 100644 --- a/tests/unit/config/test_configfiles.py +++ b/tests/unit/config/test_configfiles.py @@ -19,6 +19,7 @@ """Tests for qutebrowser.config.configfiles.""" import os +import sys import pytest @@ -207,17 +208,39 @@ class TestYaml: assert error.traceback is None +class ConfPy: + + """Helper class to get a confpy fixture.""" + + def __init__(self, tmpdir, filename: str = "config.py"): + self._file = tmpdir / filename + self.filename = str(self._file) + + def write(self, *lines): + text = '\n'.join(lines) + self._file.write_text(text, 'utf-8', ensure=True) + + def read(self): + """Read the config.py via configfiles and check for errors.""" + api = configfiles.read_config_py(self.filename) + assert not api.errors + + def write_qbmodule(self): + self.write('import qbmodule', + 'qbmodule.run(config)') + + class TestConfigPyModules: pytestmark = pytest.mark.usefixtures('config_stub', 'key_config_stub') @pytest.fixture def confpy(self, tmpdir): - return TestConfigPy.ConfPy(tmpdir) + return ConfPy(tmpdir) @pytest.fixture def qbmodulepy(self, tmpdir): - return TestConfigPy.ConfPy(tmpdir, filename="qbmodule.py") + return ConfPy(tmpdir, filename="qbmodule.py") @pytest.fixture(autouse=True) def restore_sys_path(self): @@ -279,30 +302,9 @@ class TestConfigPy: pytestmark = pytest.mark.usefixtures('config_stub', 'key_config_stub') - class ConfPy: - - """Helper class to get a confpy fixture.""" - - def __init__(self, tmpdir, filename: str = "config.py"): - self._confpy = tmpdir / filename - self.filename = str(self._confpy) - - def write(self, *lines): - text = '\n'.join(lines) - self._confpy.write_text(text, 'utf-8', ensure=True) - - def read(self): - """Read the config.py via configfiles and check for errors.""" - api = configfiles.read_config_py(self.filename) - assert not api.errors - - def write_qbmodule(self): - self.write('import qbmodule', - 'qbmodule.run(config)') - @pytest.fixture def confpy(self, tmpdir): - return self.ConfPy(tmpdir) + return ConfPy(tmpdir) @pytest.mark.parametrize('line', [ 'c.colors.hints.bg = "red"',