Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change ConfigFile to load after server init #3731

Closed
wants to merge 8 commits into from
Closed

Change ConfigFile to load after server init #3731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e9a8d04
Modify `wait_for_load` to allow passing or failing depending on the case
tarek-y-ismail Jan 30, 2025
635d093
Delay the initial load of `ConfigFile` until _after_ the server starts.
tarek-y-ismail Jan 28, 2025
ea6e997
Start server manually in `ConfigFile` tests
tarek-y-ismail Jan 30, 2025
6a0f1b9
Wrap `ConfigFiler::watcher` modifications in mutex locks
tarek-y-ismail Jan 30, 2025
6beed7f
Do not use `FAIL()` outside a test body
AlanGriffiths Jan 30, 2025
1279e61
Extract common initialization into an init method
AlanGriffiths Jan 30, 2025
5043d23
Fix cv usage
AlanGriffiths Jan 30, 2025
3d4a7de
tests: increase server init timeout for config_file
Saviq Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 50 additions & 42 deletions src/miral/config_file.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,9 @@ class miral::ConfigFile::Self
{
public:
Self(MirRunner& runner, path file, Mode mode, Loader load_config);

~Self() { std::lock_guard lock{mutex}; watcher.reset(); }
private:
std::mutex mutex;
std::shared_ptr<Watcher> watcher;
};

Expand All @@ -109,53 +110,60 @@ Watcher::Watcher(path file, miral::ConfigFile::Loader load_config) :

miral::ConfigFile::Self::Self(MirRunner& runner, path file, Mode mode, Loader load_config)
{
auto const filename{file.filename()};
auto const directory{config_directory(file)};
runner.add_start_callback(
[file, load_config=std::move(load_config), mode, this, &runner]
{
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...
std::vector<path> config_roots;
// 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...
std::vector<path> config_roots;

if (directory)
{
config_roots.push_back(directory.value());
}
if (directory)
{
config_roots.push_back(directory.value());
}

if (auto config_dirs = getenv("XDG_CONFIG_DIRS"))
{
std::istringstream config_stream{config_dirs};
for (std::string config_root; getline(config_stream, config_root, ':');)
{
config_roots.push_back(config_root);
}
}
else
{
config_roots.push_back("/etc/xdg");
}
if (auto config_dirs = getenv("XDG_CONFIG_DIRS"))
{
std::istringstream config_stream{config_dirs};
for (std::string config_root; getline(config_stream, config_root, ':');)
{
config_roots.push_back(config_root);
}
}
else
{
config_roots.push_back("/etc/xdg");
}

/* Read config file */
for (auto const& config_root : config_roots)
{
auto filepath = config_root / filename;
if (std::ifstream config_file{filepath})
{
load_config(config_file, filepath);
mir::log_debug("Loaded %s", filepath.c_str());
break;
}
}
/* Read config file */
for (auto const& config_root : config_roots)
{
auto filepath = config_root / filename;
if (std::ifstream config_file{filepath})
{
load_config(config_file, filepath);
mir::log_debug("Loaded %s", filepath.c_str());
break;
}
}

switch (mode)
{
case Mode::no_reloading:
break;
switch (mode)
{
case Mode::no_reloading:
break;

case Mode::reload_on_change:
watcher = std::make_shared<Watcher>(file, std::move(load_config));
watcher->register_handler(runner);
break;
}
case Mode::reload_on_change:
{
std::lock_guard lock{mutex};
watcher = std::make_shared<Watcher>(file, std::move(load_config));
watcher->register_handler(runner);
}
break;
}
});
}

miral::ConfigFile::ConfigFile(MirRunner& runner, path file, Mode mode, Loader load_config) :
Expand Down
81 changes: 56 additions & 25 deletions tests/miral/config_file.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class PendingLoad
{
std::unique_lock lock{mutex};

if (!cv.wait_for(lock, std::chrono::milliseconds{10}, [this] { return !pending_loads; }))
if (!cv.wait_for(lock, std::chrono::milliseconds{100}, [this] { return !pending_loads; }))
{
std::cerr << "wait_for_load() timed out" << std::endl;
}
Expand Down Expand Up @@ -89,6 +89,7 @@ struct TestConfigFile : PendingLoad, miral::TestServer

void SetUp() override
{
start_server_in_setup = false;
miral::TestServer::SetUp();
ON_CALL(*this, load(testing::_, testing::_)).WillByDefault([this]{ notify_load(); });
}
Expand All @@ -98,6 +99,36 @@ struct TestConfigFile : PendingLoad, miral::TestServer
reloading_config_file.reset();
miral::TestServer::TearDown();
}

void init_and_start(std::function<void(miral::MirRunner& runner)> const& f)
{
// The ConfigFile instance doesn't complete its setup until the start_callbacks run,
// so we need to wait for that. So we add our own callback and wait on that
std::mutex mutex;
std::condition_variable cv;
bool started = false;

invoke_runner(f);
invoke_runner([&](miral::MirRunner& runner)
{
runner.add_start_callback([&]()
{
{
std::lock_guard lock{mutex};
started = true;
}
cv.notify_one();
});
});

start_server();

std::unique_lock lock{mutex};
if (!cv.wait_for(lock, std::chrono::milliseconds{10}, [&] { return started; }))
{
std::cerr << "init_and_start() timed out" << std::endl;
}
}
};

char const* const home = "/tmp/test_reloading_config_file/home";
Expand Down Expand Up @@ -127,7 +158,7 @@ TEST_F(TestConfigFile, with_reload_on_change_and_no_file_nothing_is_loaded)
{
EXPECT_CALL(*this, load).Times(0);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -143,7 +174,7 @@ TEST_F(TestConfigFile, with_reload_on_change_and_a_file_something_is_loaded)

write_a_file();

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -156,7 +187,7 @@ TEST_F(TestConfigFile, with_reload_on_change_and_a_file_something_is_loaded)

TEST_F(TestConfigFile, with_reload_on_change_when_a_file_is_written_something_is_loaded)
{
invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -179,7 +210,7 @@ TEST_F(TestConfigFile, with_reload_on_change_each_time_a_file_is_rewritten_somet

write_a_file(); // Initial write

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -206,7 +237,7 @@ TEST_F(TestConfigFile, with_reload_on_change_when_config_home_unset_a_file_in_ho
write_config_in(xdg_conf_home);
write_config_in(home_config);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -227,7 +258,7 @@ TEST_F(TestConfigFile, with_reload_on_change_a_file_in_xdg_config_home_is_loaded
write_config_in(xdg_conf_home);
write_config_in(home_config);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -248,7 +279,7 @@ TEST_F(TestConfigFile, with_reload_on_change_a_file_in_xdg_config_home_is_reload
write_config_in(xdg_conf_home);
write_config_in(home_config);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -272,7 +303,7 @@ TEST_F(TestConfigFile, with_reload_on_change_a_config_in_xdg_conf_dir0_is_loaded

write_config_in(xdg_conf_dir0);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -296,7 +327,7 @@ TEST_F(TestConfigFile, with_reload_on_change_after_a_config_in_xdg_conf_dir0_is_
write_config_in(xdg_conf_dir1);
write_config_in(xdg_conf_dir0);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -321,7 +352,7 @@ TEST_F(TestConfigFile, with_reload_on_change_a_config_in_xdg_conf_dir0_is_loaded
write_config_in(xdg_conf_dir1);
write_config_in(xdg_conf_dir0);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -342,7 +373,7 @@ TEST_F(TestConfigFile, with_reload_on_change_a_config_in_xdg_conf_dir1_is_loaded
write_config_in(xdg_conf_dir2);
write_config_in(xdg_conf_dir1);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -362,7 +393,7 @@ TEST_F(TestConfigFile, with_reload_on_change_a_config_in_xdg_conf_dir2_is_loaded

write_config_in(xdg_conf_dir2);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -378,7 +409,7 @@ TEST_F(TestConfigFile, with_no_reloading_and_no_file_nothing_is_loaded)
{
EXPECT_CALL(*this, load).Times(0);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -394,7 +425,7 @@ TEST_F(TestConfigFile, with_no_reloading_and_a_file_something_is_loaded)

write_a_file();

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -407,7 +438,7 @@ TEST_F(TestConfigFile, with_no_reloading_and_a_file_something_is_loaded)

TEST_F(TestConfigFile, with_no_reloading_when_a_file_is_written_nothing_is_loaded)
{
invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -428,7 +459,7 @@ TEST_F(TestConfigFile, with_no_reloading_when_a_file_is_rewritten_nothing_is_rel

write_a_file(); // Initial write

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -451,7 +482,7 @@ TEST_F(TestConfigFile, with_no_reloading_when_config_home_unset_a_file_in_home_c
write_config_in(xdg_conf_home);
write_config_in(home_config);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -472,7 +503,7 @@ TEST_F(TestConfigFile, with_no_reloading_a_file_in_xdg_config_home_is_loaded)
write_config_in(xdg_conf_home);
write_config_in(home_config);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -493,7 +524,7 @@ TEST_F(TestConfigFile, with_no_reloading_a_file_in_xdg_config_home_is_not_reload
write_config_in(xdg_conf_home);
write_config_in(home_config);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -517,7 +548,7 @@ TEST_F(TestConfigFile, with_no_reloading_a_config_in_xdg_conf_dir0_is_loaded)

write_config_in(xdg_conf_dir0);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -541,7 +572,7 @@ TEST_F(TestConfigFile, with_no_reloading_after_a_config_in_xdg_conf_dir0_is_load
write_config_in(xdg_conf_dir1);
write_config_in(xdg_conf_dir0);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -566,7 +597,7 @@ TEST_F(TestConfigFile, with_no_reloading_a_config_in_xdg_conf_dir0_is_loaded_in_
write_config_in(xdg_conf_dir1);
write_config_in(xdg_conf_dir0);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -587,7 +618,7 @@ TEST_F(TestConfigFile, with_no_reloading_a_config_in_xdg_conf_dir1_is_loaded_in_
write_config_in(xdg_conf_dir2);
write_config_in(xdg_conf_dir1);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand All @@ -607,7 +638,7 @@ TEST_F(TestConfigFile, with_no_reloading_a_config_in_xdg_conf_dir2_is_loaded)

write_config_in(xdg_conf_dir2);

invoke_runner([this](miral::MirRunner& runner)
init_and_start([this](miral::MirRunner& runner)
{
reloading_config_file = ConfigFile{
runner,
Expand Down
Loading