Skip to content

Live config override directory#4781

Draft
tarek-y-ismail wants to merge 14 commits intoadd-config-file-store-adapterfrom
MIRENG-1761/live-config-override-directory
Draft

Live config override directory#4781
tarek-y-ismail wants to merge 14 commits intoadd-config-file-store-adapterfrom
MIRENG-1761/live-config-override-directory

Conversation

@tarek-y-ismail
Copy link
Copy Markdown
Contributor

What's new?

TODO

How to test

 ./build/bin/miral-test --gtest_filter=TestOverrideConfigFile*
 ./build/bin/miral-test --gtest_filter=LiveConfigIniFile*

Checklist

  • Tests added and pass
  • Adequate documentation added
  • (optional) Added Screenshots or videos

@tarek-y-ismail tarek-y-ismail self-assigned this Mar 18, 2026
@tarek-y-ismail
Copy link
Copy Markdown
Contributor Author

I did not realize I wrote 1600+ lines of code. Might be wise to split this into a PR for the IniFile changes and another for the ConfigFile changes

@tarek-y-ismail tarek-y-ismail force-pushed the MIRENG-1761/live-config-override-directory branch 2 times, most recently from f42e490 to 3bafb8a Compare March 25, 2026 14:21
@tarek-y-ismail tarek-y-ismail marked this pull request as ready for review March 25, 2026 14:21
@tarek-y-ismail tarek-y-ismail requested a review from a team as a code owner March 25, 2026 14:21
Copilot AI review requested due to automatic review settings March 25, 2026 14:21
@tarek-y-ismail tarek-y-ismail changed the base branch from main to live-config-ini-file-changes March 25, 2026 14:24
@tarek-y-ismail tarek-y-ismail marked this pull request as draft March 25, 2026 14:25
@tarek-y-ismail tarek-y-ismail force-pushed the MIRENG-1761/live-config-override-directory branch from 3bafb8a to 88e7411 Compare March 25, 2026 14:27
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support for loading a base config file plus a companion override directory (<config>.d/*.conf) and applying those overrides as a single “transaction” to the live config store (handlers called once after aggregating all inputs), with reload-on-change support.

Changes:

  • Extend miral::live_config::IniFile with load_files() and implement multi-stream parsing/override semantics (including explicit array clearing).
  • Extend miral::ConfigFile with an OverrideLoader constructor and an inotify-based watcher for override directories and relevant symlink targets.
  • Add/expand tests for multi-stream INI behavior and override-directory loading/reloading; update demo server example to use the new multi-file loader.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/miral/live_config_ini_file.cpp Implements multi-stream parsing, value provenance tracking, array clear semantics, and a load_files() entrypoint.
include/miral/miral/live_config_ini_file.h Exposes IniFile::load_files() in the public API.
src/miral/config_file.cpp Adds multi-file collection, override-directory discovery, and OverrideWatcher for reload-on-change.
include/miral/miral/config_file.h Adds OverrideLoader type + constructor overload to support multi-file loading.
tests/miral/live_config_ini_file.cpp Adds unit tests for override/aggregation semantics across multiple streams.
tests/miral/config_file.cpp Adds a large suite of tests covering override dir loading/reloading and symlink cases.
examples/mir_demo_server/server_example.cpp Switches demo server to load multi-file configs via load_files().

Comment thread src/miral/config_file.cpp Outdated
override_loader{load_config},
base_config_filename{base_config.filename().string()},
override_directory{base_config_filename + ".d"},
base_config_directory{config_directory(base_config.filename())},
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base_config_directory is derived from config_directory(base_config.filename()), which drops any explicit parent path the caller provided (e.g. /tmp/foo.conf). That causes override watching to monitor $XDG_CONFIG_HOME/$HOME/.config instead of the supplied directory. Use config_directory(base_config) so explicit paths behave correctly.

Suggested change
base_config_directory{config_directory(base_config.filename())},
base_config_directory{config_directory(base_config)},

Copilot uses AI. Check for mistakes.
Comment thread src/miral/config_file.cpp Outdated
Comment on lines 45 to 50
public:
/// Loader functor is passed both the open stream and the actual path (for use in reporting problems)
using Loader = std::function<void(std::istream& istream, std::filesystem::path const& path)>;
using OverrideLoader =
std::function<void(std::span<std::pair<std::unique_ptr<std::istream>, std::filesystem::path>>)>;

Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This header now uses std::span and std::pair/std::unique_ptr in the public API, but it doesn’t include <span> (and <utility> for std::pair). Relying on transitive includes makes the header non-self-contained and can break compilation for consumers.

Copilot uses AI. Check for mistakes.
#include <miral/test_server.h>
#include <miral/config_file.h>

#include <source_location>
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#include <source_location> appears unused in this test file (no references found). If the build treats warnings as errors, this can fail CI; please remove it or use it.

Suggested change
#include <source_location>

Copilot uses AI. Check for mistakes.
Comment thread src/miral/config_file.cpp Outdated
Comment on lines 48 to 60
@@ -54,6 +56,7 @@ class ConfigFile
};

ConfigFile(MirRunner& runner, std::filesystem::path file, Mode mode, Loader load_config);
ConfigFile(MirRunner& runner, std::filesystem::path file, Mode mode, OverrideLoader load_config);
~ConfigFile();
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding an overload that takes OverrideLoader makes call sites using generic lambdas (e.g. [](auto&... args){...}) ambiguous because such lambdas are callable with both (istream&, path) and (span<...>). Consider making the overload selection unambiguous (e.g. via a tag type, a differently-named constructor/factory, or documenting that callers should wrap lambdas in ConfigFile::Loader{...} / ConfigFile::OverrideLoader{...}).

Copilot uses AI. Check for mistakes.
@tarek-y-ismail
Copy link
Copy Markdown
Contributor Author

Ok, splitting IniFile changes out improved things a bit. But ConfigFile tests are almost 1000 lines, so the PR is still big...

@tarek-y-ismail tarek-y-ismail force-pushed the live-config-ini-file-changes branch 8 times, most recently from 7270cfd to 0009e12 Compare March 26, 2026 12:49
@tarek-y-ismail tarek-y-ismail force-pushed the MIRENG-1761/live-config-override-directory branch from 88e7411 to 3a5de42 Compare March 26, 2026 13:00
@tarek-y-ismail tarek-y-ismail force-pushed the live-config-ini-file-changes branch 2 times, most recently from ea3d4b1 to 49f4970 Compare March 26, 2026 14:42
@tarek-y-ismail
Copy link
Copy Markdown
Contributor Author

After a quick discussion with @Saviq, we're going to implement this without support for symlinks initially. That can come later.

@tarek-y-ismail tarek-y-ismail force-pushed the live-config-ini-file-changes branch from 49f4970 to ec5910c Compare March 26, 2026 15:10
@tarek-y-ismail tarek-y-ismail force-pushed the MIRENG-1761/live-config-override-directory branch from 3a5de42 to 4d8e03b Compare March 26, 2026 16:42
@tarek-y-ismail tarek-y-ismail force-pushed the MIRENG-1761/live-config-override-directory branch 2 times, most recently from 6208929 to db1da86 Compare April 16, 2026 15:10
@tarek-y-ismail tarek-y-ismail changed the base branch from live-config-ini-file-changes to add-config-file-store-adapter April 16, 2026 15:13
@tarek-y-ismail tarek-y-ismail force-pushed the add-config-file-store-adapter branch 2 times, most recently from 18f47be to 2801040 Compare April 16, 2026 15:47
@tarek-y-ismail tarek-y-ismail force-pushed the MIRENG-1761/live-config-override-directory branch from db1da86 to f48fb79 Compare April 16, 2026 15:53
@tarek-y-ismail tarek-y-ismail force-pushed the add-config-file-store-adapter branch from 2801040 to 5c5a932 Compare April 21, 2026 16:31
@tarek-y-ismail tarek-y-ismail force-pushed the MIRENG-1761/live-config-override-directory branch from 2c9295a to a7f2815 Compare April 21, 2026 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants