Simpler config override#4926
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new MirAL live-config store that can load and merge configuration from multiple ini streams (base + override layers) in a single transaction, aligning with the goal of simplifying layered config overrides.
Changes:
- Added
miral::live_config::IniFileWithOverridesas a public MirALStoreimplementation that loads multiple ini streams with last-writer-wins scalars and accumulative arrays. - Added a focused gtest suite validating override precedence, array accumulation/clearing, presets, reload behavior, and handler/done callback semantics.
- Wired the new API into build/packaging and exported the ABI symbols for MirAL 5.8.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/miral/ini_file_with_overrides.cpp | Adds unit tests covering multi-file override semantics and handler behavior. |
| tests/miral/CMakeLists.txt | Includes the new test file in miral-test. |
| src/miral/ini_file_with_overrides.cpp | Implements parsing/loading of ordered ini streams into a single BasicStore transaction. |
| include/miral/miral/ini_file_with_overrides.h | Adds the new public MirAL API for layered ini loading. |
| src/miral/CMakeLists.txt | Builds/installs the new MirAL implementation + header. |
| src/miral/symbols.map | Exports the new symbols under MIRAL_5.8. |
| debian/libmiral7.symbols | Updates Debian symbol tracking for the new ABI additions. |
| namespace | ||
| { | ||
| auto trim_start_and_end(std::string_view string) -> std::string_view | ||
| { | ||
| constexpr auto whitespace = " \t\n\r\f\v"; | ||
|
|
||
| auto const first_non_space = string.find_first_not_of(whitespace); | ||
| if (first_non_space == std::string::npos) | ||
| { | ||
| return string.substr(0, 0); | ||
| } | ||
|
|
||
| auto const last_non_space = string.find_last_not_of(whitespace); | ||
| return string.substr(first_non_space, last_non_space - first_non_space + 1); | ||
| } |
There was a problem hiding this comment.
True, can be moved to a shared header/source. Will do that if this comes to pass.
AlanGriffiths
left a comment
AlanGriffiths
left a comment
There was a problem hiding this comment.
Not sure what the right load signature is, but shared an idea
RAOF
left a comment
RAOF
left a comment
There was a problem hiding this comment.
This is also reasonable, once Alan's comments are addressed.
|
Passing a |
For what it's worth, I agree with this, but don't have as strong an opinion as Alan; I'm fine with |
|
Let me offer another approach that avoids abusing using FileLoader = std::movable_function<void(path, istream&)>;
using FileDropped= std::movable_function<void(path)>;
class FileChanges
{
public:
virtual void for_each(FileLoader unchanged, FileLoader moved_or_new, FileLoader modified, FileDropped dropped) const = 0;
};
void load(FileChanges const& changes); |
|
I continue to prefer #4926 (comment) (although possibly with |
|
I've explored the two suggestions and some others. Both should work, but I really couldn't choose between either so I just picked one randomly and massaged it into the code. Tell me what you think. |
tarek-y-ismail
force-pushed
the
simpler-config-override
branch
from
d6ceab2 to
5fa2180
Compare
RAOF
left a comment
RAOF
left a comment
There was a problem hiding this comment.
Yeah, looks fine; some nits.
tarek-y-ismail
force-pushed
the
simpler-config-override
branch
from
70e03ca to
4695d95
Compare
RAOF
left a comment
RAOF
left a comment
There was a problem hiding this comment.
The interface looks fine to me; a couple more nits in the implementation though.
AlanGriffiths
left a comment
AlanGriffiths
left a comment
There was a problem hiding this comment.
Just a little polish needed
AlanGriffiths
left a comment
AlanGriffiths
left a comment
There was a problem hiding this comment.
The blocking problem is publishing the FileChanges constructor. It is public, the symbol should be available.
At some point in the future we have the option of providing a way to supply a std::unique_ptr<Context> through a public builder. There's no need to cause confusion by having a public ctor with no corresponding symbol
AlanGriffiths
left a comment
AlanGriffiths
left a comment
There was a problem hiding this comment.
This looks fine. (A few optional nits)
BUT we should sort out the git history before landing
OverridesList is an ordered, immutable list of file-change events (unchanged, new, modified, or dropped) that can be iterated to drive config reloads. An OverridesListBuilder is provided (in miral-internal) so that callers can push events and then call build() to obtain a ready-to-iterate OverridesList. Each event stores a file_opener callback that opens the file on demand and calls the supplied Loader; if opening fails the event is treated as a drop. Renamed-file events are no longer modelled; callers should emit a drop followed by a fresh event instead.
The trim-whitespace helper and per-line parser loop that lived inside
IniFile::Self::load_file() are pulled out into parse_ini() in
live_config_ini_file_common.{h,cpp} (a miral-internal translation unit).
IniFileWithOverrides will reuse the same helper in a subsequent commit,
avoiding code duplication.
IniFileWithOverrides extends the existing IniFile concept to support
multiple layered configuration files. Its load(OverridesList const&) method
iterates all file-change events in the OverridesList in push order, merging
the results before dispatching attribute handlers.
Merge semantics:
- Scalar keys use last-writer-wins: a later (higher-priority) file's value
replaces any earlier value.
- Array keys accumulate across files; an empty assignment (key=) in a
later file clears all values accumulated from earlier files.
- on_done fires exactly once per load() call.
Covers: - Later files override earlier scalar values (last-writer-wins) - Array values accumulate across files - An empty assignment in a later file clears prior array entries - Mixed scalar + array keys across multiple files - on_done fires exactly once per load(), even with multiple files - Preset values are returned when no file provides the key - Keys absent from all files invoke handlers with std::nullopt - IniFile (single-file) still works after the parse_ini() refactor
Add MIRAL_5.8 entries to both symbols.map (the linker version script) and debian/libmiral7.symbols for all public and vtable/typeinfo symbols introduced by OverridesList and IniFileWithOverrides.
tarek-y-ismail
force-pushed
the
simpler-config-override
branch
from
da0b587 to
f0370f6
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Related #4781, #4858, #4830
What's new?
ConfigAggregatorandIniFile, producing a simpler implementation.How to test
<path/to/build>/bin/miral-test/miral-test --gtest_filter=IniFileWithOverridesTest*Checklist