From 56bbd73622d201d22620e5e50afee933d2adcf0c Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Wed, 13 Sep 2017 11:55:52 +0200 Subject: [PATCH] Introduce standarddir caching This makes things a bit more complicated, but is needed to make standarddir (and thus the config) work without a QApplication. --- qutebrowser/utils/standarddir.py | 120 ++++++++++++++++-------- tests/conftest.py | 10 +- tests/unit/browser/test_adblock.py | 3 +- tests/unit/commands/test_userscripts.py | 2 +- tests/unit/misc/test_ipc.py | 3 +- tests/unit/utils/test_standarddir.py | 68 +++++++++----- 6 files changed, 141 insertions(+), 65 deletions(-) diff --git a/qutebrowser/utils/standarddir.py b/qutebrowser/utils/standarddir.py index 28d84764f..1d0bf7749 100644 --- a/qutebrowser/utils/standarddir.py +++ b/qutebrowser/utils/standarddir.py @@ -26,11 +26,14 @@ import os.path from PyQt5.QtCore import QCoreApplication, QStandardPaths -from qutebrowser.utils import log, qtutils, debug +from qutebrowser.utils import log, qtutils, debug, usertypes + +# The cached locations +_locations = {} -# The argparse namespace passed to init() -_args = None +Location = usertypes.enum('Location', ['config', 'data', 'system_data', + 'cache', 'download', 'runtime']) class EmptyValueError(Exception): @@ -38,10 +41,10 @@ class EmptyValueError(Exception): """Error raised when QStandardPaths returns an empty value.""" -def config(): - """Get a location for configs.""" +def _init_config(args): + """Initialize the location for configs.""" typ = QStandardPaths.ConfigLocation - overridden, path = _from_args(typ, _args) + overridden, path = _from_args(typ, args) if not overridden: path = _writable_location(typ) appname = QCoreApplication.instance().applicationName() @@ -50,13 +53,17 @@ def config(): # https://bugreports.qt.io/browse/QTBUG-38872 path = os.path.join(path, appname) _create(path) - return path + _locations[Location.config] = path -def data(): - """Get a location for data.""" +def config(): + return _locations[Location.config] + + +def _init_data(args): + """Initialize the location for data.""" typ = QStandardPaths.DataLocation - overridden, path = _from_args(typ, _args) + overridden, path = _from_args(typ, args) if not overridden: path = _writable_location(typ) if os.name == 'nt': @@ -68,49 +75,71 @@ def data(): if data_path == config_path: path = os.path.join(path, 'data') _create(path) - return path + _locations[Location.data] = path + + +def data(): + return _locations[Location.data] + + +def _init_system_data(_args): + """Initialize the location for system-wide data. + + This path may be read-only.""" + _locations.pop(Location.system_data, None) # Remove old state + if sys.platform.startswith('linux'): + path = "/usr/share/qutebrowser" + if os.path.exists(path): + _locations[Location.system_data] = path def system_data(): - """Get a location for system-wide data. This path may be read-only.""" - if sys.platform.startswith('linux'): - path = "/usr/share/qutebrowser" - if not os.path.exists(path): - path = data() - else: - path = data() - return path + try: + return _locations[Location.system_data] + except KeyError: + return _locations[Location.data] + + +def _init_cache(args): + """Initialize the location for the cache.""" + typ = QStandardPaths.CacheLocation + overridden, path = _from_args(typ, args) + if not overridden: + path = _writable_location(typ) + _create(path) + _locations[Location.cache] = path def cache(): - """Get a location for the cache.""" - typ = QStandardPaths.CacheLocation - overridden, path = _from_args(typ, _args) + return _locations[Location.cache] + + +def _init_download(args): + """Initialize the location for downloads. + + Note this is only the default directory as found by Qt. + """ + typ = QStandardPaths.DownloadLocation + overridden, path = _from_args(typ, args) if not overridden: path = _writable_location(typ) _create(path) - return path + _locations[Location.download] = path def download(): - """Get a location for downloads.""" - typ = QStandardPaths.DownloadLocation - overridden, path = _from_args(typ, _args) - if not overridden: - path = _writable_location(typ) - _create(path) - return path + return _locations[Location.download] -def runtime(): - """Get a location for runtime data.""" +def _init_runtime(args): + """Initialize location for runtime data.""" if sys.platform.startswith('linux'): typ = QStandardPaths.RuntimeLocation - else: # pragma: no cover + else: # RuntimeLocation is a weird path on macOS and Windows. typ = QStandardPaths.TempLocation - overridden, path = _from_args(typ, _args) + overridden, path = _from_args(typ, args) if not overridden: try: @@ -132,7 +161,11 @@ def runtime(): appname = QCoreApplication.instance().applicationName() path = os.path.join(path, appname) _create(path) - return path + _locations[Location.runtime] = path + + +def runtime(): + return _locations[Location.runtime] def _writable_location(typ): @@ -195,13 +228,26 @@ def _create(path): pass +def _init_dirs(args=None): + """Create and cache standard directory locations. + + Mainly in a separate function because we need to call it in tests. + """ + _init_config(args) + _init_data(args) + _init_system_data(args) + _init_cache(args) + _init_download(args) + _init_runtime(args) + + def init(args): """Initialize all standard dirs.""" - global _args if args is not None: # args can be None during tests log.init.debug("Base directory: {}".format(args.basedir)) - _args = args + + _init_dirs(args) _init_cachedir_tag() if args is not None: _move_webengine_data() diff --git a/tests/conftest.py b/tests/conftest.py index f5018de66..89e17dce2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -35,7 +35,7 @@ from helpers import logfail from helpers.logfail import fail_on_logging from helpers.messagemock import message_mock from helpers.fixtures import * -from qutebrowser.utils import qtutils +from qutebrowser.utils import qtutils, standarddir import qutebrowser.app # To register commands @@ -157,6 +157,14 @@ def qapp(qapp): return qapp +@pytest.fixture +def init_standarddir(qapp): + """Initialize the standarddir module for tests which need it.""" + standarddir._init_dirs() + yield + standarddir._locations = {} + + def pytest_addoption(parser): parser.addoption('--qute-delay', action='store', default=0, type=int, help="Delay between qutebrowser commands.") diff --git a/tests/unit/browser/test_adblock.py b/tests/unit/browser/test_adblock.py index d1e2cc889..f53ec14f7 100644 --- a/tests/unit/browser/test_adblock.py +++ b/tests/unit/browser/test_adblock.py @@ -316,7 +316,8 @@ def test_failed_dl_update(config_stub, basedir, download_stub, @pytest.mark.parametrize('location', ['content', 'comment']) -def test_invalid_utf8(config_stub, download_stub, tmpdir, caplog, location): +def test_invalid_utf8(config_stub, download_stub, init_standarddir, tmpdir, + caplog, location): """Make sure invalid UTF-8 is handled correctly. See https://github.com/qutebrowser/qutebrowser/issues/2301 diff --git a/tests/unit/commands/test_userscripts.py b/tests/unit/commands/test_userscripts.py index df3c53c68..f921dc3a1 100644 --- a/tests/unit/commands/test_userscripts.py +++ b/tests/unit/commands/test_userscripts.py @@ -59,7 +59,7 @@ class TestQtFIFOReader: userscripts._POSIXUserscriptRunner, userscripts._WindowsUserscriptRunner, ]) -def runner(request): +def runner(request, init_standarddir): if (os.name != 'posix' and request.param is userscripts._POSIXUserscriptRunner): pytest.skip("Requires a POSIX os") diff --git a/tests/unit/misc/test_ipc.py b/tests/unit/misc/test_ipc.py index d0758b28d..acc0ad225 100644 --- a/tests/unit/misc/test_ipc.py +++ b/tests/unit/misc/test_ipc.py @@ -35,7 +35,7 @@ from PyQt5.QtTest import QSignalSpy import qutebrowser from qutebrowser.misc import ipc -from qutebrowser.utils import objreg, qtutils +from qutebrowser.utils import objreg, qtutils, standarddir from helpers import stubs @@ -88,6 +88,7 @@ def qlocalsocket(qapp): @pytest.fixture(autouse=True) def fake_runtime_dir(monkeypatch, short_tmpdir): monkeypatch.setenv('XDG_RUNTIME_DIR', str(short_tmpdir)) + standarddir._init_dirs() return short_tmpdir diff --git a/tests/unit/utils/test_standarddir.py b/tests/unit/utils/test_standarddir.py index ea3368862..7296d613c 100644 --- a/tests/unit/utils/test_standarddir.py +++ b/tests/unit/utils/test_standarddir.py @@ -32,6 +32,9 @@ import pytest from qutebrowser.utils import standarddir +pytestmark = pytest.mark.usefixtures('qapp') + + @pytest.fixture(autouse=True) def change_qapp_name(qapp): """Change the name of the QApplication instance. @@ -45,21 +48,14 @@ def change_qapp_name(qapp): qapp.setApplicationName(old_name) -@pytest.fixture -def no_cachedir_tag(monkeypatch): - """Fixture to prevent writing a CACHEDIR.TAG.""" - monkeypatch.setattr(standarddir, '_init_cachedir_tag', lambda: None) - - -@pytest.fixture -def reset_standarddir(no_cachedir_tag): - """Clean up standarddir arguments before and after each test.""" - standarddir.init(None) +@pytest.fixture(autouse=True) +def clear_standarddir_cache(monkeypatch): + """Make sure the standarddir cache is cleared before/after each test.""" + monkeypatch.setattr(standarddir, '_locations', {}) yield - standarddir.init(None) + monkeypatch.setattr(standarddir, '_locations', {}) -@pytest.mark.usefixtures('reset_standarddir') @pytest.mark.parametrize('data_subdir, config_subdir, expected', [ ('foo', 'foo', 'foo/data'), ('foo', 'bar', 'foo'), @@ -74,11 +70,11 @@ def test_get_fake_windows_equal_dir(data_subdir, config_subdir, expected, monkeypatch.setattr(standarddir.os, 'name', 'nt') monkeypatch.setattr(standarddir.QStandardPaths, 'writableLocation', locations.get) + standarddir._init_data(args=None) expected = str(tmpdir / expected) assert standarddir.data() == expected -@pytest.mark.usefixtures('reset_standarddir') class TestWritableLocation: """Tests for _writable_location.""" @@ -99,7 +95,6 @@ class TestWritableLocation: assert '\\' in loc -@pytest.mark.usefixtures('reset_standarddir') class TestStandardDir: """Tests for standarddir.""" @@ -119,6 +114,7 @@ class TestStandardDir: varname: The environment variable which should be set. """ monkeypatch.setenv(varname, str(tmpdir)) + standarddir._init_dirs() assert func() == str(tmpdir / 'qute_test') @pytest.mark.parametrize('func, subdirs', [ @@ -133,6 +129,7 @@ class TestStandardDir: monkeypatch.setenv('HOME', str(tmpdir)) for var in ['DATA', 'CONFIG', 'CACHE']: monkeypatch.delenv('XDG_{}_HOME'.format(var), raising=False) + standarddir._init_dirs() assert func() == str(tmpdir.join(*subdirs)) @pytest.mark.linux @@ -141,6 +138,7 @@ class TestStandardDir: """With invalid XDG_RUNTIME_DIR, fall back to TempLocation.""" monkeypatch.setenv('XDG_RUNTIME_DIR', str(tmpdir / 'does-not-exist')) monkeypatch.setenv('TMPDIR', str(tmpdir / 'temp')) + standarddir._init_dirs() assert standarddir.runtime() == str(tmpdir / 'temp' / 'qute_test') def test_runtimedir_empty_tempdir(self, monkeypatch, tmpdir): @@ -149,7 +147,7 @@ class TestStandardDir: monkeypatch.setattr(standarddir.QStandardPaths, 'writableLocation', lambda typ: '') with pytest.raises(standarddir.EmptyValueError): - standarddir.runtime() + standarddir._init_runtime(args=None) @pytest.mark.parametrize('func, elems, expected', [ (standarddir.data, 2, ['qute_test', 'data']), @@ -159,6 +157,7 @@ class TestStandardDir: ]) @pytest.mark.windows def test_windows(self, func, elems, expected): + standarddir._init_dirs() assert func().split(os.sep)[-elems:] == expected @pytest.mark.parametrize('func, elems, expected', [ @@ -169,13 +168,13 @@ class TestStandardDir: ]) @pytest.mark.mac def test_mac(self, func, elems, expected): + standarddir._init_dirs() assert func().split(os.sep)[-elems:] == expected DirArgTest = collections.namedtuple('DirArgTest', 'arg, expected') -@pytest.mark.usefixtures('reset_standarddir') class TestArguments: """Tests the --basedir argument.""" @@ -187,7 +186,7 @@ class TestArguments: """Test --basedir.""" expected = str(tmpdir / typ) args = types.SimpleNamespace(basedir=str(tmpdir)) - standarddir.init(args) + standarddir._init_dirs(args) func = getattr(standarddir, typ) assert func() == expected @@ -197,7 +196,7 @@ class TestArguments: basedir.ensure(dir=True) with tmpdir.as_cwd(): args = types.SimpleNamespace(basedir='basedir') - standarddir.init(args) + standarddir._init_dirs(args) assert standarddir.config() == str(basedir / 'config') @@ -252,7 +251,7 @@ class TestCreatingDir: basedir = tmpdir / 'basedir' assert not basedir.exists() args = types.SimpleNamespace(basedir=str(basedir)) - standarddir.init(args) + standarddir._init_dirs(args) func = getattr(standarddir, typ) func() @@ -262,7 +261,6 @@ class TestCreatingDir: if os.name == 'posix': assert basedir.stat().mode & 0o777 == 0o700 - @pytest.mark.usefixtures('reset_standarddir') @pytest.mark.parametrize('typ', DIR_TYPES) def test_exists_race_condition(self, mocker, tmpdir, typ): """Make sure there can't be a TOCTOU issue when creating the file. @@ -279,13 +277,12 @@ class TestCreatingDir: m.path.abspath = lambda x: x args = types.SimpleNamespace(basedir=str(tmpdir)) - standarddir.init(args) + standarddir._init_dirs(args) func = getattr(standarddir, typ) func() -@pytest.mark.usefixtures('reset_standarddir') class TestSystemData: """Test system data path.""" @@ -294,6 +291,7 @@ class TestSystemData: """Test that /usr/share/qutebrowser is used if path exists.""" monkeypatch.setattr('sys.platform', "linux") monkeypatch.setattr(os.path, 'exists', lambda path: True) + standarddir._init_dirs() assert standarddir.system_data() == "/usr/share/qutebrowser" @pytest.mark.linux @@ -301,16 +299,16 @@ class TestSystemData: fake_args): """Test that system-wide path isn't used on linux if path not exist.""" fake_args.basedir = str(tmpdir) - standarddir.init(fake_args) monkeypatch.setattr(os.path, 'exists', lambda path: False) + standarddir._init_dirs(fake_args) assert standarddir.system_data() == standarddir.data() def test_system_datadir_unsupportedos(self, monkeypatch, tmpdir, fake_args): """Test that system-wide path is not used on non-Linux OS.""" fake_args.basedir = str(tmpdir) - standarddir.init(fake_args) monkeypatch.setattr('sys.platform', "potato") + standarddir._init_dirs(fake_args) assert standarddir.system_data() == standarddir.data() @@ -411,3 +409,25 @@ class TestMoveWebEngineData: record = caplog.records[-1] expected = "Failed to move old QtWebEngine data/cache: error" assert record.message == expected + + +@pytest.mark.parametrize('with_args', [True, False]) +def test_init(mocker, tmpdir, with_args): + """Do some sanity checks for standarddir.init(). + + Things like _init_cachedir_tag() and _move_webengine_data() are tested in + more detail in other tests. + """ + assert standarddir._locations == {} + + if with_args: + args = types.SimpleNamespace(basedir=str(tmpdir)) + m = mocker.patch('qutebrowser.utils.standarddir._move_webengine_data') + else: + args = None + + standarddir.init(args) + + assert standarddir._locations != {} + if with_args: + assert m.called