Refactor selector argument parsing in cvd

Databean · Databean · commit 342ec18e208e · 2026-05-11T17:05:12.000-07:00
Avoid redundant parsing and string conversion of selector flags by passing
SelectorOptions directly from SeparateArguments to CommandRequest.

Assisted-by: Jetski
diff --git a/base/cvd/cuttlefish/host/commands/cvd/BUILD.bazel b/base/cvd/cuttlefish/host/commands/cvd/BUILD.bazel
@@ -16,6 +16,7 @@ cf_cc_library(
         "//cuttlefish/host/commands/cvd/cli:command_request",
         "//cuttlefish/host/commands/cvd/cli:frontline_parser",
         "//cuttlefish/host/commands/cvd/cli:nesting_commands",
+        "//cuttlefish/host/commands/cvd/cli/selector:selector_common_parser",
         "//cuttlefish/host/commands/cvd/instances",
         "//cuttlefish/host/commands/cvd/instances:instance_manager",
         "//cuttlefish/host/commands/cvd/instances/lock",
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/BUILD.bazel b/base/cvd/cuttlefish/host/commands/cvd/cli/BUILD.bazel
@@ -24,6 +24,7 @@ cf_cc_library(
     deps = [
         ":types",
         "//cuttlefish/host/commands/cvd/cli/selector:arguments_separator",
+        "//cuttlefish/host/commands/cvd/cli/selector:selector_common_parser",
         "//cuttlefish/result",
         "//libbase",
     ],
@@ -114,6 +115,7 @@ cf_cc_library(
         "//cuttlefish/host/commands/cvd/cli/parser:load_config_cc_proto",
         "//cuttlefish/host/commands/cvd/cli/parser:load_configs_parser",
         "//cuttlefish/host/commands/cvd/cli/selector:creation_analyzer",
+        "//cuttlefish/host/commands/cvd/cli/selector:selector_common_parser",
         "//cuttlefish/host/commands/cvd/fetch:fetch_cvd",
         "//cuttlefish/host/commands/cvd/instances",
         "//cuttlefish/host/commands/cvd/instances:cvd_persistent_data",
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/command_request.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/command_request.cpp
@@ -66,16 +66,6 @@ CommandRequestBuilder CommandRequestBuilder::AddArguments(
   return AddArguments(std::vector<std::string_view>(args));
 }
 
-CommandRequestBuilder& CommandRequestBuilder::AddSelectorArguments(
-    std::initializer_list<std::string_view> args) & {
-  return AddSelectorArguments(std::vector<std::string_view>(args));
-}
-
-CommandRequestBuilder CommandRequestBuilder::AddSelectorArguments(
-    std::initializer_list<std::string_view> args) && {
-  return AddSelectorArguments(std::vector<std::string_view>(args));
-}
-
 CommandRequestBuilder& CommandRequestBuilder::SetEnv(cvd_common::Envs env) & {
   env_ = std::move(env);
   return *this;
@@ -99,10 +89,8 @@ CommandRequestBuilder CommandRequestBuilder::AddEnvVar(std::string key,
 }
 
 Result<CommandRequest> CommandRequestBuilder::Build() && {
-  return CommandRequest(
-      std::move(args_), std::move(env_),
-      CF_EXPECT(selector::ParseCommonSelectorArguments(selector_args_),
-                "Failed to parse selector arguments"));
+  return CommandRequest(std::move(args_), std::move(env_),
+                        std::move(selector_options_));
 }
 
 }  // namespace cuttlefish
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/command_request.h b/base/cvd/cuttlefish/host/commands/cvd/cli/command_request.h
@@ -79,27 +79,18 @@ class CommandRequestBuilder {
   CommandRequestBuilder AddArguments(
       std::initializer_list<std::string_view>) &&;
 
-  template <typename T>
-  CommandRequestBuilder& AddSelectorArguments(T&& args) & {
-    for (auto&& arg : args) {
-      selector_args_.emplace_back(arg);
-    }
+  CommandRequestBuilder& SetSelectorOptions(
+      selector::SelectorOptions selectors) & {
+    selector_options_ = std::move(selectors);
     return *this;
   }
 
-  template <typename T>
-  CommandRequestBuilder AddSelectorArguments(T&& args) && {
-    for (auto&& arg : args) {
-      selector_args_.emplace_back(arg);
-    }
+  CommandRequestBuilder SetSelectorOptions(
+      selector::SelectorOptions selectors) && {
+    selector_options_ = std::move(selectors);
     return *this;
   }
 
-  CommandRequestBuilder& AddSelectorArguments(
-      std::initializer_list<std::string_view>) &;
-  CommandRequestBuilder AddSelectorArguments(
-      std::initializer_list<std::string_view>) &&;
-
   CommandRequestBuilder& SetEnv(cvd_common::Envs) &;
   CommandRequestBuilder SetEnv(cvd_common::Envs) &&;
 
@@ -111,7 +102,7 @@ class CommandRequestBuilder {
  private:
   cvd_common::Args args_;
   cvd_common::Envs env_;
-  cvd_common::Args selector_args_;
+  selector::SelectorOptions selector_options_;
 };
 
 }  // namespace cuttlefish
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/create.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/create.cpp
@@ -45,6 +45,7 @@
 #include "cuttlefish/host/commands/cvd/cli/commands/load_configs.h"
 #include "cuttlefish/host/commands/cvd/cli/commands/start.h"
 #include "cuttlefish/host/commands/cvd/cli/selector/creation_analyzer.h"
+#include "cuttlefish/host/commands/cvd/cli/selector/selector_common_parser.h"
 #include "cuttlefish/host/commands/cvd/cli/types.h"
 #include "cuttlefish/host/commands/cvd/instances/cvd_persistent_data.pb.h"
 #include "cuttlefish/host/commands/cvd/instances/instance_manager.h"
@@ -149,13 +150,15 @@ Result<CommandRequest> CreateLoadCommand(const CommandRequest& request,
 Result<CommandRequest> CreateStartCommand(const LocalInstanceGroup& group,
                                           const cvd_common::Args& args,
                                           const cvd_common::Envs& envs) {
-  return CF_EXPECT(
-      CommandRequestBuilder()
-          .SetEnv(envs)
-          .AddArguments({"cvd", "start"})
-          .AddArguments(args)
-          .AddSelectorArguments({"--group_name", group.GroupName()})
-          .Build());
+  selector::SelectorOptions selector_options{
+      .group_name = group.GroupName(),
+  };
+  return CF_EXPECT(CommandRequestBuilder()
+                       .SetEnv(envs)
+                       .AddArguments({"cvd", "start"})
+                       .AddArguments(args)
+                       .SetSelectorOptions(std::move(selector_options))
+                       .Build());
 }
 
 Result<cvd_common::Envs> GetEnvs(const CommandRequest& request) {
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/help.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/help.cpp
@@ -150,7 +150,7 @@ class CvdHelpHandler : public CvdCommandHandler {
                       .AddArguments(args)
                       .AddArguments({"--help"})
                       .SetEnv(request.Env())
-                      .AddSelectorArguments(request.Selectors().AsArgs())
+                      .SetSelectorOptions(request.Selectors())
                       .Build());
 
     std::stringstream help_message;
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/load_configs.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/load_configs.cpp
@@ -33,6 +33,7 @@
 #include "cuttlefish/host/commands/cvd/cli/commands/start.h"
 #include "cuttlefish/host/commands/cvd/cli/parser/load_config.pb.h"
 #include "cuttlefish/host/commands/cvd/cli/parser/load_configs_parser.h"
+#include "cuttlefish/host/commands/cvd/cli/selector/selector_common_parser.h"
 #include "cuttlefish/host/commands/cvd/cli/types.h"
 #include "cuttlefish/host/commands/cvd/fetch/fetch_cvd.h"
 #include "cuttlefish/host/commands/cvd/instances/cvd_persistent_data.pb.h"
@@ -224,6 +225,9 @@ class LoadConfigsCommand : public CvdCommandHandler {
         fmt::format("--system_image_dir={}", group.ProductOutPath());
     env.erase(kAndroidProductOut);
 
+    selector::SelectorOptions selector_options = cvd_flags.selector_flags;
+    selector_options.group_name = group.GroupName();
+
     return CF_EXPECT(
         CommandRequestBuilder()
             .SetEnv(env)
@@ -233,8 +237,7 @@ class LoadConfigsCommand : public CvdCommandHandler {
              */
             .AddArguments({"cvd", "start", "--daemon", system_build_arg})
             .AddArguments(cvd_flags.launch_cvd_flags)
-            .AddSelectorArguments(cvd_flags.selector_flags)
-            .AddSelectorArguments({"--group_name", group.GroupName()})
+            .SetSelectorOptions(std::move(selector_options))
             .Build());
   }
 
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/frontline_parser.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/frontline_parser.cpp
@@ -30,7 +30,7 @@ namespace cuttlefish {
 using selector::SeparateArguments;
 using selector::SeparatedArguments;
 
-Result<cvd_common::Args> ExtractCvdArgs(cvd_common::Args& args) {
+Result<selector::SelectorOptions> ExtractCvdArgs(cvd_common::Args& args) {
   CF_EXPECT(!args.empty());
 
   SeparatedArguments separated_arguments = CF_EXPECT(SeparateArguments(args));
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/frontline_parser.h b/base/cvd/cuttlefish/host/commands/cvd/cli/frontline_parser.h
@@ -16,12 +16,13 @@
 
 #pragma once
 
+#include "cuttlefish/host/commands/cvd/cli/selector/selector_common_parser.h"
 #include "cuttlefish/host/commands/cvd/cli/types.h"
 #include "cuttlefish/result/result.h"
 
 namespace cuttlefish {
 
 /* Returns `cvd` arguments before the subcommand, removing them from `args`. */
-Result<cvd_common::Args> ExtractCvdArgs(cvd_common::Args& args);
+Result<selector::SelectorOptions> ExtractCvdArgs(cvd_common::Args& args);
 
 }  // namespace cuttlefish
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/frontline_parser_test.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/frontline_parser_test.cpp
@@ -28,7 +28,8 @@ TEST(FrontlineParserTest, SelectorArgs) {
   auto result = ExtractCvdArgs(input);
 
   EXPECT_THAT(result, IsOk());
-  ASSERT_EQ(*result, std::vector<std::string>{"--instance_name=1"});
+  ASSERT_TRUE(result->instance_names.has_value());
+  ASSERT_EQ(result->instance_names.value(), std::vector<std::string>{"1"});
   ASSERT_EQ(input, (std::vector<std::string>{"cvd", "status"}));
 }
 
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/parser/BUILD.bazel b/base/cvd/cuttlefish/host/commands/cvd/cli/parser/BUILD.bazel
@@ -157,6 +157,7 @@ cf_cc_library(
         "//cuttlefish/host/commands/cvd/cli/parser:launch_cvd_parser",
         "//cuttlefish/host/commands/cvd/cli/parser:load_config_cc_proto",
         "//cuttlefish/host/commands/cvd/cli/parser:selector_parser",
+        "//cuttlefish/host/commands/cvd/cli/selector:selector_common_parser",
         "//cuttlefish/host/commands/cvd/instances",
         "//cuttlefish/host/commands/cvd/instances:instance_manager",
         "//cuttlefish/host/commands/cvd/utils:common",
@@ -186,8 +187,8 @@ cf_cc_library(
     srcs = ["selector_parser.cpp"],
     hdrs = ["selector_parser.h"],
     deps = [
-        "//cuttlefish/host/commands/cvd/cli/parser:cf_configs_common",
         "//cuttlefish/host/commands/cvd/cli/parser:load_config_cc_proto",
+        "//cuttlefish/host/commands/cvd/cli/selector:selector_common_parser",
     ],
 )
 
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/parser/load_configs_parser.h b/base/cvd/cuttlefish/host/commands/cvd/cli/parser/load_configs_parser.h
@@ -21,6 +21,7 @@
 #include <vector>
 
 #include "cuttlefish/host/commands/cvd/cli/parser/load_config.pb.h"
+#include "cuttlefish/host/commands/cvd/cli/selector/selector_common_parser.h"
 #include "cuttlefish/host/commands/cvd/instances/instance_manager.h"
 #include "cuttlefish/host/commands/cvd/instances/local_instance_group.h"
 #include "cuttlefish/result/result.h"
@@ -29,7 +30,7 @@ namespace cuttlefish {
 
 struct CvdFlags {
   std::vector<std::string> launch_cvd_flags;
-  std::vector<std::string> selector_flags;
+  selector::SelectorOptions selector_flags;
   std::vector<std::string> fetch_cvd_flags;
   std::string target_directory;
 };
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/parser/selector_parser.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/parser/selector_parser.cpp
@@ -18,26 +18,27 @@
 #include <string>
 #include <vector>
 
-#include "cuttlefish/host/commands/cvd/cli/parser/cf_configs_common.h"
 #include "cuttlefish/host/commands/cvd/cli/parser/load_config.pb.h"
+#include "cuttlefish/host/commands/cvd/cli/selector/selector_common_parser.h"
 
 namespace cuttlefish {
 
 using cvd::config::EnvironmentSpecification;
-using cvd::config::Instance;
 
-std::string InsName(const Instance& instance) { return instance.name(); }
-
-std::vector<std::string> ParseSelectorConfigs(
+selector::SelectorOptions ParseSelectorConfigs(
     const EnvironmentSpecification& config) {
-  auto ins_name_flag = GenerateInstanceFlag("instance_name", config, InsName);
+  selector::SelectorOptions options;
+
+  options.instance_names = std::vector<std::string>();
+  for (const auto& instance : config.instances()) {
+    options.instance_names->push_back(instance.name());
+  }
 
-  if (!config.common().has_group_name()) {
-    return {ins_name_flag};
+  if (config.common().has_group_name()) {
+    options.group_name = config.common().group_name();
   }
 
-  auto group_flag = GenerateFlag("group_name", config.common().group_name());
-  return {ins_name_flag, group_flag};
+  return options;
 }
 
 }  // namespace cuttlefish
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/parser/selector_parser.h b/base/cvd/cuttlefish/host/commands/cvd/cli/parser/selector_parser.h
@@ -16,14 +16,12 @@
 
 #pragma once
 
-#include <string>
-#include <vector>
-
 #include "cuttlefish/host/commands/cvd/cli/parser/load_config.pb.h"
+#include "cuttlefish/host/commands/cvd/cli/selector/selector_common_parser.h"
 
 namespace cuttlefish {
 
-std::vector<std::string> ParseSelectorConfigs(
+selector::SelectorOptions ParseSelectorConfigs(
     const cvd::config::EnvironmentSpecification&);
 
 };  // namespace cuttlefish
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/selector/arguments_separator.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/selector/arguments_separator.cpp
@@ -51,7 +51,7 @@ Result<SeparatedArguments> SeparateArguments(
 
   // Mutates `args` to remove selector arguments
   SelectorOptions selectors = CF_EXPECT(ParseCommonSelectorArguments(args));
-  output.cvd_args = selectors.AsArgs();
+  output.cvd_args = std::move(selectors);
 
   if (!args.empty()) {
     output.sub_cmd = args[0];
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/selector/arguments_separator.h b/base/cvd/cuttlefish/host/commands/cvd/cli/selector/arguments_separator.h
@@ -20,14 +20,15 @@
 #include <string>
 #include <vector>
 
+#include "cuttlefish/host/commands/cvd/cli/selector/selector_common_parser.h"
 #include "cuttlefish/result/result.h"
 
 namespace cuttlefish {
 namespace selector {
 
 struct SeparatedArguments {
   std::string prog_path;
-  std::vector<std::string> cvd_args;
+  SelectorOptions cvd_args;
   std::optional<std::string> sub_cmd;
   std::vector<std::string> sub_cmd_args;
 };
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/selector/selector_common_parser.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/selector/selector_common_parser.cpp
@@ -36,19 +36,6 @@
 namespace cuttlefish {
 namespace selector {
 
-std::vector<std::string> SelectorOptions::AsArgs() const {
-  std::vector<std::string> ret;
-  if (group_name) {
-    ret.push_back(
-        fmt::format("--{}={}", SelectorFlags::kGroupName, *group_name));
-  }
-  if (instance_names) {
-    ret.push_back(fmt::format("--{}={}", SelectorFlags::kInstanceName,
-                              absl::StrJoin(*instance_names, ",")));
-  }
-  return ret;
-}
-
 Result<std::string> HandleGroupName(const std::string& group_name) {
   CF_EXPECTF(IsValidGroupName(group_name), "Invalid group name: {}",
              group_name);
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/selector/selector_common_parser.h b/base/cvd/cuttlefish/host/commands/cvd/cli/selector/selector_common_parser.h
@@ -32,7 +32,6 @@ struct SelectorOptions {
   bool HasOptions() const {
     return group_name.has_value() || instance_names.has_value();
   }
-  std::vector<std::string> AsArgs() const;
 };
 
 // Parses and consumes the selector arguments from the given argument list
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cvd.cpp b/base/cvd/cuttlefish/host/commands/cvd/cvd.cpp
@@ -41,12 +41,13 @@ Cvd::Cvd(InstanceManager& instance_manager,
 Result<void> Cvd::HandleCommand(
     const std::vector<std::string>& cvd_process_args,
     const std::unordered_map<std::string, std::string>& env,
-    const std::vector<std::string>& selector_args) {
-  CommandRequest request = CF_EXPECT(CommandRequestBuilder()
-                                         .AddArguments(cvd_process_args)
-                                         .SetEnv(env)
-                                         .AddSelectorArguments(selector_args)
-                                         .Build());
+    selector::SelectorOptions selector_args) {
+  CommandRequest request =
+      CF_EXPECT(CommandRequestBuilder()
+                    .AddArguments(cvd_process_args)
+                    .SetEnv(env)
+                    .SetSelectorOptions(std::move(selector_args))
+                    .Build());
 
   RequestContext context(instance_manager_, lock_file_manager_);
   auto handler = CF_EXPECT(context.Handler(request));
@@ -66,9 +67,9 @@ Result<void> Cvd::HandleCvdCommand(
   if (args.size() == 1ul) {
     args = cvd_common::Args{"cvd", "help"};
   }
-  std::vector<std::string> selector_args = CF_EXPECT(ExtractCvdArgs(args));
+  selector::SelectorOptions selector_args = CF_EXPECT(ExtractCvdArgs(args));
   // TODO(schuffelen): Deduplicate cvd process split.
-  CF_EXPECT(HandleCommand(args, env, selector_args));
+  CF_EXPECT(HandleCommand(args, env, std::move(selector_args)));
   return {};
 }
 
diff --git a/base/cvd/cuttlefish/host/commands/cvd/cvd.h b/base/cvd/cuttlefish/host/commands/cvd/cvd.h
@@ -20,6 +20,7 @@
 #include <unordered_map>
 #include <vector>
 
+#include "cuttlefish/host/commands/cvd/cli/selector/selector_common_parser.h"
 #include "cuttlefish/host/commands/cvd/instances/instance_manager.h"
 #include "cuttlefish/host/commands/cvd/instances/lock/instance_lock.h"
 #include "cuttlefish/result/result.h"
@@ -33,7 +34,7 @@ class Cvd {
   Result<void> HandleCommand(
       const std::vector<std::string>& cvd_process_args,
       const std::unordered_map<std::string, std::string>& env,
-      const std::vector<std::string>& selector_args);
+      selector::SelectorOptions selector_args);
 
   Result<void> HandleCvdCommand(
       const std::vector<std::string>& all_args,

Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,8 @@ TEST(FrontlineParserTest, SelectorArgs) {`
`28`	`28`	`auto result = ExtractCvdArgs(input);`
`29`	`29`
`30`	`30`	`EXPECT_THAT(result, IsOk());`
`31`		`- ASSERT_EQ(*result, std::vector<std::string>{"--instance_name=1"});`
	`31`	`+ ASSERT_TRUE(result->instance_names.has_value());`
	`32`	`+ ASSERT_EQ(result->instance_names.value(), std::vector<std::string>{"1"});`
`32`	`33`	`ASSERT_EQ(input, (std::vector<std::string>{"cvd", "status"}));`
`33`	`34`	`}`
`34`	`35`