cvd: Better help for load and start subcommands

base/cvd/cuttlefish/common/libs/utils/flag_parser.cpp

@@ -425,10 +425,10 @@ std::ostream& operator<<(std::ostream& out, const Flag& flag) {
   }
   out << "\n";
   if (flag.help_) {
-    out << "  " << *flag.help_ << "\n";
+    out <<  *flag.help_ << "\n";
   }
   if (flag.getter_) {
-    out << "  Current value: \"" << (*flag.getter_)() << "\"\n";
+    out << "Current value: \"" << (*flag.getter_)() << "\"\n";
   }
   return out;
 }

base/cvd/cuttlefish/common/libs/utils/flag_parser.h

@@ -95,6 +95,7 @@ class Flag {
   Flag AddValidator(std::function<Result<void>()>) &&;
 
   const std::string& Name() const { return name_; }
+  bool operator<(const Flag& other) const { return Name() < other.Name(); }
 
   /* Examines a list of arguments, removing any matches from the list and
    * invoking the `Setter` for every match. Returns `false` if the callback ever

base/cvd/cuttlefish/host/commands/cvd/cli/BUILD.bazel

@@ -40,6 +40,18 @@ cf_cc_test(
     ],
 )
 
+cf_cc_library(
+    name = "help_format",
+    srcs = ["help_format.cpp"],
+    hdrs = ["help_format.h"],
+    deps = [
+        "//cuttlefish/common/libs/utils:flag_parser",
+        "@abseil-cpp//absl/log",
+        "@abseil-cpp//absl/log:check",
+        "@abseil-cpp//absl/strings",
+    ],
+)
+
 cf_cc_library(
     name = "interruptible_terminal",
     srcs = ["interruptible_terminal.cpp"],
@@ -104,6 +116,7 @@ cf_cc_library(
         "//cuttlefish/common/libs/utils:subprocess",
         "//cuttlefish/common/libs/utils:users",
         "//cuttlefish/host/commands/cvd/cli:command_request",
+        "//cuttlefish/host/commands/cvd/cli:help_format",
         "//cuttlefish/host/commands/cvd/cli:types",
         "//cuttlefish/host/commands/cvd/cli/commands:bugreport",
         "//cuttlefish/host/commands/cvd/cli/commands:cache",

base/cvd/cuttlefish/host/commands/cvd/cli/commands/BUILD.bazel

@@ -82,9 +82,11 @@ cf_cc_library(
     srcs = ["command_handler.cpp"],
     hdrs = ["command_handler.h"],
     deps = [
-        "//cuttlefish/common/libs/utils:contains",
+        "//cuttlefish/common/libs/utils:flag_parser",
         "//cuttlefish/host/commands/cvd/cli:command_request",
+        "//cuttlefish/host/commands/cvd/cli:help_format",
         "//cuttlefish/host/commands/cvd/cli:types",
+        "//cuttlefish/host/commands/cvd/cli/selector:selector_common_parser",
         "//cuttlefish/result",
     ],
 )
@@ -196,7 +198,7 @@ cf_cc_library(
     srcs = ["lint.cpp"],
     hdrs = ["lint.h"],
     deps = [
-        "//cuttlefish/common/libs/utils:files",
+        "//cuttlefish/common/libs/utils:flag_parser",
         "//cuttlefish/host/commands/cvd/cli:command_request",
         "//cuttlefish/host/commands/cvd/cli:types",
         "//cuttlefish/host/commands/cvd/cli/commands:command_handler",
@@ -425,12 +427,12 @@ cf_cc_library(
         "//cuttlefish/common/libs/utils:subprocess_managed_stdio",
         "//cuttlefish/common/libs/utils:users",
         "//cuttlefish/host/commands/cvd/cli:command_request",
+        "//cuttlefish/host/commands/cvd/cli:help_format",
         "//cuttlefish/host/commands/cvd/cli:types",
         "//cuttlefish/host/commands/cvd/cli:utils",
         "//cuttlefish/host/commands/cvd/cli/commands:command_handler",
         "//cuttlefish/host/commands/cvd/cli/commands:host_tool_target",
         "//cuttlefish/host/commands/cvd/cli/selector",
-        "//cuttlefish/host/commands/cvd/cli/selector:selector_common_parser",
         "//cuttlefish/host/commands/cvd/fetch:substitute",
         "//cuttlefish/host/commands/cvd/instances",
         "//cuttlefish/host/commands/cvd/instances:cvd_persistent_data",

base/cvd/cuttlefish/host/commands/cvd/cli/commands/command_handler.cpp

@@ -16,8 +16,62 @@
 
 #include "cuttlefish/host/commands/cvd/cli/commands/command_handler.h"
 
+#include <stddef.h>
+
+#include <algorithm>
+#include <sstream>
+#include <string>
+#include <vector>
+
+#include "cuttlefish/common/libs/utils/flag_parser.h"
+#include "cuttlefish/host/commands/cvd/cli/command_request.h"
+#include "cuttlefish/host/commands/cvd/cli/help_format.h"
+#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 {
 
 bool CvdCommandHandler::RequiresDeviceExists() const { return false; }
 
+std::vector<HelpParagraph> CvdCommandHandler::Description() const { return {}; }
+
+Result<std::vector<Flag>> CvdCommandHandler::Flags(const CommandRequest&) {
+  return {};
+}
+
+Result<std::string> CvdCommandHandler::DetailedHelp(
+    const CommandRequest& request) {
+  std::stringstream ss;
+  std::vector<std::string> cmd_list = CmdList();
+  CF_EXPECT(!cmd_list.empty(), "Command aliases list is empty");
+
+  ss << "cvd " << cmd_list[0] << " - " << SummaryHelp() << "\n";
+  ss << "\n";
+  ss << FormatHelpText(Description());
+
+  std::vector<Flag> flags = CF_EXPECT(Flags(request));
+  // Consume flags to ensure "current value" is accurate
+  cvd_common::Args args = request.SubcommandArguments();
+  CF_EXPECT(ConsumeFlags(flags, args));
+
+  selector::SelectorOptions selector_options = request.Selectors();
+  if (RequiresDeviceExists()) {
+    // Add the common selector flags if the command supports them. This doesn't
+    // need to hapen before consuming as the selector flags were consumed
+    // already. Using the selectors from the request ensures the flags's
+    // "current value" is correct.
+    std::vector<Flag> selector_flags =
+        selector::BuildCommonSelectorFlags(selector_options);
+    flags.insert(flags.end(), selector_flags.begin(), selector_flags.end());
+  }
+
+  // Make sure the flags are in alphabetical order
+  std::sort(flags.begin(), flags.end());
+
+  ss << FormatFlagsHelp(flags);
+
+  return ss.str();
+}
+
 }  // namespace cuttlefish

base/cvd/cuttlefish/host/commands/cvd/cli/commands/command_handler.h

@@ -18,7 +18,9 @@
 
 #include <string>
 
+#include "cuttlefish/common/libs/utils/flag_parser.h"
 #include "cuttlefish/host/commands/cvd/cli/command_request.h"
+#include "cuttlefish/host/commands/cvd/cli/help_format.h"
 #include "cuttlefish/host/commands/cvd/cli/types.h"
 #include "cuttlefish/result/result.h"
 
@@ -34,8 +36,11 @@ class CvdCommandHandler {
   // used for command help text
   virtual std::string SummaryHelp() const = 0;
   virtual bool RequiresDeviceExists() const;
+  virtual Result<std::string> DetailedHelp(const CommandRequest&);
+  virtual std::vector<HelpParagraph> Description() const;
+  virtual Result<std::vector<Flag>> Flags(const CommandRequest&);
+
   virtual bool RequiresHostConfiguration() const { return true; }
-  virtual Result<std::string> DetailedHelp(const CommandRequest&) = 0;
 };
 
 }  // namespace cuttlefish

base/cvd/cuttlefish/host/commands/cvd/cli/commands/lint.cpp

@@ -21,7 +21,7 @@
 #include <string>
 #include <vector>
 
-#include "cuttlefish/common/libs/utils/files.h"
+#include "cuttlefish/common/libs/utils/flag_parser.h"
 #include "cuttlefish/host/commands/cvd/cli/command_request.h"
 #include "cuttlefish/host/commands/cvd/cli/commands/command_handler.h"
 #include "cuttlefish/host/commands/cvd/cli/parser/load_configs_parser.h"
@@ -47,8 +47,7 @@ Usage: cvd lint /path/to/input.json
 
 Result<void> LintCommandHandler::Handle(const CommandRequest& request) {
   std::vector<std::string> args = request.SubcommandArguments();
-  auto working_directory = CurrentDirectory();
-  const auto config_path = CF_EXPECT(ValidateConfig(args, working_directory));
+  const auto config_path = CF_EXPECT(ValidateConfig(args));
 
   std::cout << "Lint of flags and config \"" << config_path << "\" succeeded\n";
 
@@ -65,10 +64,16 @@ Result<std::string> LintCommandHandler::DetailedHelp(
 }
 
 Result<std::string> LintCommandHandler::ValidateConfig(
-    std::vector<std::string>& args, const std::string& working_directory) {
-  const LoadFlags flags = CF_EXPECT(GetFlags(args, working_directory));
-  CF_EXPECT(GetEnvironmentSpecification(flags));
-  return flags.config_path;
+    std::vector<std::string>& args) {
+  LoadFlags load_flags;
+  std::vector<Flag> flags = BuildCvdLoadFlags(load_flags);
+  CF_EXPECT(ConsumeFlags(flags, args));
+  CF_EXPECT(
+      !args.empty(),
+      "No arguments provided to cvd command, please provide path to json file");
+  std::string& config_path = args.front();
+  CF_EXPECT(GetEnvironmentSpecification(config_path, load_flags.overrides));
+  return config_path;
 }
 
 std::unique_ptr<CvdCommandHandler> NewLintCommand() {

base/cvd/cuttlefish/host/commands/cvd/cli/commands/lint.h

@@ -34,8 +34,7 @@ class LintCommandHandler : public CvdCommandHandler {
   Result<std::string> DetailedHelp(const CommandRequest& request) override;
 
  private:
-  Result<std::string> ValidateConfig(std::vector<std::string>& args,
-                                     const std::string& working_directory);
+  Result<std::string> ValidateConfig(std::vector<std::string>& args);
 };
 
 std::unique_ptr<CvdCommandHandler> NewLintCommand();

base/cvd/cuttlefish/host/commands/cvd/cli/commands/load_configs.cpp

@@ -27,10 +27,12 @@
 #include "absl/log/log.h"
 
 #include "cuttlefish/common/libs/utils/files.h"
+#include "cuttlefish/common/libs/utils/flag_parser.h"
 #include "cuttlefish/host/commands/cvd/cli/command_request.h"
 #include "cuttlefish/host/commands/cvd/cli/commands/command_handler.h"
 #include "cuttlefish/host/commands/cvd/cli/commands/fetch.h"
 #include "cuttlefish/host/commands/cvd/cli/commands/start.h"
+#include "cuttlefish/host/commands/cvd/cli/help_format.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"
@@ -52,26 +54,8 @@ namespace {
 constexpr char kLoadSubCmd[] = "load";
 
 constexpr char kSummaryHelpText[] =
-    R"(Loads the given JSON configuration file and launches devices based on the options provided)";
+    "Creates and starts an instance group from a JSON configuration file";
 
-constexpr char kDetailedHelpText[] = R"(
-Warning: This command is deprecated, use cvd create --config_file instead.
-
-Usage:
-cvd load <config_filepath> [--override=<key>:<value>]
-
-Reads the fields in the JSON configuration file and translates them to corresponding start command and flags.
-
-Optionally fetches remote artifacts prior to launching the cuttlefish environment.
-
-The --override flag can be used to give new values for properties in the config file without needing to edit the file directly.  Convenient for one-off invocations.
-)";
-
-Result<LoadFlags> GetLoadFlags(const CommandRequest& request) {
-  std::vector<std::string> args = request.SubcommandArguments();
-  auto working_directory = CurrentDirectory();
-  return CF_EXPECT(GetFlags(args, working_directory));
-}
 
 Result<CommandRequest> BuildFetchCmd(const CommandRequest& request,
                                      const CvdFlags& cvd_flags) {
@@ -133,9 +117,16 @@ LoadConfigsCommand::LoadConfigsCommand(InstanceManager& instance_manager)
     : instance_manager_(instance_manager) {}
 
 Result<void> LoadConfigsCommand::Handle(const CommandRequest& request) {
-  LoadFlags load_flags = CF_EXPECT(GetLoadFlags(request));
+  std::vector<std::string> args = request.SubcommandArguments();
+  std::vector<Flag> flags = CF_EXPECT(Flags(request));
+  CF_EXPECT(ConsumeFlags(flags, args));
+  CF_EXPECT(
+      !args.empty(),
+      "No arguments provided to cvd command, please provide path to json file");
+  std::string& config_path = args.front();
+
   EnvironmentSpecification env_spec =
-      CF_EXPECT(GetEnvironmentSpecification(load_flags));
+      CF_EXPECT(GetEnvironmentSpecification(config_path, flags_.overrides));
 
   std::mutex group_creation_mtx;
   // Have to use the group name because LocalInstanceGroup can't be default
@@ -181,7 +172,7 @@ Result<void> LoadConfigsCommand::Handle(const CommandRequest& request) {
   group_creation_mtx.lock();
   // Don't use CF_EXPECT here or the mutex will be left locked.
   auto group_res =
-      CreateGroup(instance_manager_, load_flags.base_dir, env_spec);
+      CreateGroup(instance_manager_, flags_.base_dir, env_spec);
   if (group_res.ok()) {
     // Have to initialize the group_name variable before releasing the mutex.
     group_name = (*group_res).GroupName();
@@ -250,9 +241,79 @@ cvd_common::Args LoadConfigsCommand::CmdList() const { return {kLoadSubCmd}; }
 
 std::string LoadConfigsCommand::SummaryHelp() const { return kSummaryHelpText; }
 
-Result<std::string> LoadConfigsCommand::DetailedHelp(
+std::vector<HelpParagraph> LoadConfigsCommand::Description() const {
+  std::vector<HelpParagraph> description;
+  description.emplace_back(
+      "This command is an alias of `cvd create --config_file=<config_filepath> "
+      "[--override=<key>:<value>]...`, provided for convenience and backwards "
+      "compatibility.");
+
+  description.emplace_back("Usage:");
+
+  description.emplace_back(
+      "    cvd load <config_filepath> [--override=<key>:<value>]");
+
+  description.emplace_back(
+      "Creates and starts a new instance group from a specification file. An "
+      "example specification file looks like:");
+
+  description.emplace_back(HelpParagraph::Raw(R"(  {
+    "instances": [
+      {
+        "name": "ins-1",
+        "disk": {
+          "default_build": "@ab/aosp-android-latest-release/aosp_cf_x86_64_only_phone-userdebug"
+        },
+        "vm": {
+          "cpus": 8,
+          "memory_mb": 2048
+        }
+      },
+      {
+        "name": "ins-2",
+        "disk": {
+          "default_build": "/path/to/android/build"
+        }
+      }
+    ]
+  })"));
+
+  description.emplace_back(
+      "A complete reference of the specification file format can be found in "
+      "https://github.com/google/android-cuttlefish/blob/main/base/cvd/"
+      "cuttlefish/host/commands/cvd/cli/parser/load_config.proto.");
+
+  description.emplace_back(
+      "While most config file properties are self explanatory, the build "
+      "properties (default_build, kernel.build, bootloader.build, etc) require "
+      "more explanation. These properties support two types of values:");
+
+  description.emplace_back(HelpParagraph::Raw(
+      R"( - "@ab/<branch_or_build_id>[/<target>[{<filepath>}]]"
+ - "<absolute_path>")"));
+
+  description.emplace_back(
+      "If the build value starts with \"@ab\", cvd will fetch the specified "
+      "Android build target from the Android build servers. By default it will "
+      "download the cuttlefish host package archive or the images zip as "
+      "needed, but for more advanced use cases the file to download from the "
+      "server can be specified with the <filepath> optional parameter in curly "
+      "braces. For more information on build fetching and caching operations "
+      "refer to `cvd help fetch`.");
+
+  description.emplace_back(
+      "Alternatively, the build value may point to an absolute path (starts "
+      "with '/') in the filesystem where the Android source code has been "
+      "checked out and a Cuttlefish target has been built. This is "
+      "particularly useful for rapid iteration during development in "
+      "combination with incremental builds and the `cvd stop` and `cvd start` "
+      "subcommands.");
+  return description;
+}
+
+Result<std::vector<Flag>> LoadConfigsCommand::Flags(
     const CommandRequest& request) {
-  return kDetailedHelpText;
+  return BuildCvdLoadFlags(flags_);
 }
 
 std::unique_ptr<CvdCommandHandler> NewLoadConfigsCommand(

base/cvd/cuttlefish/host/commands/cvd/cli/commands/load_configs.h

@@ -20,6 +20,7 @@
 
 #include "cuttlefish/host/commands/cvd/cli/command_request.h"
 #include "cuttlefish/host/commands/cvd/cli/commands/command_handler.h"
+#include "cuttlefish/host/commands/cvd/cli/help_format.h"
 #include "cuttlefish/host/commands/cvd/cli/parser/load_configs_parser.h"
 #include "cuttlefish/host/commands/cvd/instances/instance_manager.h"
 #include "cuttlefish/host/commands/cvd/instances/local_instance_group.h"
@@ -35,13 +36,15 @@ class LoadConfigsCommand : public CvdCommandHandler {
   Result<void> Handle(const CommandRequest& request) override;
   cvd_common::Args CmdList() const override;
   std::string SummaryHelp() const override;
-  Result<std::string> DetailedHelp(const CommandRequest& request) override;
+  std::vector<HelpParagraph> Description() const override;
+  Result<std::vector<Flag>> Flags(const CommandRequest&) override;
 
  private:
   Result<void> LoadGroup(const CommandRequest& request,
                          LocalInstanceGroup& group, CvdFlags cvd_flags);
 
   InstanceManager& instance_manager_;
+  LoadFlags flags_;
 };
 
 /*