From 4607ac7aed34b1bc2d7a74bff99c63f3bd684511 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 16 Jan 2023 19:13:31 -0500 Subject: [PATCH] Fix handling of experimental features mid-parse If we conditionally "declare" the argument, as we did before, based upon weather the feature is enabled, commands like nix --experimental-features=foo ... --thing-gated-on-foo won't work, because the experimental feature isn't enabled until *after* we start parsing. Instead, allow arguments to also be associated with experimental features (just as we did for builtins and settings), and then the command line parser will filter out the experimental ones. Since the effects of arguments (handler functions) are performed right away, we get the required behavior: earlier arguments can enable later arguments enabled! There is just one catch: we want to keep non-positional flags...non-positional. So if nix --experimental-features=foo ... --thing-gated-on-foo works, then nix --thing-gated-on-foo --experimental-features=foo ... should also work. This is not my favorite long-term solution, but for now this is implemented by delaying the requirement of needed experimental features until *after* all the arguments have been parsed. --- src/libutil/args.cc | 25 ++++++++++++++++++++++--- src/libutil/args.hh | 14 ++++++++++++-- src/libutil/config.cc | 16 ++++++++++++---- src/libutil/util.hh | 1 - src/nix/main.cc | 13 +++++++------ 5 files changed, 53 insertions(+), 16 deletions(-) diff --git a/src/libutil/args.cc b/src/libutil/args.cc index 35686a8aa..fc009592c 100644 --- a/src/libutil/args.cc +++ b/src/libutil/args.cc @@ -52,7 +52,7 @@ std::shared_ptr completions; std::string completionMarker = "___COMPLETE___"; -std::optional needsCompletion(std::string_view s) +static std::optional needsCompletion(std::string_view s) { if (!completions) return {}; auto i = s.find(completionMarker); @@ -120,6 +120,12 @@ void Args::parseCmdline(const Strings & _cmdline) if (!argsSeen) initialFlagsProcessed(); + + /* Now that we are done parsing, make sure that any experimental + * feature required by the flags is enabled */ + for (auto & f : flagExperimentalFeatures) + experimentalFeatureSettings.require(f); + } bool Args::processFlag(Strings::iterator & pos, Strings::iterator end) @@ -128,12 +134,18 @@ bool Args::processFlag(Strings::iterator & pos, Strings::iterator end) auto process = [&](const std::string & name, const Flag & flag) -> bool { ++pos; + + if (auto & f = flag.experimentalFeature) + flagExperimentalFeatures.insert(*f); + std::vector args; bool anyCompleted = false; for (size_t n = 0 ; n < flag.handler.arity; ++n) { if (pos == end) { if (flag.handler.arity == ArityAny || anyCompleted) break; - throw UsageError("flag '%s' requires %d argument(s)", name, flag.handler.arity); + throw UsageError( + "flag '%s' requires %d argument(s), but only %d were given", + name, flag.handler.arity, n); } if (auto prefix = needsCompletion(*pos)) { anyCompleted = true; @@ -152,7 +164,11 @@ bool Args::processFlag(Strings::iterator & pos, Strings::iterator end) for (auto & [name, flag] : longFlags) { if (!hiddenCategories.count(flag->category) && hasPrefix(name, std::string(*prefix, 2))) + { + if (auto & f = flag->experimentalFeature) + flagExperimentalFeatures.insert(*f); completions->add("--" + name, flag->description); + } } return false; } @@ -172,7 +188,8 @@ bool Args::processFlag(Strings::iterator & pos, Strings::iterator end) if (prefix == "-") { completions->add("--"); for (auto & [flagName, flag] : shortFlags) - completions->add(std::string("-") + flagName, flag->description); + if (experimentalFeatureSettings.isEnabled(flag->experimentalFeature)) + completions->add(std::string("-") + flagName, flag->description); } } @@ -219,6 +236,8 @@ nlohmann::json Args::toJSON() auto flags = nlohmann::json::object(); for (auto & [name, flag] : longFlags) { + /* Skip experimental flags when listing flags. */ + if (!experimentalFeatureSettings.isEnabled(flag->experimentalFeature)) continue; auto j = nlohmann::json::object(); if (flag->aliases.count(name)) continue; if (flag->shortName) diff --git a/src/libutil/args.hh b/src/libutil/args.hh index 7211ee307..2969806dd 100644 --- a/src/libutil/args.hh +++ b/src/libutil/args.hh @@ -117,6 +117,8 @@ protected: Handler handler; std::function completer; + std::optional experimentalFeature; + static Flag mkHashTypeFlag(std::string && longName, HashType * ht); static Flag mkHashTypeOptFlag(std::string && longName, std::optional * oht); }; @@ -188,6 +190,16 @@ public: friend class MultiCommand; MultiCommand * parent = nullptr; + +private: + + /** + * Experimental features needed when parsing args. These are checked + * after flag parsing is completed in order to support enabling + * experimental features coming after the flag that needs the + * experimental feature. + */ + std::set flagExperimentalFeatures; }; /* A command is an argument parser that can be executed by calling its @@ -253,8 +265,6 @@ enum CompletionType { }; extern CompletionType completionType; -std::optional needsCompletion(std::string_view s); - void completePath(size_t, std::string_view prefix); void completeDir(size_t, std::string_view prefix); diff --git a/src/libutil/config.cc b/src/libutil/config.cc index 0dbb4de81..8d63536d6 100644 --- a/src/libutil/config.cc +++ b/src/libutil/config.cc @@ -170,9 +170,13 @@ std::string Config::toKeyValue() void Config::convertToArgs(Args & args, const std::string & category) { - for (auto & s : _settings) - if (applicable(s.second)) + for (auto & s : _settings) { + /* We do include args for settings gated on disabled + experimental-features. The args themselves however will also be + gated on any experimental feature the underlying setting is. */ + if (!s.second.isAlias) s.second.setting->convertToArg(args, category); + } } AbstractSetting::AbstractSetting( @@ -219,6 +223,7 @@ void BaseSetting::convertToArg(Args & args, const std::string & category) .category = category, .labels = {"value"}, .handler = {[this](std::string s) { overridden = true; set(s); }}, + .experimentalFeature = experimentalFeature, }); if (isAppendable()) @@ -228,6 +233,7 @@ void BaseSetting::convertToArg(Args & args, const std::string & category) .category = category, .labels = {"value"}, .handler = {[this](std::string s) { overridden = true; set(s, true); }}, + .experimentalFeature = experimentalFeature, }); } @@ -279,13 +285,15 @@ template<> void BaseSetting::convertToArg(Args & args, const std::string & .longName = name, .description = fmt("Enable the `%s` setting.", name), .category = category, - .handler = {[this]() { override(true); }} + .handler = {[this]() { override(true); }}, + .experimentalFeature = experimentalFeature, }); args.addFlag({ .longName = "no-" + name, .description = fmt("Disable the `%s` setting.", name), .category = category, - .handler = {[this]() { override(false); }} + .handler = {[this]() { override(false); }}, + .experimentalFeature = experimentalFeature, }); } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 52ca36fd1..c4ea6c34b 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -450,7 +450,6 @@ template Strings quoteStrings(const C & c) return res; } - /* Remove trailing whitespace from a string. FIXME: return std::string_view. */ std::string chomp(std::string_view s); diff --git a/src/nix/main.cc b/src/nix/main.cc index da920615f..c79d39459 100644 --- a/src/nix/main.cc +++ b/src/nix/main.cc @@ -54,12 +54,11 @@ static bool haveInternet() std::string programPath; -struct HelpRequested { }; - struct NixArgs : virtual MultiCommand, virtual MixCommonArgs { bool useNet = true; bool refresh = false; + bool helpRequested = false; bool showVersion = false; NixArgs() : MultiCommand(RegisterCommand::getCommandsFor({})), MixCommonArgs("nix") @@ -74,7 +73,7 @@ struct NixArgs : virtual MultiCommand, virtual MixCommonArgs .longName = "help", .description = "Show usage information.", .category = miscCategory, - .handler = {[&]() { throw HelpRequested(); }}, + .handler = {[this]() { this->helpRequested = true; }}, }); addFlag({ @@ -337,7 +336,11 @@ void mainWrapped(int argc, char * * argv) try { args.parseCmdline(argvToStrings(argc, argv)); - } catch (HelpRequested &) { + } catch (UsageError &) { + if (!args.helpRequested && !completions) throw; + } + + if (args.helpRequested) { std::vector subcommand; MultiCommand * command = &args; while (command) { @@ -349,8 +352,6 @@ void mainWrapped(int argc, char * * argv) } showHelp(subcommand, args); return; - } catch (UsageError &) { - if (!completions) throw; } if (completions) {