Migrate the 40track etc extension configs to actual options. Add the

ability to have --group=value options to make this cleaner.

build.py

@@ -28,7 +28,7 @@ else:
         ("acorndfs", "", "--200"),
         ("agat", "", ""),
         ("amiga", "", ""),
-        ("apple2", "", "--140 -c 40track_drive"),
+        ("apple2", "", "--140 --drivetype=40"),
         ("atarist", "", "--360"),
         ("atarist", "", "--370"),
         ("atarist", "", "--400"),
@@ -38,17 +38,17 @@ else:
         ("atarist", "", "--800"),
         ("atarist", "", "--820"),
         ("bk", "", ""),
-        ("brother", "", "--120 -c 40track_drive"),
+        ("brother", "", "--120 --drivetype=40"),
         ("brother", "", "--240"),
         (
             "commodore",
             "scripts/commodore1541_test.textpb",
-            "--171 -c 40track_drive",
+            "--171 --drivetype=40",
         ),
         (
             "commodore",
             "scripts/commodore1541_test.textpb",
-            "--192 -c 40track_drive",
+            "--192 --drivetype=40",
         ),
         ("commodore", "", "--800"),
         ("commodore", "", "--1620"),
@@ -60,17 +60,17 @@ else:
         ("ibm", "", "--1232"),
         ("ibm", "", "--1440"),
         ("ibm", "", "--1680"),
-        ("ibm", "", "--180 -c 40track_drive"),
+        ("ibm", "", "--180 --drivetype=40"),
-        ("ibm", "", "--160 -c 40track_drive"),
+        ("ibm", "", "--160 --drivetype=40"),
-        ("ibm", "", "--320 -c 40track_drive"),
+        ("ibm", "", "--320 --drivetype=40"),
-        ("ibm", "", "--360 -c 40track_drive"),
+        ("ibm", "", "--360 --drivetype=40"),
         ("ibm", "", "--720_96"),
         ("ibm", "", "--720_135"),
         ("mac", "scripts/mac400_test.textpb", "--400"),
         ("mac", "scripts/mac800_test.textpb", "--800"),
         ("n88basic", "", ""),
         ("rx50", "", ""),
-        ("tartu", "", "--390 -c 40track_drive"),
+        ("tartu", "", "--390 --drivetype=40"),
         ("tartu", "", "--780"),
         ("tids990", "", ""),
         ("victor9k", "", "--612"),

doc/disk-40track_drive.md

@@ -10,6 +10,6 @@ no way to detect this automatically.
 For example:
 
 ```
-fluxengine read -c ibm --180 40track_drive
+fluxengine read ibm --180 40track_drive
 ```
 

doc/disk-apple2_drive.md

@@ -11,6 +11,6 @@ connector.
 For example:
 
 ```
-fluxengine read -c apple2 --160 apple2_drive
+fluxengine read apple2 --160 apple2_drive
 ```
 

doc/disk-shugart_drive.md

@@ -10,6 +10,6 @@ on Greaseweazle hardware.
 For example:
 
 ```
-fluxengine read -c ibm --720 shugart_drive
+fluxengine read ibm --720 shugart_drive
 ```
 

lib/config/config.cc

@@ -5,6 +5,7 @@
 #include "lib/core/utils.h"
 #include <fstream>
 #include <google/protobuf/text_format.h>
+#include <fmt/ranges.h>
 
 static Config config;
 
@@ -181,35 +182,8 @@ ConfigProto* Config::combined()
     {
         _combinedConfig = _baseConfig;
 
-        /* First apply any standalone options. */
+        for (const auto& optionInfo : _appliedOptions)
-
+            _combinedConfig.MergeFrom(optionInfo.option->config());
-        std::set<std::string> options = _appliedOptions;
-        for (const auto& option : _baseConfig.option())
-        {
-            if (options.find(option.name()) != options.end())
-            {
-                _combinedConfig.MergeFrom(option.config());
-                options.erase(option.name());
-            }
-        }
-
-        /* Then apply any group options. */
-
-        for (auto& group : _baseConfig.option_group())
-        {
-            const OptionProto* selectedOption = &*group.option().begin();
-
-            for (auto& option : group.option())
-            {
-                if (options.find(option.name()) != options.end())
-                {
-                    selectedOption = &option;
-                    options.erase(option.name());
-                }
-            }
-
-            _combinedConfig.MergeFrom(selectedOption->config());
-        }
 
         /* Add in the user overrides. */
 
@@ -241,51 +215,27 @@ std::vector<std::string> Config::validate()
 {
     std::vector<std::string> results;
 
-    std::set<std::string> optionNames = _appliedOptions;
+    /* Ensure that only one item in each group is set. */
-    std::set<const OptionProto*> appliedOptions;
-    for (const auto& option : _baseConfig.option())
-    {
-        if (optionNames.find(option.name()) != optionNames.end())
-        {
-            appliedOptions.insert(&option);
-            optionNames.erase(option.name());
-        }
-    }
 
-    /* Then apply any group options. */
+    std::map<const OptionGroupProto*, const OptionProto*> optionsByGroup;
-
+    for (auto [group, option, hasArgument] : _appliedOptions)
-    for (auto& group : _baseConfig.option_group())
+        if (group)
         {
-        int count = 0;
+            auto& o = optionsByGroup[group];
-
+            if (o)
-        for (auto& option : group.option())
-        {
-            if (optionNames.find(option.name()) != optionNames.end())
-            {
-                optionNames.erase(option.name());
-                appliedOptions.insert(&option);
-
-                count++;
-                if (count == 2)
                 results.push_back(
-                        fmt::format("multiple mutually exclusive options set "
+                    fmt::format("multiple mutually exclusive values set for "
-                                    "for group '{}'",
+                                "group '{}': valid values are: {}",
-                            group.comment()));
+                        group->comment(),
-            }
+                        fmt::join(std::views::transform(
-        }
+                                      group->option(), &OptionProto::name),
-    }
+                            ", ")));
-
+            o = option;
-    /* Check for unknown options. */
-
-    if (!optionNames.empty())
-    {
-        for (auto& name : optionNames)
-            results.push_back(fmt::format("'{}' is not a known option", name));
         }
 
     /* Check option requirements. */
 
-    for (auto& option : appliedOptions)
+    for (auto [group, option, hasArgument] : _appliedOptions)
     {
         try
         {
@@ -360,11 +310,12 @@ void Config::readBaseConfig(std::string data)
         error("couldn't load external config proto");
 }
 
-const OptionProto& Config::findOption(const std::string& optionName)
+Config::OptionInfo Config::findOption(
+    const std::string& name, const std::string value)
 {
     const OptionProto* found = nullptr;
 
-    auto searchOptionList = [&](auto& optionList)
+    auto searchOptionList = [&](auto& optionList, const std::string& optionName)
     {
         for (const auto& option : optionList)
         {
@@ -377,17 +328,39 @@ const OptionProto& Config::findOption(const std::string& optionName)
         return false;
     };
 
-    if (searchOptionList(base()->option()))
+    /* First look for any group names which match. */
-        return *found;
+
+    if (!value.empty())
+        for (const auto& optionGroup : base()->option_group())
+            if (optionGroup.name() == name)
+            {
+                /* The option must therefore be one of these. */
+
+                if (searchOptionList(optionGroup.option(), value))
+                    return {&optionGroup, found, true};
+
+                throw OptionNotFoundException(fmt::format(
+                    "value {} is not valid for option {}; valid values are: {}",
+                    value,
+                    name,
+                    fmt::join(std::views::transform(
+                                  optionGroup.option(), &OptionProto::name),
+                        ", ")));
+            }
+
+    /* Now search for individual options. */
+
+    if (searchOptionList(base()->option(), name))
+        return {nullptr, found, false};
 
     for (const auto& optionGroup : base()->option_group())
     {
-        if (searchOptionList(optionGroup.option()))
+        if (optionGroup.name().empty())
-            return *found;
+            if (searchOptionList(optionGroup.option(), name))
+                return {nullptr, found, false};
     }
 
-    throw OptionNotFoundException(
+    throw OptionNotFoundException(fmt::format("option {} not found", name));
-        fmt::format("option {} not found", optionName));
 }
 
 void Config::checkOptionValid(const OptionProto& option)
@@ -445,22 +418,20 @@ bool Config::isOptionValid(const OptionProto& option)
     }
 }
 
-bool Config::isOptionValid(std::string option)
+void Config::applyOption(const OptionInfo& optionInfo)
-{
-    return isOptionValid(findOption(option));
-}
-
-void Config::applyOption(const OptionProto& option)
 {
+    auto* option = optionInfo.option;
     log(OptionLogMessage{
-        option.has_message() ? option.message() : option.comment()});
+        option->has_message() ? option->message() : option->comment()});
 
-    _appliedOptions.insert(option.name());
+    _appliedOptions.insert(optionInfo);
 }
 
-void Config::applyOption(std::string option)
+bool Config::applyOption(const std::string& name, const std::string value)
 {
-    applyOption(findOption(option));
+    auto optionInfo = findOption(name, value);
+    applyOption(optionInfo);
+    return optionInfo.usesValue;
 }
 
 void Config::clearOptions()

lib/config/config.h

@@ -66,6 +66,18 @@ struct FluxConstructor
 
 class Config
 {
+private:
+    struct OptionInfo
+    {
+        bool operator==(const OptionInfo& other) const = default;
+        std::strong_ordering operator<=>(
+            const OptionInfo& other) const = default;
+
+        const OptionGroupProto* group;
+        const OptionProto* option;
+        bool usesValue;
+    };
+
 public:
     /* Direct access to the various proto layers. */
 
@@ -124,12 +136,12 @@ public:
     /* 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);
+    OptionInfo findOption(
+        const std::string& name, const std::string value = "");
     void checkOptionValid(const OptionProto& option);
     bool isOptionValid(const OptionProto& option);
-    bool isOptionValid(std::string option);
+    void applyOption(const OptionInfo& optionInfo);
-    void applyOption(const OptionProto& option);
+    bool applyOption(const std::string& name, const std::string value = "");
-    void applyOption(std::string option);
     void clearOptions();
 
     /* Adjust overall inputs and outputs. */
@@ -165,7 +177,7 @@ private:
     ConfigProto _baseConfig;
     ConfigProto _overridesConfig;
     ConfigProto _combinedConfig;
-    std::set<std::string> _appliedOptions;
+    std::set<OptionInfo> _appliedOptions;
     bool _configValid;
 
     FluxSourceProto _verificationFluxSourceProto;

lib/config/config.proto

@@ -73,5 +73,6 @@ message OptionProto
 message OptionGroupProto
 {
     optional string comment = 1 [(help) = "help text for option group"];
-    repeated OptionProto option = 2;
+    optional string name = 2 [(help) = "option group name"];
+    repeated OptionProto option = 3;
 }

lib/config/flags.cc

@@ -158,7 +158,7 @@ std::vector<std::string> FlagGroup::parseFlagsWithFilenames(int argc,
                         index += usesthat;
                     }
                     else
-                        globalConfig().applyOption(path);
+                        usesthat = globalConfig().applyOption(path, value);
                 }
                 else
                     error("unrecognised flag '-{}'; try --help", key);
@@ -197,6 +197,7 @@ void FlagGroup::parseFlagsWithConfigFiles(int argc,
     const char* argv[],
     const std::map<std::string, const ConfigProto*>& configFiles)
 {
+    globalConfig().readBaseConfigFile("_global_options");
     FlagGroup({this, &configGroup}).parseFlags(argc, argv);
 }
 

src/formats/40track_drive.textpb

@@ -1,22 +0,0 @@
-comment: 'Adjust configuration for a 40-track drive'
-is_extension: true
-
-documentation:
-<<<
-This is an extension profile; adding this to the command line will configure
-FluxEngine to read from 40-track, 48tpi 5.25" drives. You have to tell it because there is
-no way to detect this automatically.
-
-For example:
-
-```
-fluxengine read ibm --180 40track_drive
-```
->>>
-
-drive {
-    tracks: "c0-40h0-1"
-    drive_type: DRIVETYPE_40TRACK
-}
-
-

src/formats/_global_options.textpb

@@ -0,0 +1,77 @@
+comment: 'Options which can be applied everywhere.'
+is_extension: true
+
+option_group {
+	comment: "Drive type"
+    name: "drivetype"
+
+	option {
+		name: "80"
+		comment: '80 track drive'
+        set_by_default: true
+
+		config {
+		}
+	}
+
+	option {
+		name: "40"
+		comment: '40 track drive'
+
+		config {
+            drive {
+                tracks: "c0-40h0-1"
+                drive_type: DRIVETYPE_40TRACK
+            }
+		}
+	}
+
+	option {
+		name: "160"
+		comment: '160 track Apple II drive'
+
+		config {
+            drive {
+                tracks: "c0-159h0"
+                drive_type: DRIVETYPE_APPLE2
+            }
+		}
+	}
+}
+
+option_group {
+    comment: 'Bus interface'
+    name: "bus"
+
+	option {
+		name: "pc"
+		comment: 'PC drive interface'
+        set_by_default: true
+    }
+
+	option {
+		name: "shugart"
+		comment: 'Shugart bus interface (only on Greaseweazle)'
+
+		config {
+            usb {
+                greaseweazle {
+                    bus_type: SHUGART
+                }
+            }
+        }
+    }
+
+	option {
+		name: "appleii"
+		comment: 'Apple II bus interface (only on Greaseweazle)'
+
+		config {
+            usb {
+                greaseweazle {
+                    bus_type: APPLE2
+                }
+            }
+		}
+	}
+}

src/formats/apple2_drive.textpb

@@ -1,29 +0,0 @@
-comment: 'Adjust configuration for a 40-track Apple II drive'
-is_extension: true
-
-documentation:
-<<<
-This is an extension profile; adding this to the command line will configure
-FluxEngine to adjust the pinout and track spacing to work with an Apple II
-drive.  This only works on Greaseweazle hardware and requires a custom
-connector.
-
-For example:
-
-```
-fluxengine read apple2 --160 apple2_drive
-```
->>>
-
-usb {
-    greaseweazle {
-        bus_type: APPLE2
-    }
-}
-
-drive {
-    tracks: "c0-159h0"
-    drive_type: DRIVETYPE_APPLE2
-}
-
-

src/formats/build.py

@@ -3,14 +3,13 @@ from build.c import cxxlibrary
 from scripts.build import protoencode
 
 formats = [
-    "40track_drive",
+    "_global_options",
     "acornadfs",
     "acorndfs",
     "aeslanier",
     "agat",
     "amiga",
     "ampro",
-    "apple2_drive",
     "apple2",
     "atarist",
     "bk",
@@ -33,7 +32,6 @@ formats = [
     "psos",
     "rolandd20",
     "rx50",
-    "shugart_drive",
     "smaky6",
     "tartu",
     "ti99",

src/formats/shugart_drive.textpb

@@ -1,22 +0,0 @@
-comment: 'Adjust configuration for a Shugart drive'
-is_extension: true
-
-documentation:
-<<<
-This is an extension profile; adding this to the command line will configure
-FluxEngine to adjust the pinout to work with a Shugart drive. This only works
-on Greaseweazle hardware.
-
-For example:
-
-```
-fluxengine read ibm --720 shugart_drive
-```
->>>
-
-usb {
-    greaseweazle {
-        bus_type: SHUGART
-    }
-}
-

tests/options.cc

@@ -55,14 +55,14 @@ static void test_option_validity()
         }
 	)M");
 
-    AssertThat(
+    AssertThat(globalConfig().isOptionValid(
-        globalConfig().isOptionValid(globalConfig().findOption("option1")),
+                   *globalConfig().findOption("option1").option),
         Equals(true));
-    AssertThat(
+    AssertThat(globalConfig().isOptionValid(
-        globalConfig().isOptionValid(globalConfig().findOption("option2")),
+                   *globalConfig().findOption("option2").option),
         Equals(false));
-    AssertThat(
+    AssertThat(globalConfig().isOptionValid(
-        globalConfig().isOptionValid(globalConfig().findOption("option3")),
+                   *globalConfig().findOption("option3").option),
         Equals(true));
 }