Enforce option requirements --- but the config stuff is still kinda broken and

will need rethinking, especially if flux files can carry configs with them.

lib/config.cc

@@ -29,7 +29,7 @@ Config::operator ConfigProto&() const
 
 void Config::clear()
 {
-    this->clear();
+    (*this)->Clear();
 }
 
 void Config::set(std::string key, std::string value)
@@ -69,30 +69,17 @@ void Config::readConfigFile(std::string filename)
     globalConfig()->MergeFrom(loadSingleConfigFile(filename));
 }
 
-void Config::applyOption(const OptionProto& option)
+const OptionProto& Config::findOption(const std::string& optionName)
 {
-    if (option.config().option_size() > 0)
-        error("option '{}' has an option inside it, which isn't allowed",
-            option.name());
-    if (option.config().option_group_size() > 0)
-        error("option '{}' has an option group inside it, which isn't allowed",
-            option.name());
+    const OptionProto* found = nullptr;
 
-    log("OPTION: {}",
-        option.has_message() ? option.message() : option.comment());
-
-    (*this)->MergeFrom(option.config());
-}
-
-bool Config::applyOption(const std::string& optionName)
-{
     auto searchOptionList = [&](auto& optionList)
     {
         for (const auto& option : optionList)
         {
             if (optionName == option.name())
             {
-                applyOption(option);
+                found = &option;
                 return true;
             }
         }
@@ -100,13 +87,57 @@ bool Config::applyOption(const std::string& optionName)
     };
 
     if (searchOptionList((*this)->option()))
-        return true;
+        return *found;
 
     for (const auto& optionGroup : (*this)->option_group())
     {
         if (searchOptionList(optionGroup.option()))
-            return true;
+            return *found;
     }
 
-    return false;
+    throw OptionNotFoundException("option name not found");
+}
+
+bool Config::isOptionValid(const OptionProto& option)
+{
+    for (const auto& req : option.requires())
+    {
+        bool matched = false;
+        try
+        {
+            auto value = get(req.key());
+            for (auto requiredValue : req.value())
+                matched |= (requiredValue == value);
+        }
+        catch (const ProtoPathNotFoundException e)
+        {
+            /* This field isn't available, therefore it cannot match. */
+        }
+
+        if (!matched)
+            return false;
+    }
+
+    return true;
+}
+
+void Config::applyOption(const OptionProto& option)
+{
+    if (option.config().option_size() > 0)
+        throw InvalidOptionException(fmt::format(
+            "option '{}' has an option inside it, which isn't allowed",
+            option.name()));
+    if (option.config().option_group_size() > 0)
+        throw InvalidOptionException(fmt::format(
+            "option '{}' has an option group inside it, which isn't allowed",
+            option.name()));
+    if (!isOptionValid(option))
+        throw InapplicableOptionException(
+            fmt::format("option '{}' is inapplicable to this configuration",
+                option.name()));
+
+    log("OPTION: {}",
+        option.has_message() ? option.message() : option.comment());
+
+    (*this)->MergeFrom(option.config());
 }

lib/config.h

@@ -5,6 +5,30 @@
 class ConfigProto;
 class OptionProto;
 
+class OptionException : public ErrorException
+{
+public:
+    OptionException(const std::string& message): ErrorException(message) {}
+};
+
+class OptionNotFoundException : public OptionException
+{
+public:
+    OptionNotFoundException(const std::string& message): OptionException(message) {}
+};
+
+class InvalidOptionException : public OptionException
+{
+public:
+    InvalidOptionException(const std::string& message): OptionException(message) {}
+};
+
+class InapplicableOptionException : public OptionException
+{
+public:
+    InapplicableOptionException(const std::string& message): OptionException(message) {}
+};
+
 class Config
 {
 public:
@@ -25,10 +49,12 @@ public:
 
 	void readConfigFile(std::string filename);
 
-    /* Modify the current config to engage the named option. */
+    /* Option management: look up an option by name, determine whether an option
+     * is valid, and apply an option. */
 
+	const OptionProto& findOption(const std::string& option);
+	bool isOptionValid(const OptionProto& option);
     void applyOption(const OptionProto& option);
-    bool applyOption(const std::string& option);
 };
 
 extern Config& globalConfig();

lib/flags.cc

@@ -169,7 +169,31 @@ std::vector<std::string> FlagGroup::parseFlagsWithFilenames(int argc,
         index++;
     }
 
-    /* Apply any default options in groups. */
+    /* First apply any value overrides (in order). We need to set the up front
+     * because the options may depend on them. */
+
+    auto applyOverrides = [&]()
+    {
+        for (auto [k, v] : overrides)
+            globalConfig().set(k, v);
+    };
+    applyOverrides();
+
+    /* First apply any standalone options. After each one, reapply the overrides
+     * in case the option changed them. */
+
+    for (auto& option : globalConfig()->option())
+    {
+        if (options.find(option.name()) != options.end())
+        {
+            globalConfig().applyOption(option);
+            applyOverrides();
+            options.erase(option.name());
+        }
+    }
+
+    /* Then apply any default options in groups, likewise applying the
+     * overrides. */
 
     for (auto& group : globalConfig()->option_group())
     {
@@ -186,28 +210,13 @@ std::vector<std::string> FlagGroup::parseFlagsWithFilenames(int argc,
         }
 
         globalConfig().applyOption(*defaultOption);
-    }
-
-    /* Next, any standalone options. */
-
-    for (auto& option : globalConfig()->option())
-    {
-        if (options.find(option.name()) != options.end())
-        {
-            globalConfig().applyOption(option);
-            options.erase(option.name());
-        }
+        applyOverrides();
     }
 
     if (!options.empty())
         error("--{} is not a known flag or format option; try --help",
             *options.begin());
 
-    /* Now apply any value overrides (in order). */
-
-    for (auto [k, v] : overrides)
-        globalConfig().set(k, v);
-
     return filenames;
 }
 

src/fluxengine.cc

@@ -1,5 +1,6 @@
 #include "globals.h"
 #include "proto.h"
+#include <fmt/format.h>
 
 typedef int command_cb(int agrc, const char* argv[]);
 
@@ -175,7 +176,7 @@ int main(int argc, const char* argv[])
             }
             catch (const ErrorException& e)
             {
-                e.print();
+                fmt::print(stderr, "Error: {}\n", e.message);
                 exit(1);
             }
         }

src/gui/idlepanel.cc

@@ -184,12 +184,42 @@ public:
         auto formatName = _formatNames[formatChoice->GetSelection()];
         globalConfig().readConfigFile(formatName);
 
+        /* Merge in any custom config. */
+
+        for (auto setting : split(_extraConfiguration, '\n'))
+        {
+            setting = trimWhitespace(setting);
+            if (setting.size() == 0)
+                continue;
+            if (setting[0] == '#')
+                continue;
+
+            auto equals = setting.find('=');
+            if (equals != std::string::npos)
+            {
+                auto key = setting.substr(0, equals);
+                auto value = setting.substr(equals + 1);
+                setProtoByString(globalConfig(), key, value);
+            }
+            else
+                globalConfig().readConfigFile(setting);
+        }
+
         /* Apply any format options. */
 
         for (const auto& e : _formatOptions)
         {
             if (e.first == formatName)
-                globalConfig().applyOption(e.second);
+            {
+                try
+                {
+                    globalConfig().applyOption(
+                        globalConfig().findOption(e.second));
+                }
+                catch (const OptionNotFoundException e)
+                {
+                }
+            }
         }
 
         /* Locate the device, if any. */
@@ -242,27 +272,6 @@ public:
                 break;
             }
         }
-
-        /* Merge in any custom config. */
-
-        for (auto setting : split(_extraConfiguration, '\n'))
-        {
-            setting = trimWhitespace(setting);
-            if (setting.size() == 0)
-                continue;
-            if (setting[0] == '#')
-                continue;
-
-            auto equals = setting.find('=');
-            if (equals != std::string::npos)
-            {
-                auto key = setting.substr(0, equals);
-                auto value = setting.substr(equals + 1);
-                setProtoByString(globalConfig(), key, value);
-            }
-            else
-                globalConfig().readConfigFile(setting);
-        }
     }
 
     const wxBitmap GetBitmap() override

tests/build.mk

@@ -40,6 +40,7 @@ $(call declare-test,greaseweazle)
 $(call declare-test,kryoflux)
 $(call declare-test,layout)
 $(call declare-test,ldbs)
+$(call declare-test,options)
 $(call declare-test,proto)
 $(call declare-test,utils)
 $(call declare-test,vfs)

tests/options.cc

@@ -0,0 +1,79 @@
+#include "globals.h"
+#include "bytes.h"
+#include "tests/testproto.pb.h"
+#include "lib/config.pb.h"
+#include "proto.h"
+#include "snowhouse/snowhouse.h"
+#include <google/protobuf/text_format.h>
+#include <assert.h>
+#include <regex>
+
+using namespace snowhouse;
+
+static std::string cleanup(const std::string& s)
+{
+    auto outs = std::regex_replace(s, std::regex("[ \t\n]+"), " ");
+    outs = std::regex_replace(outs, std::regex("^[ \t\n]+"), "");
+    outs = std::regex_replace(outs, std::regex("[ \t\n]+$"), "");
+    return outs;
+}
+
+static void load_config(const std::string s)
+{
+    globalConfig().clear();
+    if (!google::protobuf::TextFormat::MergeFromString(
+            cleanup(s), globalConfig()))
+        error("couldn't load test config");
+}
+
+static void test_option_validity()
+{
+    load_config(R"M(
+		drive {
+			tpi: 96
+		}
+
+        option {
+            name: "option1"
+
+            requires: {
+                key: "drive.tpi"
+                value: "96"
+            }
+        }
+
+        option {
+            name: "option2"
+
+            requires: {
+                key: "drive.tpi"
+                value: "95"
+            }
+        }
+
+        option {
+            name: "option3"
+
+            requires: {
+                key: "drive.tpi"
+                value: ["0", "96"]
+            }
+        }
+	)M");
+
+    AssertThat(
+        globalConfig().isOptionValid(globalConfig().findOption("option1")),
+        Equals(true));
+    AssertThat(
+        globalConfig().isOptionValid(globalConfig().findOption("option2")),
+        Equals(false));
+    AssertThat(
+        globalConfig().isOptionValid(globalConfig().findOption("option3")),
+        Equals(true));
+}
+
+int main(int argc, const char* argv[])
+{
+    test_option_validity();
+    return 0;
+}