From e4dc3dbf5d4bfe369257904a0841d002348566b3 Mon Sep 17 00:00:00 2001 From: Alan Griffiths Date: Tue, 20 Aug 2024 14:05:50 +0100 Subject: [PATCH] Add non-reloading option --- include/miral/miral/config_file.h | 21 ++- src/miral/config_file.cpp | 47 +++-- tests/miral/config_file.cpp | 291 ++++++++++++++++++++++++++++-- 3 files changed, 325 insertions(+), 34 deletions(-) diff --git a/include/miral/miral/config_file.h b/include/miral/miral/config_file.h index 409a4b9a1f6..5edf291247c 100644 --- a/include/miral/miral/config_file.h +++ b/include/miral/miral/config_file.h @@ -30,12 +30,14 @@ namespace miral /** * Utility to locate and monitor a configuration file via the XDG Base Directory * Specification. Vis: ($XDG_CONFIG_HOME or $HOME/.config followed by - * $XDG_CONFIG_DIRS). - * The user-specific configuration file base ($XDG_CONFIG_HOME or $HOME/.config) - * is monitored for subsequent changes (but not directories in $XDG_CONFIG_DIRS) + * $XDG_CONFIG_DIRS). If, instead of a filename, a path is given, then the base + * directories are not applied. * - * If, instead of a filename, a path is given, then the base directories are not - * applied and the supplied path monitored for changes. + * If mode is `no_reloading`, then the file is loaded on startup and not reloaded + * + * If mode is `reload_on_change`, then the file is loaded on startup and either + * the user-specific configuration file base ($XDG_CONFIG_HOME or $HOME/.config), + * or the supplied path is monitored for changes. * \remark MirAL 5.1 */ class ConfigFile @@ -44,7 +46,14 @@ class ConfigFile /// Loader functor is passed both the open stream and the actual path (for use in reporting problems) using Loader = std::function; - ConfigFile(MirRunner& runner, std::filesystem::path file, Loader load_config); + /// Mode of reloading + enum class Mode + { + no_reloading, + reload_on_change + }; + + ConfigFile(MirRunner& runner, std::filesystem::path file, Mode mode, Loader load_config); ~ConfigFile(); private: diff --git a/src/miral/config_file.cpp b/src/miral/config_file.cpp index 1ae807de8df..de9bef0d8c2 100644 --- a/src/miral/config_file.cpp +++ b/src/miral/config_file.cpp @@ -65,25 +65,34 @@ auto watch_descriptor(mir::Fd const& inotify_fd, std::optional const& path return inotify_add_watch(inotify_fd, path.value().c_str(), IN_CLOSE_WRITE | IN_CREATE | IN_MOVED_TO); } -} -class miral::ConfigFile::Self +class Watcher { public: - Self(MirRunner& runner, path file, Loader load_config); + using Loader = miral::ConfigFile::Loader; + Watcher(miral::MirRunner& runner, path file, miral::ConfigFile::Loader load_config); -private: mir::Fd const inotify_fd; Loader const load_config; - std::filesystem::path const filename; - std::optional const directory; + path const filename; + std::optional const directory; std::optional const directory_watch_descriptor; - void register_handler(MirRunner& runner); + void register_handler(miral::MirRunner& runner); std::unique_ptr fd_handle; }; +} + +class miral::ConfigFile::Self +{ +public: + Self(MirRunner& runner, path file, Mode mode, Loader load_config); + +private: + std::unique_ptr watcher; +}; -miral::ConfigFile::Self::Self(MirRunner& runner, path file, Loader load_config) : +Watcher::Watcher(miral::MirRunner& runner, path file, miral::ConfigFile::Loader load_config) : inotify_fd{inotify_init1(IN_CLOEXEC)}, load_config{load_config}, filename{file.filename()}, @@ -96,6 +105,12 @@ miral::ConfigFile::Self::Self(MirRunner& runner, path file, Loader load_config) { mir::log_debug("Monitoring %s for configuration changes", (directory.value()/filename).c_str()); } +} + +miral::ConfigFile::Self::Self(MirRunner& runner, path file, Mode mode, Loader load_config) +{ + auto const filename{file.filename()}; + auto const directory{config_directory(file)}; // With C++26 we should be able to use the optional directory as a range to // initialize config_roots. Until then, we'll just do it the long way... @@ -130,16 +145,26 @@ miral::ConfigFile::Self::Self(MirRunner& runner, path file, Loader load_config) break; } } + + switch (mode) + { + case Mode::no_reloading: + break; + + case Mode::reload_on_change: + watcher = std::make_unique(runner, file, std::move(load_config)); + break; + } } -miral::ConfigFile::ConfigFile(MirRunner& runner, path file, Loader load_config) : - self{std::make_shared(runner, file, load_config)} +miral::ConfigFile::ConfigFile(MirRunner& runner, path file, Mode mode, Loader load_config) : + self{std::make_shared(runner, file, mode, load_config)} { } miral::ConfigFile::~ConfigFile() = default; -void miral::ConfigFile::Self::register_handler(MirRunner& runner) +void Watcher::register_handler(miral::MirRunner& runner) { if (directory_watch_descriptor) { diff --git a/tests/miral/config_file.cpp b/tests/miral/config_file.cpp index 02a1b0f741c..f56bd6ad474 100644 --- a/tests/miral/config_file.cpp +++ b/tests/miral/config_file.cpp @@ -44,9 +44,9 @@ class PendingLoad { std::unique_lock lock{mutex}; - if (!cv.wait_for(lock, std::chrono::milliseconds{1000}, [this] { return !pending_loads; })) + if (!cv.wait_for(lock, std::chrono::milliseconds{10}, [this] { return !pending_loads; })) { - std::cerr << "wait_for_load() failed" << std::endl; + std::cerr << "wait_for_load() timed out" << std::endl; } } @@ -66,9 +66,9 @@ class PendingLoad bool pending_loads = false; }; -struct TestReloadingConfigFile : miral::TestServer, PendingLoad +struct TestConfigFile : miral::TestServer, PendingLoad { - TestReloadingConfigFile(); + TestConfigFile(); MOCK_METHOD(void, load, (std::istream& in, std::filesystem::path path), ()); std::optional reloading_config_file; @@ -101,7 +101,7 @@ char const* const xdg_conf_dir0 = "/tmp/test_reloading_config_file/xdg_conf_dir_ char const* const xdg_conf_dir1 = "/tmp/test_reloading_config_file/xdg_conf_dir_one"; char const* const xdg_conf_dir2 = "/tmp/test_reloading_config_file/xdg_conf_dir_two"; -TestReloadingConfigFile::TestReloadingConfigFile() +TestConfigFile::TestConfigFile() { std::filesystem::remove_all("/tmp/test_reloading_config_file/"); @@ -117,7 +117,7 @@ TestReloadingConfigFile::TestReloadingConfigFile() } -TEST_F(TestReloadingConfigFile, with_no_file_nothing_is_loaded) +TEST_F(TestConfigFile, with_reload_on_change_and_no_file_nothing_is_loaded) { EXPECT_CALL(*this, load).Times(0); @@ -126,11 +126,12 @@ TEST_F(TestReloadingConfigFile, with_no_file_nothing_is_loaded) reloading_config_file = ConfigFile{ runner, no_such_file, + ConfigFile::Mode::reload_on_change, [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; }); } -TEST_F(TestReloadingConfigFile, with_a_file_something_is_loaded) +TEST_F(TestConfigFile, with_reload_on_change_and_a_file_something_is_loaded) { EXPECT_CALL(*this, load).Times(1); @@ -141,18 +142,20 @@ TEST_F(TestReloadingConfigFile, with_a_file_something_is_loaded) reloading_config_file = ConfigFile{ runner, a_file, + ConfigFile::Mode::reload_on_change, [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; }); wait_for_load(); } -TEST_F(TestReloadingConfigFile, when_a_file_is_written_something_is_loaded) +TEST_F(TestConfigFile, with_reload_on_change_when_a_file_is_written_something_is_loaded) { invoke_runner([this](miral::MirRunner& runner) { reloading_config_file = ConfigFile{ runner, a_file, + ConfigFile::Mode::reload_on_change, [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; }); @@ -162,7 +165,7 @@ TEST_F(TestReloadingConfigFile, when_a_file_is_written_something_is_loaded) wait_for_load(); } -TEST_F(TestReloadingConfigFile, each_time_a_file_is_rewritten_something_is_loaded) +TEST_F(TestConfigFile, with_reload_on_change_each_time_a_file_is_rewritten_something_is_loaded) { auto const times = 42; @@ -175,6 +178,7 @@ TEST_F(TestReloadingConfigFile, each_time_a_file_is_rewritten_something_is_loade reloading_config_file = ConfigFile{ runner, a_file, + ConfigFile::Mode::reload_on_change, [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; }); @@ -187,7 +191,7 @@ TEST_F(TestReloadingConfigFile, each_time_a_file_is_rewritten_something_is_loade wait_for_load(); } -TEST_F(TestReloadingConfigFile, when_config_home_unset_a_file_in_home_config_is_loaded) +TEST_F(TestConfigFile, with_reload_on_change_when_config_home_unset_a_file_in_home_config_is_loaded) { add_to_environment("XDG_CONFIG_HOME", nullptr); using testing::_; @@ -201,13 +205,14 @@ TEST_F(TestReloadingConfigFile, when_config_home_unset_a_file_in_home_config_is_ reloading_config_file = ConfigFile{ runner, config_file, + ConfigFile::Mode::reload_on_change, [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; }); wait_for_load(); } -TEST_F(TestReloadingConfigFile, a_file_in_xdg_config_home_is_loaded) +TEST_F(TestConfigFile, with_reload_on_change_a_file_in_xdg_config_home_is_loaded) { using testing::_; EXPECT_CALL(*this, load(_, xdg_conf_home/config_file)).Times(1); @@ -221,13 +226,14 @@ TEST_F(TestReloadingConfigFile, a_file_in_xdg_config_home_is_loaded) reloading_config_file = ConfigFile{ runner, config_file, + ConfigFile::Mode::reload_on_change, [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; }); wait_for_load(); } -TEST_F(TestReloadingConfigFile, a_file_in_xdg_config_home_is_reloaded) +TEST_F(TestConfigFile, with_reload_on_change_a_file_in_xdg_config_home_is_reloaded) { using testing::_; EXPECT_CALL(*this, load(_, xdg_conf_home/config_file)).Times(2); @@ -241,6 +247,7 @@ TEST_F(TestReloadingConfigFile, a_file_in_xdg_config_home_is_reloaded) reloading_config_file = ConfigFile{ runner, config_file, + ConfigFile::Mode::reload_on_change, [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; }); @@ -252,7 +259,7 @@ TEST_F(TestReloadingConfigFile, a_file_in_xdg_config_home_is_reloaded) wait_for_load(); } -TEST_F(TestReloadingConfigFile, a_config_in_xdg_conf_dir0_is_loaded) +TEST_F(TestConfigFile, with_reload_on_change_a_config_in_xdg_conf_dir0_is_loaded) { using testing::_; EXPECT_CALL(*this, load(_, xdg_conf_dir0/config_file)).Times(1); @@ -264,13 +271,14 @@ TEST_F(TestReloadingConfigFile, a_config_in_xdg_conf_dir0_is_loaded) reloading_config_file = ConfigFile{ runner, config_file, + ConfigFile::Mode::reload_on_change, [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; }); wait_for_load(); } -TEST_F(TestReloadingConfigFile, after_a_config_in_xdg_conf_dir0_is_loaded_a_new_config_in_xdg_conf_home_is_loaded) +TEST_F(TestConfigFile, with_reload_on_change_after_a_config_in_xdg_conf_dir0_is_loaded_a_new_config_in_xdg_conf_home_is_loaded) { using testing::_; @@ -287,6 +295,7 @@ TEST_F(TestReloadingConfigFile, after_a_config_in_xdg_conf_dir0_is_loaded_a_new_ reloading_config_file = ConfigFile{ runner, config_file, + ConfigFile::Mode::reload_on_change, [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; }); @@ -296,7 +305,7 @@ TEST_F(TestReloadingConfigFile, after_a_config_in_xdg_conf_dir0_is_loaded_a_new_ wait_for_load(); } -TEST_F(TestReloadingConfigFile, a_config_in_xdg_conf_dir0_is_loaded_in_preference_to_dir1_or_2) +TEST_F(TestConfigFile, with_reload_on_change_a_config_in_xdg_conf_dir0_is_loaded_in_preference_to_dir1_or_2) { using testing::_; @@ -311,13 +320,14 @@ TEST_F(TestReloadingConfigFile, a_config_in_xdg_conf_dir0_is_loaded_in_preferenc reloading_config_file = ConfigFile{ runner, config_file, + ConfigFile::Mode::reload_on_change, [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; }); wait_for_load(); } -TEST_F(TestReloadingConfigFile, a_config_in_xdg_conf_dir1_is_loaded_in_preference_to_dir2) +TEST_F(TestConfigFile, with_reload_on_change_a_config_in_xdg_conf_dir1_is_loaded_in_preference_to_dir2) { using testing::_; @@ -331,13 +341,14 @@ TEST_F(TestReloadingConfigFile, a_config_in_xdg_conf_dir1_is_loaded_in_preferenc reloading_config_file = ConfigFile{ runner, config_file, + ConfigFile::Mode::reload_on_change, [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; }); wait_for_load(); } -TEST_F(TestReloadingConfigFile, a_config_in_xdg_conf_dir2_is_loaded) +TEST_F(TestConfigFile, with_reload_on_change_a_config_in_xdg_conf_dir2_is_loaded) { using testing::_; @@ -350,6 +361,252 @@ TEST_F(TestReloadingConfigFile, a_config_in_xdg_conf_dir2_is_loaded) reloading_config_file = ConfigFile{ runner, config_file, + ConfigFile::Mode::reload_on_change, + [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; + }); + + wait_for_load(); +} + +TEST_F(TestConfigFile, with_no_reloading_and_no_file_nothing_is_loaded) +{ + EXPECT_CALL(*this, load).Times(0); + + invoke_runner([this](miral::MirRunner& runner) + { + reloading_config_file = ConfigFile{ + runner, + no_such_file, + ConfigFile::Mode::no_reloading, + [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; + }); +} + +TEST_F(TestConfigFile, with_no_reloading_and_a_file_something_is_loaded) +{ + EXPECT_CALL(*this, load).Times(1); + + write_a_file(); + + invoke_runner([this](miral::MirRunner& runner) + { + reloading_config_file = ConfigFile{ + runner, + a_file, + ConfigFile::Mode::no_reloading, + [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; + }); + wait_for_load(); +} + +TEST_F(TestConfigFile, with_no_reloading_when_a_file_is_written_nothing_is_loaded) +{ + invoke_runner([this](miral::MirRunner& runner) + { + reloading_config_file = ConfigFile{ + runner, + a_file, + ConfigFile::Mode::no_reloading, + [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; + }); + + EXPECT_CALL(*this, load).Times(0); + + write_a_file(); + wait_for_load(); +} + +TEST_F(TestConfigFile, with_no_reloading_when_a_file_is_rewritten_nothing_is_reloaded) +{ + EXPECT_CALL(*this, load).Times(1); + + write_a_file(); // Initial write + + invoke_runner([this](miral::MirRunner& runner) + { + reloading_config_file = ConfigFile{ + runner, + a_file, + ConfigFile::Mode::no_reloading, + [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; + }); + + wait_for_load(); + write_a_file(); + wait_for_load(); +} + +TEST_F(TestConfigFile, with_no_reloading_when_config_home_unset_a_file_in_home_config_is_loaded) +{ + add_to_environment("XDG_CONFIG_HOME", nullptr); + using testing::_; + EXPECT_CALL(*this, load(_, home_config/config_file)).Times(1); + + write_config_in(xdg_conf_home); + write_config_in(home_config); + + invoke_runner([this](miral::MirRunner& runner) + { + reloading_config_file = ConfigFile{ + runner, + config_file, + ConfigFile::Mode::no_reloading, + [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; + }); + + wait_for_load(); +} + +TEST_F(TestConfigFile, with_no_reloading_a_file_in_xdg_config_home_is_loaded) +{ + using testing::_; + EXPECT_CALL(*this, load(_, xdg_conf_home/config_file)).Times(1); + + write_config_in(xdg_conf_dir0); + write_config_in(xdg_conf_home); + write_config_in(home_config); + + invoke_runner([this](miral::MirRunner& runner) + { + reloading_config_file = ConfigFile{ + runner, + config_file, + ConfigFile::Mode::no_reloading, + [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; + }); + + wait_for_load(); +} + +TEST_F(TestConfigFile, with_no_reloading_a_file_in_xdg_config_home_is_not_reloaded) +{ + using testing::_; + EXPECT_CALL(*this, load(_, xdg_conf_home/config_file)).Times(1); + + write_config_in(xdg_conf_dir0); + write_config_in(xdg_conf_home); + write_config_in(home_config); + + invoke_runner([this](miral::MirRunner& runner) + { + reloading_config_file = ConfigFile{ + runner, + config_file, + ConfigFile::Mode::no_reloading, + [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; + }); + + wait_for_load(); + + write_config_in(xdg_conf_dir0); + write_config_in(xdg_conf_home); + write_config_in(home_config); + wait_for_load(); +} + +TEST_F(TestConfigFile, with_no_reloading_a_config_in_xdg_conf_dir0_is_loaded) +{ + using testing::_; + EXPECT_CALL(*this, load(_, xdg_conf_dir0/config_file)).Times(1); + + write_config_in(xdg_conf_dir0); + + invoke_runner([this](miral::MirRunner& runner) + { + reloading_config_file = ConfigFile{ + runner, + config_file, + ConfigFile::Mode::no_reloading, + [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; + }); + + wait_for_load(); +} + +TEST_F(TestConfigFile, with_no_reloading_after_a_config_in_xdg_conf_dir0_is_loaded_a_new_config_in_xdg_conf_home_is_not_loaded) +{ + using testing::_; + + testing::InSequence sequence; + EXPECT_CALL(*this, load(_, xdg_conf_dir0/config_file)).Times(1); + EXPECT_CALL(*this, load(_, xdg_conf_home/config_file)).Times(0); + + write_config_in(xdg_conf_dir2); + write_config_in(xdg_conf_dir1); + write_config_in(xdg_conf_dir0); + + invoke_runner([this](miral::MirRunner& runner) + { + reloading_config_file = ConfigFile{ + runner, + config_file, + ConfigFile::Mode::no_reloading, + [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; + }); + + wait_for_load(); + + write_config_in(xdg_conf_home); + wait_for_load(); +} + +TEST_F(TestConfigFile, with_no_reloading_a_config_in_xdg_conf_dir0_is_loaded_in_preference_to_dir1_or_2) +{ + using testing::_; + + EXPECT_CALL(*this, load(_, xdg_conf_dir0/config_file)).Times(1); + + write_config_in(xdg_conf_dir2); + write_config_in(xdg_conf_dir1); + write_config_in(xdg_conf_dir0); + + invoke_runner([this](miral::MirRunner& runner) + { + reloading_config_file = ConfigFile{ + runner, + config_file, + ConfigFile::Mode::no_reloading, + [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; + }); + + wait_for_load(); +} + +TEST_F(TestConfigFile, with_no_reloading_a_config_in_xdg_conf_dir1_is_loaded_in_preference_to_dir2) +{ + using testing::_; + + EXPECT_CALL(*this, load(_, xdg_conf_dir1/config_file)).Times(1); + + write_config_in(xdg_conf_dir2); + write_config_in(xdg_conf_dir1); + + invoke_runner([this](miral::MirRunner& runner) + { + reloading_config_file = ConfigFile{ + runner, + config_file, + ConfigFile::Mode::no_reloading, + [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; + }); + + wait_for_load(); +} + +TEST_F(TestConfigFile, with_no_reloading_a_config_in_xdg_conf_dir2_is_loaded) +{ + using testing::_; + + EXPECT_CALL(*this, load(_, xdg_conf_dir2/config_file)).Times(1); + + write_config_in(xdg_conf_dir2); + + invoke_runner([this](miral::MirRunner& runner) + { + reloading_config_file = ConfigFile{ + runner, + config_file, + ConfigFile::Mode::no_reloading, [this](std::istream& in, std::filesystem::path path) { load(in, path); }}; });