From 9367dc70a8144d0417f3d90d2473c759b422d0c8 Mon Sep 17 00:00:00 2001
From: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>
Date: Mon, 9 Sep 2024 11:52:35 +1000
Subject: [PATCH] Configuration: Split options into global and per-module
 configuration.

This lets each module define its own options, without needing the option names
to be unique across all modules.

Also organises the module-specific options into their own sections at the end
of the help text.
---
 include/platform/mir/options/configuration.h  |  21 +-
 .../mir/options/default_configuration.h       |  48 ++++-
 include/platform/mir/options/program_option.h |   2 +-
 .../server/mir/default_server_configuration.h |   8 +-
 .../options/default_configuration.cpp         | 195 +++++++++++++++++-
 src/platform/options/program_option.cpp       |  21 +-
 src/server/default_server_configuration.cpp   |  11 +-
 src/server/graphics/default_configuration.cpp |  10 +-
 src/server/graphics/platform_probe.cpp        |  58 +++---
 src/server/graphics/platform_probe.h          |  12 +-
 src/server/server.cpp                         |   2 +-
 .../server_configuration_options.cpp          |  34 +++
 .../command_line_server_configuration.cpp     |   1 -
 .../graphics/test_platform_prober.cpp         |  75 ++++---
 .../options/test_program_option.cpp           |  22 --
 15 files changed, 399 insertions(+), 121 deletions(-)

diff --git a/include/platform/mir/options/configuration.h b/include/platform/mir/options/configuration.h
index f4af7a7af28..f37a2bbf5be 100644
--- a/include/platform/mir/options/configuration.h
+++ b/include/platform/mir/options/configuration.h
@@ -23,6 +23,8 @@
 
 namespace mir
 {
+class SharedLibrary;
+
 namespace options
 {
 extern char const* const arw_server_socket_opt;
@@ -66,17 +68,24 @@ extern char const* const auto_console;
 
 extern char const* const vt_option_name;
 
-class Configuration
+class OptionsProvider
 {
 public:
-    virtual std::shared_ptr<options::Option> the_options() const = 0;
+    /**
+     * The options not associated with a specific loaded module
+     */
+    virtual std::shared_ptr<options::Option> global_options() const = 0;
+    /**
+     * All options, including those added by the specified module
+     */
+    virtual auto the_options_for(SharedLibrary const& module) const -> std::shared_ptr<options::Option> = 0;
 
 protected:
+    OptionsProvider() = default;
+    virtual ~OptionsProvider() = default;
 
-    Configuration() = default;
-    virtual ~Configuration() = default;
-    Configuration(Configuration const&) = delete;
-    Configuration& operator=(Configuration const&) = delete;
+    OptionsProvider(OptionsProvider const&) = delete;
+    OptionsProvider& operator=(OptionsProvider const&) = delete;
 };
 }
 }
diff --git a/include/platform/mir/options/default_configuration.h b/include/platform/mir/options/default_configuration.h
index cb3c9d3ace5..6315d921c15 100644
--- a/include/platform/mir/options/default_configuration.h
+++ b/include/platform/mir/options/default_configuration.h
@@ -19,15 +19,15 @@
 
 #include "mir/options/configuration.h"
 #include "mir/options/program_option.h"
+#include "mir/shared_library.h"
 #include <boost/program_options/options_description.hpp>
 #include <vector>
 
 namespace mir
 {
-class SharedLibrary;
 namespace options
 {
-class DefaultConfiguration : public Configuration
+class DefaultConfiguration : public OptionsProvider
 {
 public:
     DefaultConfiguration(int argc, char const* argv[]);
@@ -54,7 +54,8 @@ class DefaultConfiguration : public Configuration
 
     void add_platform_options();
     // accessed via the base interface, when access to add_options() has been "lost"
-    std::shared_ptr<options::Option> the_options() const override;
+    std::shared_ptr<options::Option> global_options() const override;
+    auto the_options_for(SharedLibrary const& module) const -> std::shared_ptr<Option> override;
 
     virtual void parse_arguments(
         boost::program_options::options_description desc,
@@ -74,6 +75,47 @@ class DefaultConfiguration : public Configuration
     char const** const argv;
     std::function<void(int argc, char const* const* argv)> const unparsed_arguments_handler;
     std::shared_ptr<boost::program_options::options_description> const program_options;
+
+    template<typename Value>
+    class LibraryMap
+    {
+    private:
+        std::vector<std::pair<mir::SharedLibrary::Handle, Value>> values;
+
+        auto find(SharedLibrary const&) -> decltype(values)::iterator;
+        auto find(SharedLibrary const&) const -> decltype(values)::const_iterator;
+    public:
+        LibraryMap() = default;
+        ~LibraryMap() = default;
+
+        auto operator[](SharedLibrary const& lib) -> Value&;
+
+        auto at(SharedLibrary const& lib) const -> Value const&;
+
+        auto begin() const -> decltype(values)::const_iterator;
+        auto end() const -> decltype(values)::const_iterator;
+
+        template<typename ...Args>
+        auto emplace(SharedLibrary const& lib, Args&& ...args) -> std::pair<typename decltype(values)::iterator, bool>
+        {
+            auto existing = find(lib);
+
+            if (existing != values.end())
+            {
+                return {existing, false};
+            }
+
+            values.emplace_back(
+                std::piecewise_construct,
+                std::forward_as_tuple(lib.get_handle()),
+                std::forward<Args>(args)...);
+
+            return {values.end() - 1, true};
+        }
+    };
+
+    LibraryMap<boost::program_options::options_description> module_options_desc;
+    LibraryMap<std::shared_ptr<Option>> mutable module_options;
     std::shared_ptr<Option> mutable options;
 };
 }
diff --git a/include/platform/mir/options/program_option.h b/include/platform/mir/options/program_option.h
index dbfe05d0450..37f7571ba90 100644
--- a/include/platform/mir/options/program_option.h
+++ b/include/platform/mir/options/program_option.h
@@ -39,7 +39,7 @@ class ProgramOption : public Option
 
     void parse_environment(
         boost::program_options::options_description const& description,
-        char const* prefix);
+        std::function<std::string(std::string)> env_var_name_mapper);
 
     void parse_file(
         boost::program_options::options_description const& description,
diff --git a/src/include/server/mir/default_server_configuration.h b/src/include/server/mir/default_server_configuration.h
index f1d05bb7d5b..e62866e3549 100644
--- a/src/include/server/mir/default_server_configuration.h
+++ b/src/include/server/mir/default_server_configuration.h
@@ -17,6 +17,7 @@
 #define MIR_DEFAULT_SERVER_CONFIGURATION_H_
 
 #include "mir/cached_ptr.h"
+#include "mir/options/option.h"
 #include "mir/server_configuration.h"
 #include "mir/shell/window_manager_builder.h"
 
@@ -137,7 +138,7 @@ class Logger;
 namespace options
 {
 class Option;
-class Configuration;
+class OptionsProvider;
 }
 
 namespace report
@@ -160,7 +161,7 @@ class DefaultServerConfiguration : public virtual ServerConfiguration
 {
 public:
     DefaultServerConfiguration(int argc, char const* argv[]);
-    explicit DefaultServerConfiguration(std::shared_ptr<options::Configuration> const& configuration_options);
+    explicit DefaultServerConfiguration(std::shared_ptr<options::OptionsProvider> const& configuration_options);
 
     /** @name DisplayServer dependencies
      * dependencies of DisplayServer on the rest of the Mir
@@ -347,6 +348,7 @@ class DefaultServerConfiguration : public virtual ServerConfiguration
 
 protected:
     std::shared_ptr<options::Option> the_options() const;
+    auto the_options_provider() const -> std::shared_ptr<options::OptionsProvider>;
     std::shared_ptr<input::DefaultInputDeviceHub>  the_default_input_device_hub();
     std::shared_ptr<graphics::DisplayConfigurationObserver> the_display_configuration_observer();
     std::shared_ptr<input::SeatObserver> the_seat_observer();
@@ -434,7 +436,7 @@ class DefaultServerConfiguration : public virtual ServerConfiguration
     std::shared_ptr<DecorationStrategy> decoration_strategy;
 
 private:
-    std::shared_ptr<options::Configuration> const configuration_options;
+    std::shared_ptr<options::OptionsProvider> const configuration_options;
     std::shared_ptr<input::EventFilter> default_filter;
     CachedPtr<ObserverMultiplexer<graphics::DisplayConfigurationObserver>>
         display_configuration_observer_multiplexer;
diff --git a/src/platform/options/default_configuration.cpp b/src/platform/options/default_configuration.cpp
index b2412781c9d..4845a868a11 100644
--- a/src/platform/options/default_configuration.cpp
+++ b/src/platform/options/default_configuration.cpp
@@ -14,6 +14,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "mir/options/program_option.h"
 #include "mir/shared_library.h"
 #include "mir/options/default_configuration.h"
 #include "mir/graphics/platform.h"
@@ -68,6 +69,62 @@ namespace
 bool const enable_input_default        = true;
 }
 
+template<typename Value>
+auto mo::DefaultConfiguration::LibraryMap<Value>::find(SharedLibrary const& lib) const -> decltype(values)::const_iterator
+{
+    return std::find_if(
+        values.begin(),
+        values.end(),
+        [&lib](auto const& candidate) { return candidate.first == lib.get_handle(); });
+}
+
+template<typename Value>
+auto mo::DefaultConfiguration::LibraryMap<Value>::find(SharedLibrary const& lib) -> decltype(values)::iterator
+{
+    return std::find_if(
+        values.begin(),
+        values.end(),
+        [&lib](auto const& candidate) { return candidate.first == lib.get_handle(); });
+}
+
+template<typename Value>
+auto mo::DefaultConfiguration::LibraryMap<Value>::operator[](SharedLibrary const& lib) -> Value&
+{
+    auto existing = find(lib);
+    if (existing != values.end())
+    {
+        return existing->second;
+    }
+
+    values.emplace_back(std::make_pair(lib.get_handle(), Value{}));
+    return values.back().second;
+}
+
+template<typename Value>
+auto mo::DefaultConfiguration::LibraryMap<Value>::at(SharedLibrary const& lib) const -> Value const&
+{
+    auto existing = find(lib);
+
+    if (existing == values.end())
+    {
+        BOOST_THROW_EXCEPTION((std::out_of_range{"SharedLibrary not in map"}));
+    }
+
+    return existing->second;
+}
+
+template<typename Value>
+auto mo::DefaultConfiguration::LibraryMap<Value>::begin() const -> decltype(values)::const_iterator
+{
+    return values.cbegin();
+}
+
+template<typename Value>
+auto mo::DefaultConfiguration::LibraryMap<Value>::end() const -> decltype(values)::const_iterator
+{
+    return values.cend();
+}
+
 mo::DefaultConfiguration::DefaultConfiguration(int argc, char const* argv[]) :
     DefaultConfiguration(argc, argv, std::string{})
 {
@@ -200,6 +257,17 @@ mo::DefaultConfiguration::DefaultConfiguration(
         add_platform_options();
 }
 
+namespace
+{
+auto option_header_for(mir::SharedLibrary const& module) -> std::string
+{
+    auto const describe_module = module.load_function<mir::graphics::DescribeModule>("describe_graphics_module", MIR_SERVER_GRAPHICS_PLATFORM_VERSION);
+    auto const description = describe_module();
+
+    return std::format("Options for module {}", description->name);
+}
+}
+
 void mo::DefaultConfiguration::add_platform_options()
 {
     namespace po = boost::program_options;
@@ -226,15 +294,19 @@ void mo::DefaultConfiguration::add_platform_options()
 
         for (auto& platform : platform_libraries)
         {
-            /* Ideally we'd namespace these options with the platform,
-             * and display them in a group as $FOO-platform-specific.
-             */
             try
             {
                 auto add_platform_options = platform->load_function<mir::graphics::AddPlatformOptions>("add_graphics_platform_options", MIR_SERVER_GRAPHICS_PLATFORM_VERSION);
-                add_platform_options(*this->program_options);
+                auto [options, inserted] = module_options_desc.emplace(
+                    *platform,
+                    std::forward_as_tuple(option_header_for(*platform)));
+
+                // *Pretty* sure this is impossible, but let's be sure!
+                assert(inserted && "Attempted to add platform options twice for the same module");
+
+                add_platform_options(options->second);
             }
-            catch (std::runtime_error&)
+            catch (std::runtime_error& e)
             {
                 /* We've failed to add the options - probably because it's not a graphics platform,
                  * or because it's got the wrong version - unload it; it's unnecessary.
@@ -269,7 +341,7 @@ boost::program_options::options_description_easy_init mo::DefaultConfiguration::
     return program_options->add_options();
 }
 
-std::shared_ptr<mo::Option> mo::DefaultConfiguration::the_options() const
+std::shared_ptr<mo::Option> mo::DefaultConfiguration::global_options() const
 {
     if (!options)
     {
@@ -282,6 +354,31 @@ std::shared_ptr<mo::Option> mo::DefaultConfiguration::the_options() const
     return options;
 }
 
+auto mo::DefaultConfiguration::the_options_for(SharedLibrary const& module) const -> std::shared_ptr<Option>
+{
+    auto parsed_options = module_options[module];
+    if (!parsed_options)
+    {
+        auto options = std::make_shared<ProgramOption>();
+        auto& module_option_desc = module_options_desc.at(module);
+
+        /* We can't combine the parsed global options with a parse of the module-specific options,
+         * but we *can* combine the global options description with the module options description
+         * and (re)parse everything against that.
+         */
+        boost::program_options::options_description joint_desc;
+        joint_desc.add(*program_options).add(module_option_desc);
+
+        parse_arguments(joint_desc, *options, argc, argv);
+        parse_environment(joint_desc, *options);
+        parse_config_file(joint_desc, *options);
+
+        module_options[module] = options;
+        parsed_options = options;
+    }
+
+    return parsed_options;
+}
 
 void mo::DefaultConfiguration::parse_arguments(
     boost::program_options::options_description desc,
@@ -300,10 +397,25 @@ void mo::DefaultConfiguration::parse_arguments(
 
         options.parse_arguments(desc, argc, argv);
 
-        auto const unparsed_arguments = options.unparsed_command_line();
+        auto unparsed_arguments = options.unparsed_command_line();
         std::vector<char const*> tokens;
         for (auto const& token : unparsed_arguments)
             tokens.push_back(token.c_str());
+
+        // Maybe the unparsed tokens are for a module?
+        for (auto const& [_, module_option_desc] : module_options_desc)
+        {
+            mo::ProgramOption tmp_options;
+            tmp_options.parse_arguments(module_option_desc, tokens.size(), tokens.data());
+
+            tokens.clear();
+            unparsed_arguments = tmp_options.unparsed_command_line();
+            for (auto const& token : unparsed_arguments)
+            {
+                tokens.push_back(token.c_str());
+            }
+        }
+
         if (!tokens.empty()) unparsed_arguments_handler(tokens.size(), tokens.data());
 
         // See the ExitWithOutput documentation for information about its usage.
@@ -311,6 +423,14 @@ void mo::DefaultConfiguration::parse_arguments(
         {
             std::ostringstream help_text;
             help_text << desc;
+            for (auto& [_, module_desc] : module_options_desc)
+            {
+                // We don't want to print out headers for platforms that don't have any options
+                if (!module_desc.options().empty())
+                {
+                    help_text << module_desc;
+                }
+            }
             BOOST_THROW_EXCEPTION(mir::ExitWithOutput(help_text.str()));
         }
 
@@ -329,11 +449,70 @@ void mo::DefaultConfiguration::parse_arguments(
     }
 }
 
+namespace
+{
+auto is_an_option(boost::program_options::options_description const& options, std::string const& name) -> bool
+{
+    return std::find_if(
+        options.options().cbegin(),
+        options.options().cend(),
+        [&name](auto const& option) -> bool
+        {
+            return option->long_name() == name;
+        }) != options.options().cend();
+}
+}
+
 void mo::DefaultConfiguration::parse_environment(
     boost::program_options::options_description& desc,
     mo::ProgramOption& options) const
 {
-    options.parse_environment(desc, "MIR_SERVER_");
+    options.parse_environment(
+        desc,
+        [=, this](std::string const& from) -> std::string
+        {
+            auto const prefix = "MIR_SERVER_";
+            auto const sizeof_prefix = strlen(prefix);
+
+            if (!from.starts_with(prefix))
+            {
+                return std::string{};
+            }
+
+            std::string result(from, sizeof_prefix);
+
+            for(auto& ch : result)
+            {
+                if (ch == '_') ch = '-';
+                else ch = std::tolower(ch, std::locale::classic()); // avoid current locale
+            }
+
+            /* Now, we want to not fail on any options that are module-specific but
+             * not for *this* module (or for the global parse)
+             *
+             * So, we check:
+             * 1. Is the result an option in `desc` (ie: an option we're trying to parse)? Return it. Or,
+             * 2. Is the option in one of the module-specific option descriptors? Ignore it.
+             * 3. Otherwise, return it as normal, and cause a failure
+             */
+
+            if (is_an_option(desc, result))
+            {
+                return result;
+            }
+
+            // Is it an option for a different module?
+            for (auto const& [_, module_opt_desc] : module_options_desc)
+            {
+                if (is_an_option(module_opt_desc, result))
+                {
+                    // We want to ignore this for now, rather than raise an unknown option error
+                    return std::string{};
+                }
+            }
+
+            return result;
+        });
 }
 
 void mo::DefaultConfiguration::parse_config_file(
diff --git a/src/platform/options/program_option.cpp b/src/platform/options/program_option.cpp
index 1cf88974083..b91f1d91d0e 100644
--- a/src/platform/options/program_option.cpp
+++ b/src/platform/options/program_option.cpp
@@ -58,26 +58,9 @@ std::vector<std::string> mo::ProgramOption::unparsed_command_line() const
 
 void mo::ProgramOption::parse_environment(
     po::options_description const& desc,
-    char const* prefix)
+    std::function<std::string(std::string)> env_var_mapper)
 {
-    auto parsed_options = po::parse_environment(
-        desc,
-        [=](std::string const& from) -> std::string
-        {
-             auto const sizeof_prefix = strlen(prefix);
-
-             if (from.length() < sizeof_prefix || 0 != from.find(prefix)) return std::string();
-
-             std::string result(from, sizeof_prefix);
-
-             for(auto& ch : result)
-             {
-                 if (ch == '_') ch = '-';
-                 else ch = std::tolower(ch, std::locale::classic()); // avoid current locale
-             }
-
-             return result;
-        });
+    auto parsed_options = po::parse_environment(desc, env_var_mapper);
 
     po::store(parsed_options, options);
 }
diff --git a/src/server/default_server_configuration.cpp b/src/server/default_server_configuration.cpp
index 6f6ce12ed03..80c5faff6d7 100644
--- a/src/server/default_server_configuration.cpp
+++ b/src/server/default_server_configuration.cpp
@@ -23,6 +23,7 @@
 #include "mir/frontend/wayland.h"
 
 #include "mir/logging/dumb_console_logger.h"
+#include "mir/options/option.h"
 #include "mir/options/program_option.h"
 #include "mir/frontend/session_credentials.h"
 #include "mir/frontend/session_authorizer.h"
@@ -55,7 +56,7 @@ mir::DefaultServerConfiguration::DefaultServerConfiguration(int argc, char const
 {
 }
 
-mir::DefaultServerConfiguration::DefaultServerConfiguration(std::shared_ptr<mo::Configuration> const& configuration_options) :
+mir::DefaultServerConfiguration::DefaultServerConfiguration(std::shared_ptr<mo::OptionsProvider> const& configuration_options) :
     configuration_options(configuration_options),
     enabled_wayland_extensions(frontend::get_standard_extensions())
 {
@@ -64,7 +65,13 @@ mir::DefaultServerConfiguration::DefaultServerConfiguration(std::shared_ptr<mo::
 auto mir::DefaultServerConfiguration::the_options() const
 ->std::shared_ptr<options::Option>
 {
-    return configuration_options->the_options();
+    return configuration_options->global_options();
+}
+
+auto mir::DefaultServerConfiguration::the_options_provider() const
+    -> std::shared_ptr<mo::OptionsProvider>
+{
+    return configuration_options;
 }
 
 std::shared_ptr<ms::SessionListener>
diff --git a/src/server/graphics/default_configuration.cpp b/src/server/graphics/default_configuration.cpp
index 5db02e1d707..9a1da5a296c 100644
--- a/src/server/graphics/default_configuration.cpp
+++ b/src/server/graphics/default_configuration.cpp
@@ -140,7 +140,7 @@ auto mir::DefaultServerConfiguration::the_display_platforms() -> std::vector<std
 
         try
         {
-            auto const platform_modules = mg::select_display_modules(*the_options(), the_console_services(), *the_shared_library_prober_report());
+            auto const platform_modules = mg::select_display_modules(*the_options_provider(), the_console_services(), *the_shared_library_prober_report());
 
             for (auto const& [device, platform]: platform_modules)
             {
@@ -178,7 +178,7 @@ auto mir::DefaultServerConfiguration::the_display_platforms() -> std::vector<std
                 display_platforms.push_back(
                     create_display_platform(
                         device,
-                        the_options(),
+                        the_options_provider()->the_options_for(*platform),
                         the_emergency_cleanup(),
                         the_console_services(),
                         the_display_report()));
@@ -261,7 +261,7 @@ auto mir::DefaultServerConfiguration::the_rendering_platforms() ->
             }
             else
             {
-                platform_modules = mir::graphics::rendering_modules_for_device(platforms, display_targets, *the_options(), the_console_services());
+                platform_modules = mir::graphics::rendering_modules_for_device(platforms, display_targets, *the_options_provider(), the_console_services());
             }
 
             for (auto const& [device, platform]: platform_modules)
@@ -296,13 +296,15 @@ auto mir::DefaultServerConfiguration::the_rendering_platforms() ->
                         device.device->devnode());
                 }
 
+                mir::log_info("Hello? Do we see this for %s", description->name);
+
                 // TODO: Do we want to be able to continue on partial failure here?
                 rendering_platform_map.emplace(
                     description->name,
                     create_rendering_platform(
                         device,
                         display_targets,
-                        *the_options(),
+                        *the_options_provider()->the_options_for(*platform),
                         *the_emergency_cleanup()));
                 // Add this module to the list searched by the input stack later
                 // TODO: Come up with a more principled solution for combined input/rendering/output platforms
diff --git a/src/server/graphics/platform_probe.cpp b/src/server/graphics/platform_probe.cpp
index 430972670d4..db707a7cf52 100644
--- a/src/server/graphics/platform_probe.cpp
+++ b/src/server/graphics/platform_probe.cpp
@@ -14,6 +14,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "mir/graphics/default_display_configuration_policy.h"
 #include "mir/graphics/display.h"
 #include "mir/log.h"
 #include "mir/graphics/platform.h"
@@ -80,11 +81,24 @@ auto probe_module(
     }
     return supported_devices;
 }
+
+auto is_graphics_module(mir::SharedLibrary const& module) -> bool
+{
+    try
+    {
+        module.load_function<mg::DescribeModule>("describe_graphics_module", MIR_SERVER_GRAPHICS_PLATFORM_VERSION);
+        return true;
+    }
+    catch (std::exception const&)
+    {
+        return false;
+    }
+}
 }
 
 auto mir::graphics::probe_display_module(
     SharedLibrary const& module,
-    options::Option const& options,
+    mo::Option const& options,
     std::shared_ptr<ConsoleServices> const& console) -> std::vector<SupportedDevice>
 {
     return probe_module(
@@ -102,7 +116,7 @@ auto mir::graphics::probe_display_module(
 auto mir::graphics::probe_rendering_module(
     std::span<std::shared_ptr<mg::DisplayPlatform>> const& platforms,
     SharedLibrary const& module,
-    options::Option const& options,
+    mo::Option const& options,
     std::shared_ptr<ConsoleServices> const& console) -> std::vector<SupportedDevice>
 {
     return probe_module(
@@ -269,13 +283,13 @@ auto mg::modules_for_device(
 
 auto mir::graphics::display_modules_for_device(
     std::vector<std::shared_ptr<SharedLibrary>> const& modules,
-    options::Option const& options,
+    mo::OptionsProvider const& options,
     std::shared_ptr<ConsoleServices> const& console) -> std::vector<std::pair<SupportedDevice, std::shared_ptr<SharedLibrary>>>
 {
     return modules_for_device(
         [&options, &console](mir::SharedLibrary const& module) -> std::vector<mg::SupportedDevice>
         {
-            return mg::probe_display_module(module, options, console);
+            return mg::probe_display_module(module, *options.the_options_for(module), console);
         },
         modules,
         TypePreference::prefer_nested);
@@ -284,13 +298,20 @@ auto mir::graphics::display_modules_for_device(
 auto mir::graphics::rendering_modules_for_device(
     std::vector<std::shared_ptr<SharedLibrary>> const& modules,
     std::span<std::shared_ptr<DisplayPlatform>> const& platforms,
-    options::Option const& options,
+    mo::OptionsProvider const& options,
     std::shared_ptr<ConsoleServices> const& console) -> std::vector<std::pair<SupportedDevice, std::shared_ptr<SharedLibrary>>>
 {
     return modules_for_device(
         [&platforms, &options, &console](SharedLibrary const& module) -> std::vector<SupportedDevice>
         {
-            return probe_rendering_module(platforms, module, options, console);
+            if (is_graphics_module(module))
+            {
+                return probe_rendering_module(platforms, module, *options.the_options_for(module), console);
+            }
+            else
+            {
+                return {};
+            }
         },
         modules,
         TypePreference::prefer_hardware);
@@ -372,14 +393,15 @@ auto dso_filename_alphabetically_before(mir::SharedLibrary const& a, mir::Shared
 }
 
 auto mg::select_display_modules(
-    options::Option const& options,
+    options::OptionsProvider const& options,
     std::shared_ptr<ConsoleServices> const& console,
     SharedLibraryProberReport& lib_loader_report)
     -> std::vector<std::pair<SupportedDevice, std::shared_ptr<SharedLibrary>>>
 {
     std::vector<std::pair<SupportedDevice, std::shared_ptr<SharedLibrary>>> platform_modules;
 
-    auto const& path = options.get<std::string>(options::platform_path);
+    auto const global_options = options.global_options();
+    auto const& path = global_options->get<std::string>(options::platform_path);
     auto platforms = mir::libraries_for_path(path, lib_loader_report);
 
     if (platforms.empty())
@@ -398,17 +420,7 @@ auto mg::select_display_modules(
             platforms.end(),
             [](auto const& module)
             {
-                try
-                {
-                    module->template load_function<mir::graphics::DescribeModule>(
-                        "describe_graphics_module",
-                        MIR_SERVER_GRAPHICS_PLATFORM_VERSION);
-                    return false;
-                }
-                catch (std::runtime_error const&)
-                {
-                    return true;
-                }
+                return !is_graphics_module(*module);
             }),
         platforms.end());
 
@@ -457,17 +469,17 @@ auto mg::select_display_modules(
         });
     auto virtual_platform = virtual_platform_pos != platforms.end() ? *virtual_platform_pos : std::shared_ptr<SharedLibrary>{};
 
-    if (options.is_set(options::platform_display_libs))
+    if (global_options->is_set(options::platform_display_libs))
     {
         auto const manually_selected_platforms =
-            select_platforms_from_list(options.get<std::string>(options::platform_display_libs), platforms);
+            select_platforms_from_list(global_options->get<std::string>(options::platform_display_libs), platforms);
 
         for (auto const& platform : manually_selected_platforms)
         {
             auto supported_devices =
                 graphics::probe_display_module(
                     *platform,
-                    options,
+                    *options.the_options_for(*platform),
                     console);
 
             bool found_supported_device{false};
@@ -519,7 +531,7 @@ auto mg::select_display_modules(
     if (virtual_platform)
     {
         auto virtual_probe = probe_display_module(
-            *virtual_platform, options, console);
+            *virtual_platform, *options.the_options_for(*virtual_platform), console);
         if (virtual_probe.size() && virtual_probe.front().support_level >= mg::probe::supported)
         {
             platform_modules.emplace_back(std::move(virtual_probe.front()), std::move(virtual_platform));
diff --git a/src/server/graphics/platform_probe.h b/src/server/graphics/platform_probe.h
index 55276048449..ffb0cd91036 100644
--- a/src/server/graphics/platform_probe.h
+++ b/src/server/graphics/platform_probe.h
@@ -20,7 +20,6 @@
 #include <vector>
 #include <memory>
 #include "mir/shared_library.h"
-#include "mir/options/program_option.h"
 #include "mir/graphics/platform.h"
 #include "mir/shared_library_prober_report.h"
 
@@ -28,6 +27,11 @@ namespace mir
 {
 class ConsoleServices;
 
+namespace options
+{
+class OptionsProvider;
+}
+
 namespace graphics
 {
 class RenderingPlatform;
@@ -57,19 +61,19 @@ auto probe_rendering_module(
 
 auto display_modules_for_device(
     std::vector<std::shared_ptr<SharedLibrary>> const& modules,
-    options::Option const& options,
+    options::OptionsProvider const& options,
     std::shared_ptr<ConsoleServices> const& console)
     -> std::vector<std::pair<SupportedDevice, std::shared_ptr<SharedLibrary>>>;
 
 auto rendering_modules_for_device(
     std::vector<std::shared_ptr<SharedLibrary>> const& modules,
     std::span<std::shared_ptr<DisplayPlatform>> const& platforms,
-    options::Option const& options,
+    options::OptionsProvider const& options,
     std::shared_ptr<ConsoleServices> const& console)
     -> std::vector<std::pair<SupportedDevice, std::shared_ptr<SharedLibrary>>>;
 
 auto select_display_modules(
-    options::Option const& options,
+    options::OptionsProvider const& options,
     std::shared_ptr<ConsoleServices> const& console,
     SharedLibraryProberReport& lib_loader_report)
     -> std::vector<std::pair<SupportedDevice, std::shared_ptr<SharedLibrary>>>;
diff --git a/src/server/server.cpp b/src/server/server.cpp
index 3126ca86b9b..8d31a61bb5f 100644
--- a/src/server/server.cpp
+++ b/src/server/server.cpp
@@ -208,7 +208,7 @@ auto wrap_##name(decltype(Self::name##_wrapper)::result_type const& wrapped)\
 struct mir::Server::ServerConfiguration : mir::DefaultServerConfiguration
 {
     ServerConfiguration(
-        std::shared_ptr<options::Configuration> const& configuration_options,
+        std::shared_ptr<options::OptionsProvider> const& configuration_options,
         std::shared_ptr<Self> const& self) :
         DefaultServerConfiguration(configuration_options),
         self(self.get())
diff --git a/tests/acceptance-tests/server_configuration_options.cpp b/tests/acceptance-tests/server_configuration_options.cpp
index dfa01a8f713..4d52789939d 100644
--- a/tests/acceptance-tests/server_configuration_options.cpp
+++ b/tests/acceptance-tests/server_configuration_options.cpp
@@ -18,6 +18,7 @@
 
 #include "mir/options/option.h"
 
+#include <boost/program_options/errors.hpp>
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
 
@@ -132,6 +133,39 @@ TEST_F(ServerConfigurationOptions, unknown_command_line_options_are_passed_to_ha
     server.apply_settings();
 }
 
+TEST_F(ServerConfigurationOptions, are_parsed_from_environment)
+{
+    auto const option_name = "test-option";
+    auto const option_env_key = "MIR_SERVER_TEST_OPTION";
+    auto const option_env_value = "a string";
+
+    add_to_environment(option_env_key, option_env_value);
+
+    server.add_configuration_option(option_name, "", mir::OptionType::string);
+
+    server.apply_settings();
+
+    auto const options = server.get_options();
+    EXPECT_THAT(options->get<std::string>(option_name), StrEq(option_env_value));
+}
+
+TEST_F(ServerConfigurationOptions, unknown_environment_options_are_fatal)
+{
+    auto const option_env_key = "MIR_SERVER_NOT_ACTUALLY_AN_OPTION_DEFINED";
+    auto const option_env_value = "a string";
+
+    add_to_environment(option_env_key, option_env_value);
+
+    EXPECT_THROW(
+        {
+            server.apply_settings();
+
+            server.get_options();
+        },
+        boost::program_options::unknown_option
+    );
+}
+
 TEST_F(ServerConfigurationOptions, are_read_from_xdg_config_home)
 {
     create_config_file_in(fake_xdg_config_home);
diff --git a/tests/mir_test_framework/command_line_server_configuration.cpp b/tests/mir_test_framework/command_line_server_configuration.cpp
index 6650ebf5114..1c1e79d6010 100644
--- a/tests/mir_test_framework/command_line_server_configuration.cpp
+++ b/tests/mir_test_framework/command_line_server_configuration.cpp
@@ -23,7 +23,6 @@
 
 #include <gtest/gtest.h>
 
-#include <algorithm>
 #include <cstring>
 
 namespace mo=mir::options;
diff --git a/tests/unit-tests/graphics/test_platform_prober.cpp b/tests/unit-tests/graphics/test_platform_prober.cpp
index af833f2801f..e607abfa613 100644
--- a/tests/unit-tests/graphics/test_platform_prober.cpp
+++ b/tests/unit-tests/graphics/test_platform_prober.cpp
@@ -27,6 +27,7 @@
 #include "mir/graphics/platform.h"
 #include "mir/options/configuration.h"
 #include "mir/options/default_configuration.h"
+#include "mir/options/option.h"
 #include "mir/shared_library.h"
 #include "mir/shared_library_prober_report.h"
 #include "mir/test/doubles/fake_display.h"
@@ -181,12 +182,29 @@ class ServerPlatformProbeMockDRM : public ::testing::Test
 #endif
 };
 
+class StubOptionsProvider : public mo::OptionsProvider
+{
+public:
+    StubOptionsProvider() = default;
+    ~StubOptionsProvider() override = default;
+
+    auto global_options() const -> std::shared_ptr<mo::Option> override
+    {
+        return std::make_shared<mo::ProgramOption>();
+    }
+
+    auto the_options_for(mir::SharedLibrary const& /*module*/) const -> std::shared_ptr<mo::Option> override
+    {
+        return std::make_shared<mo::ProgramOption>();
+    }
+};
+
 }
 
 TEST(ServerPlatformProbe, ConstructingWithNoModulesIsAnError)
 {
     std::vector<std::shared_ptr<mir::SharedLibrary>> empty_modules;
-    mir::options::ProgramOption options;
+    StubOptionsProvider options;
 
     EXPECT_THROW(mir::graphics::display_modules_for_device(empty_modules, options, nullptr),
                  std::runtime_error);
@@ -196,7 +214,7 @@ TEST(ServerPlatformProbe, ConstructingWithNoModulesIsAnError)
 TEST_F(ServerPlatformProbeMockDRM, LoadsMesaPlatformWhenDrmMasterCanBeAcquired)
 {
     using namespace testing;
-    mir::options::ProgramOption options;
+    StubOptionsProvider options;
     auto fake_mesa = ensure_mesa_probing_succeeds();
 
     auto modules = available_platforms();
@@ -220,7 +238,7 @@ TEST_F(ServerPlatformProbeMockDRM, LoadsMesaPlatformWhenDrmMasterCanBeAcquired)
 TEST_F(ServerPlatformProbeMockDRM, DoesNotLoadDummyPlatformWhenBetterPlatformExists)
 {
     using namespace testing;
-    mir::options::ProgramOption options;
+    StubOptionsProvider options;
     auto fake_mesa = ensure_mesa_probing_succeeds();
 
     auto modules = available_platforms();
@@ -243,7 +261,7 @@ TEST_F(ServerPlatformProbeMockDRM, DoesNotLoadDummyPlatformWhenBetterPlatformExi
 TEST(ServerPlatformProbe, ThrowsExceptionWhenNothingProbesSuccessfully)
 {
     using namespace testing;
-    mir::options::ProgramOption options;
+    StubOptionsProvider options;
     auto block_mesa = ensure_mesa_probing_fails();
 
     auto modules = available_platforms();
@@ -260,7 +278,7 @@ TEST(ServerPlatformProbe, ThrowsExceptionWhenNothingProbesSuccessfully)
 TEST(ServerPlatformProbe, LoadsSupportedModuleWhenNoBestModule)
 {
     using namespace testing;
-    mir::options::ProgramOption options;
+    StubOptionsProvider options;
     auto block_mesa = ensure_mesa_probing_fails();
 
     auto modules = available_platforms();
@@ -285,7 +303,7 @@ TEST(ServerPlatformProbe, LoadsSupportedModuleWhenNoBestModule)
 TEST(ServerPlatformProbe, IgnoresNonPlatformModules)
 {
     using namespace testing;
-    mir::options::ProgramOption options;
+    StubOptionsProvider options;
 
     std::vector<std::shared_ptr<mir::SharedLibrary>> modules;
     add_dummy_platform(modules);
@@ -632,7 +650,16 @@ class FullProbeStack : public testing::Test
             ((mtd::logging_opt), boost::program_options::value<std::string>()->default_value(""), mtd::logging_descr);
 
         options = std::move(opts);
-        return *(options->the_options());
+        return *(options->global_options());
+    }
+
+    auto the_options_provider() -> mo::OptionsProvider const&
+    {
+        if (!options)
+        {
+            the_options();
+        }
+        return *options;
     }
 
     auto the_console_services() -> std::shared_ptr<mir::ConsoleServices>
@@ -656,7 +683,7 @@ class FullProbeStack : public testing::Test
      * would call mo::Configuration::~Configiration and fail, whereas std::make_shared<mo::DefaultConfiguration> will capture
      * mo::DefaultConfiguration::~DefaultConfiguration and compile.
      */
-    std::shared_ptr<mo::Configuration> options;
+    std::shared_ptr<mo::OptionsProvider> options;
     std::shared_ptr<mir::SharedLibraryProberReport> const report;
 
     std::vector<std::unique_ptr<mtf::TemporaryEnvironmentValue>> temporary_env;
@@ -689,7 +716,7 @@ TEST_F(FullProbeStack, select_display_modules_loads_all_available_hardware_when_
         }
     }
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
     EXPECT_THAT(devices.size(), Eq(expected_hardware_count));
 }
 
@@ -747,7 +774,7 @@ TEST_F(FullProbeStack, select_display_modules_loads_virtual_platform_as_well_as_
 
     add_virtual_option();
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     // We expect the Virtual platform to load
     EXPECT_THAT(devices, Contains(Pair(_, ModuleNameMatches(StrEq("mir:virtual")))));
@@ -773,7 +800,7 @@ TEST_F(FullProbeStack, select_display_loads_gbm_platform_for_each_kms_device)
         GTEST_SKIP() << "gbm-kms platform not built";
     }
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     for(auto const& device: udev_devices)
     {
@@ -796,7 +823,7 @@ TEST_F(FullProbeStack, select_display_modules_loads_x11_platform_even_when_gbm_i
         GTEST_SKIP() << "gbm-kms platform not built; cannot test hardware platform probing";
     }
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     // We expect the X11 platform to load
     EXPECT_THAT(devices, Contains(Pair(_, ModuleNameMatches(StrEq("mir:x11")))));
@@ -818,7 +845,7 @@ TEST_F(FullProbeStack, when_all_of_gbm_and_x11_and_virtual_are_available_select_
         GTEST_SKIP() << "gbm-kms platform not built; cannot test hardware platform probing";
     }
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     // We expect the X11 platform to load
     EXPECT_THAT(devices, Contains(Pair(_, ModuleNameMatches(StrEq("mir:x11")))));
@@ -841,7 +868,7 @@ TEST_F(FullProbeStack, select_display_modules_loads_wayland_platform_even_when_g
         GTEST_SKIP() << "gbm-kms platform not built; cannot test hardware platform probing";
     }
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     // We expect the Wayland platform to load
     EXPECT_THAT(devices, Contains(Pair(_, ModuleNameMatches(StrEq("mir:wayland")))));
@@ -863,7 +890,7 @@ TEST_F(FullProbeStack, when_all_of_gbm_and_wayland_and_virtual_are_available_sel
         GTEST_SKIP() << "gbm-kms platform not built; cannot test hardware platform probing";
     }
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     // We expect the Wayland platform to load
     EXPECT_THAT(devices, Contains(Pair(_, ModuleNameMatches(StrEq("mir:wayland")))));
@@ -878,7 +905,7 @@ TEST_F(FullProbeStack, when_both_wayland_and_x11_are_supported_x11_is_chosen)
     enable_host_wayland();
     enable_host_x11();
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     EXPECT_THAT(devices, Not(Contains(Pair(_, ModuleNameMatches(StrEq("mir:wayland"))))));
     EXPECT_THAT(devices, Contains(Pair(_, ModuleNameMatches(StrEq("mir:x11")))));
@@ -907,7 +934,7 @@ TEST_F(FullProbeStack, instantiates_all_manually_selected_platforms)
         set_display_libs_option("mir:x11,mir:wayland");
     }
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     if (expect_kms_device)
     {
@@ -952,7 +979,7 @@ TEST_F(FullProbeStack, instantiates_all_manually_selected_platforms_and_virtual_
         set_display_libs_option("mir:x11,mir:wayland");
     }
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     if (expect_kms_device)
     {
@@ -996,7 +1023,7 @@ TEST_F(FullProbeStack, selects_only_gbm_when_manually_specified_even_if_x11_and_
 
     set_display_libs_option("mir:gbm-kms");
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     EXPECT_THAT(
         devices,
@@ -1015,7 +1042,7 @@ TEST_F(FullProbeStack, doesnt_instantiate_other_platforms_when_manually_selected
 
     set_display_libs_option("mir:gbm-kms");
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     EXPECT_THAT(devices, IsEmpty());
 }
@@ -1029,7 +1056,7 @@ TEST_F(FullProbeStack, manually_selecting_only_virtual_platform_with_no_virtual_
 
     set_display_libs_option("mir:virtual");
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     EXPECT_THAT(devices, IsEmpty());
 }
@@ -1044,7 +1071,7 @@ TEST_F(FullProbeStack, manually_selecting_only_virtual_platform_with_required_op
 
     set_display_libs_option("mir:virtual");
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     ASSERT_THAT(devices.size(), Eq(1));
     EXPECT_THAT(devices.front(), Pair(_, ModuleNameMatches(StrEq("mir:virtual"))));
@@ -1067,7 +1094,7 @@ TEST_F(FullProbeStack, gbm_kms_is_not_selected_for_nvidia_driver_by_default)
     enable_gbm_on_kms_device(*nouveau_device);
     enable_gbm_on_kms_device(*amd_device);
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     EXPECT_THAT(devices, UnorderedElementsAre(
         Pair(IsPlatformForDevice(nouveau_device.get()), ModuleNameMatches(StrEq("mir:gbm-kms"))),
@@ -1094,7 +1121,7 @@ TEST_F(FullProbeStack, gbm_kms_is_selected_for_nvidia_driver_when_quirk_is_allow
 
     mtf::TemporaryEnvironmentValue skip_nvidia_quirk{"MIR_SERVER_DRIVER_QUIRKS", "allow:driver:nvidia"};
 
-    auto devices = mg::select_display_modules(the_options(), the_console_services(), *the_library_prober_report());
+    auto devices = mg::select_display_modules(the_options_provider(), the_console_services(), *the_library_prober_report());
 
     EXPECT_THAT(devices, UnorderedElementsAre(
         Pair(IsPlatformForDevice(nvidia_device.get()), ModuleNameMatches(StrEq("mir:gbm-kms"))),
diff --git a/tests/unit-tests/options/test_program_option.cpp b/tests/unit-tests/options/test_program_option.cpp
index c2c14b2ad0b..a0ddfa66c8d 100644
--- a/tests/unit-tests/options/test_program_option.cpp
+++ b/tests/unit-tests/options/test_program_option.cpp
@@ -209,28 +209,6 @@ TEST_F(ProgramOption, unparsed_command_line_returns_unprocessed_tokens)
     EXPECT_THAT(po.unparsed_command_line(), ElementsAre("--hello", "world", "--answer", "42"));
 }
 
-TEST(ProgramOptionEnv, parse_environment)
-{
-    // Env variables should be uppercase and "_" delimited
-    char const* name = "some-key";
-    char const* key = "SOME_KEY";
-    char const* value = "test_value";
-    auto const env = std::string(__PRETTY_FUNCTION__) + key;
-    setenv(env.c_str(), value, true);
-
-    bpo::options_description desc;
-    desc.add_options()
-        (name, bpo::value<std::string>());
-
-    mir::options::ProgramOption po;
-    po.parse_environment(desc, __PRETTY_FUNCTION__);
-
-    EXPECT_EQ(value, po.get(name, "default"));
-    EXPECT_EQ("default", po.get("garbage", "default"));
-    EXPECT_TRUE(po.is_set(name));
-    EXPECT_FALSE(po.is_set("garbage"));
-}
-
 // TODO need to parse something
 TEST(ProgramOptionFile, parse_files)
 {