cmCTest*Command: Generalize argument parsing and checking

Move the instantiation of unparsed arguments as well as the `parser.Parse` call
behind an abstraction.  Merge checks on `captureCMakeError`.

Source/CTest/cmCTestBuildCommand.cxx

@@ -43,9 +43,9 @@ bool cmCTestBuildCommand::InitialPass(std::vector<std::string> const& args,
       .Bind("PROJECT_NAME"_s, &BuildArguments::ProjectName)
       .Bind("PARALLEL_LEVEL"_s, &BuildArguments::ParallelLevel);
 
-  std::vector<std::string> unparsedArguments;
-  BuildArguments arguments = parser.Parse(args, &unparsedArguments);
-  return this->ExecuteHandlerCommand(arguments, unparsedArguments, status);
+  return this->Invoke(parser, args, status, [&](BuildArguments& a) {
+    return this->ExecuteHandlerCommand(a, status);
+  });
 }
 
 std::unique_ptr<cmCTestGenericHandler> cmCTestBuildCommand::InitializeHandler(

Source/CTest/cmCTestConfigureCommand.cxx

@@ -170,7 +170,7 @@ bool cmCTestConfigureCommand::InitialPass(std::vector<std::string> const& args,
     cmArgumentParser<Args>{ MakeHandlerParser<Args>() } //
       .Bind("OPTIONS"_s, &ConfigureArguments::Options);
 
-  std::vector<std::string> unparsedArguments;
-  ConfigureArguments arguments = parser.Parse(args, &unparsedArguments);
-  return this->ExecuteHandlerCommand(arguments, unparsedArguments, status);
+  return this->Invoke(parser, args, status, [&](ConfigureArguments& a) {
+    return this->ExecuteHandlerCommand(a, status);
+  });
 }

Source/CTest/cmCTestCoverageCommand.cxx

@@ -50,7 +50,7 @@ bool cmCTestCoverageCommand::InitialPass(std::vector<std::string> const& args,
     cmArgumentParser<Args>{ MakeHandlerParser<Args>() } //
       .Bind("LABELS"_s, &CoverageArguments::Labels);
 
-  std::vector<std::string> unparsedArguments;
-  CoverageArguments arguments = parser.Parse(args, &unparsedArguments);
-  return this->ExecuteHandlerCommand(arguments, unparsedArguments, status);
+  return this->Invoke(parser, args, status, [&](CoverageArguments& a) {
+    return this->ExecuteHandlerCommand(a, status);
+  });
 }

Source/CTest/cmCTestHandlerCommand.cxx

@@ -12,7 +12,6 @@
 #include "cmCTestGenericHandler.h"
 #include "cmExecutionStatus.h"
 #include "cmMakefile.h"
-#include "cmMessageType.h"
 #include "cmStringAlgorithms.h"
 #include "cmSystemTools.h"
 #include "cmValue.h"
@@ -70,52 +69,55 @@ private:
 };
 }
 
-bool cmCTestHandlerCommand::ExecuteHandlerCommand(
-  HandlerArguments& args, std::vector<std::string> const& unparsedArguments,
-  cmExecutionStatus& status)
+bool cmCTestHandlerCommand::InvokeImpl(
+  BasicArguments& args, std::vector<std::string> const& unparsed,
+  cmExecutionStatus& status, std::function<bool()> handler)
 {
   // save error state and restore it if needed
   SaveRestoreErrorState errorState;
-
-  // Process input arguments.
-  this->CheckArguments(args);
-
-  std::sort(args.ParsedKeywords.begin(), args.ParsedKeywords.end());
-  auto it =
-    std::adjacent_find(args.ParsedKeywords.begin(), args.ParsedKeywords.end());
-  if (it != args.ParsedKeywords.end()) {
-    this->Makefile->IssueMessage(
-      MessageType::FATAL_ERROR,
-      cmStrCat("Called with more than one value for ", *it));
-  }
-
-  bool const foundBadArgument = !unparsedArguments.empty();
-  if (foundBadArgument) {
-    this->SetError(cmStrCat("called with unknown argument \"",
-                            unparsedArguments.front(), "\"."));
-  }
-  bool const captureCMakeError = !args.CaptureCMakeError.empty();
-  // now that arguments are parsed check to see if there is a
-  // CAPTURE_CMAKE_ERROR specified let the errorState object know.
-  if (captureCMakeError) {
+  if (!args.CaptureCMakeError.empty()) {
     errorState.CaptureCMakeError();
   }
-  // if we found a bad argument then exit before running command
-  if (foundBadArgument) {
-    // store the cmake error
-    if (captureCMakeError) {
-      this->Makefile->AddDefinition(args.CaptureCMakeError, "-1");
-      std::string const err = this->GetName() + " " + status.GetError();
-      if (!cmSystemTools::FindLastString(err.c_str(), "unknown error.")) {
-        cmCTestLog(this->CTest, ERROR_MESSAGE, err << " error from command\n");
-      }
-      // return success because failure is recorded in CAPTURE_CMAKE_ERROR
-      return true;
+
+  bool success = [&]() -> bool {
+    std::sort(args.ParsedKeywords.begin(), args.ParsedKeywords.end());
+    auto const it = std::adjacent_find(args.ParsedKeywords.begin(),
+                                       args.ParsedKeywords.end());
+    if (it != args.ParsedKeywords.end()) {
+      status.SetError(cmStrCat("called with more than one value for ", *it));
+      return false;
     }
-    // return failure because of bad argument
-    return false;
+
+    if (!unparsed.empty()) {
+      status.SetError(
+        cmStrCat("called with unknown argument \"", unparsed.front(), "\"."));
+      return false;
+    }
+
+    return handler();
+  }();
+
+  if (args.CaptureCMakeError.empty()) {
+    return success;
   }
 
+  if (!success) {
+    cmCTestLog(this->CTest, ERROR_MESSAGE,
+               this->GetName() << ' ' << status.GetError() << '\n');
+  }
+
+  cmMakefile& mf = status.GetMakefile();
+  success = success && !cmSystemTools::GetErrorOccurredFlag();
+  mf.AddDefinition(args.CaptureCMakeError, success ? "0" : "-1");
+  return true;
+}
+
+bool cmCTestHandlerCommand::ExecuteHandlerCommand(
+  HandlerArguments& args, cmExecutionStatus& /*status*/)
+{
+  // Process input arguments.
+  this->CheckArguments(args);
+
   // Set the config type of this ctest to the current value of the
   // CTEST_CONFIGURATION_TYPE script variable if it is defined.
   // The current script value trumps the -C argument on the command
@@ -165,14 +167,6 @@ bool cmCTestHandlerCommand::ExecuteHandlerCommand(
     cmCTestLog(this->CTest, ERROR_MESSAGE,
                "Cannot instantiate test handler " << this->GetName()
                                                   << std::endl);
-    if (captureCMakeError) {
-      this->Makefile->AddDefinition(args.CaptureCMakeError, "-1");
-      std::string const& err = status.GetError();
-      if (!cmSystemTools::FindLastString(err.c_str(), "unknown error.")) {
-        cmCTestLog(this->CTest, ERROR_MESSAGE, err << " error from command\n");
-      }
-      return true;
-    }
     return false;
   }
 
@@ -186,13 +180,6 @@ bool cmCTestHandlerCommand::ExecuteHandlerCommand(
     this->CTest->GetCTestConfiguration("BuildDirectory"));
   if (workdir.Failed()) {
     this->SetError(workdir.GetError());
-    if (captureCMakeError) {
-      this->Makefile->AddDefinition(args.CaptureCMakeError, "-1");
-      cmCTestLog(this->CTest, ERROR_MESSAGE,
-                 this->GetName() << " " << status.GetError() << "\n");
-      // return success because failure is recorded in CAPTURE_CMAKE_ERROR
-      return true;
-    }
     return false;
   }
 
@@ -205,21 +192,6 @@ bool cmCTestHandlerCommand::ExecuteHandlerCommand(
     this->Makefile->AddDefinition(args.ReturnValue, std::to_string(res));
   }
   this->ProcessAdditionalValues(handler.get(), args);
-  // log the error message if there was an error
-  if (captureCMakeError) {
-    const char* returnString = "0";
-    if (cmSystemTools::GetErrorOccurredFlag()) {
-      returnString = "-1";
-      std::string const& err = status.GetError();
-      // print out the error if it is not "unknown error" which means
-      // there was no message
-      if (!cmSystemTools::FindLastString(err.c_str(), "unknown error.")) {
-        cmCTestLog(this->CTest, ERROR_MESSAGE, err);
-      }
-    }
-    // store the captured cmake error state 0 or -1
-    this->Makefile->AddDefinition(args.CaptureCMakeError, returnString);
-  }
   return true;
 }
 

Source/CTest/cmCTestHandlerCommand.h

@@ -4,8 +4,10 @@
 
 #include "cmConfigure.h" // IWYU pragma: keep
 
+#include <functional>
 #include <memory>
 #include <string>
+#include <type_traits>
 #include <vector>
 
 #include <cm/string_view>
@@ -23,12 +25,25 @@ public:
   using cmCTestCommand::cmCTestCommand;
 
 protected:
-  struct HandlerArguments
+  struct BasicArguments
   {
+    std::string CaptureCMakeError;
     std::vector<cm::string_view> ParsedKeywords;
+  };
+
+  template <typename Args>
+  static auto MakeBasicParser() -> cmArgumentParser<Args>
+  {
+    static_assert(std::is_base_of<BasicArguments, Args>::value, "");
+    return cmArgumentParser<Args>{}
+      .Bind("CAPTURE_CMAKE_ERROR"_s, &BasicArguments::CaptureCMakeError)
+      .BindParsedKeywords(&BasicArguments::ParsedKeywords);
+  }
+
+  struct HandlerArguments : BasicArguments
+  {
     bool Append = false;
     bool Quiet = false;
-    std::string CaptureCMakeError;
     std::string ReturnValue;
     std::string Build;
     std::string Source;
@@ -38,11 +53,10 @@ protected:
   template <typename Args>
   static auto MakeHandlerParser() -> cmArgumentParser<Args>
   {
-    return cmArgumentParser<Args>{}
-      .BindParsedKeywords(&HandlerArguments::ParsedKeywords)
+    static_assert(std::is_base_of<HandlerArguments, Args>::value, "");
+    return cmArgumentParser<Args>{ MakeBasicParser<Args>() }
       .Bind("APPEND"_s, &HandlerArguments::Append)
       .Bind("QUIET"_s, &HandlerArguments::Quiet)
-      .Bind("CAPTURE_CMAKE_ERROR"_s, &HandlerArguments::CaptureCMakeError)
       .Bind("RETURN_VALUE"_s, &HandlerArguments::ReturnValue)
       .Bind("SOURCE"_s, &HandlerArguments::Source)
       .Bind("BUILD"_s, &HandlerArguments::Build)
@@ -50,11 +64,25 @@ protected:
   }
 
 protected:
+  template <typename Args, typename Handler>
+  bool Invoke(cmArgumentParser<Args> const& parser,
+              std::vector<std::string> const& arguments,
+              cmExecutionStatus& status, Handler handler)
+  {
+    std::vector<std::string> unparsed;
+    Args args = parser.Parse(arguments, &unparsed);
+    return this->InvokeImpl(args, unparsed, status,
+                            [&]() -> bool { return handler(args); });
+  };
+
   bool ExecuteHandlerCommand(HandlerArguments& args,
-                             std::vector<std::string> const& unparsedArguments,
                              cmExecutionStatus& status);
 
 private:
+  bool InvokeImpl(BasicArguments& args,
+                  std::vector<std::string> const& unparsed,
+                  cmExecutionStatus& status, std::function<bool()> handler);
+
   virtual std::string GetName() const = 0;
 
   virtual void CheckArguments(HandlerArguments& arguments);

Source/CTest/cmCTestMemCheckCommand.cxx

@@ -64,7 +64,7 @@ bool cmCTestMemCheckCommand::InitialPass(std::vector<std::string> const& args,
     cmArgumentParser<MemCheckArguments>{ MakeTestParser<MemCheckArguments>() }
       .Bind("DEFECT_COUNT"_s, &MemCheckArguments::DefectCount);
 
-  std::vector<std::string> unparsedArguments;
-  MemCheckArguments arguments = parser.Parse(args, &unparsedArguments);
-  return this->ExecuteHandlerCommand(arguments, unparsedArguments, status);
+  return this->Invoke(parser, args, status, [&](MemCheckArguments& a) {
+    return this->ExecuteHandlerCommand(a, status);
+  });
 }

Source/CTest/cmCTestSubmitCommand.cxx

@@ -207,10 +207,10 @@ bool cmCTestSubmitCommand::InitialPass(std::vector<std::string> const& args,
   bool const cdashUpload = !args.empty() && args[0] == "CDASH_UPLOAD";
   auto const& parser = cdashUpload ? uploadParser : partsParser;
 
-  std::vector<std::string> unparsedArguments;
-  SubmitArguments arguments = parser.Parse(args, &unparsedArguments);
-  arguments.CDashUpload = cdashUpload;
-  return this->ExecuteHandlerCommand(arguments, unparsedArguments, status);
+  return this->Invoke(parser, args, status, [&](SubmitArguments& a) -> bool {
+    a.CDashUpload = cdashUpload;
+    return this->ExecuteHandlerCommand(a, status);
+  });
 }
 
 void cmCTestSubmitCommand::CheckArguments(HandlerArguments& arguments)

Source/CTest/cmCTestTestCommand.cxx

@@ -158,7 +158,7 @@ bool cmCTestTestCommand::InitialPass(std::vector<std::string> const& args,
 {
   static auto const parser = MakeTestParser<TestArguments>();
 
-  std::vector<std::string> unparsedArguments;
-  TestArguments arguments = parser.Parse(args, &unparsedArguments);
-  return this->ExecuteHandlerCommand(arguments, unparsedArguments, status);
+  return this->Invoke(parser, args, status, [&](TestArguments& a) {
+    return this->ExecuteHandlerCommand(a, status);
+  });
 }

Source/CTest/cmCTestUpdateCommand.cxx

@@ -6,7 +6,6 @@
 
 #include <cm/memory>
 
-#include "cmArgumentParser.h"
 #include "cmCTest.h"
 #include "cmCTestGenericHandler.h"
 #include "cmCTestUpdateHandler.h"
@@ -103,7 +102,7 @@ bool cmCTestUpdateCommand::InitialPass(std::vector<std::string> const& args,
 {
   static auto const parser = MakeHandlerParser<HandlerArguments>();
 
-  std::vector<std::string> unparsedArguments;
-  HandlerArguments arguments = parser.Parse(args, &unparsedArguments);
-  return this->ExecuteHandlerCommand(arguments, unparsedArguments, status);
+  return this->Invoke(parser, args, status, [&](HandlerArguments& a) {
+    return this->ExecuteHandlerCommand(a, status);
+  });
 }

Source/CTest/cmCTestUploadCommand.cxx

@@ -61,7 +61,7 @@ bool cmCTestUploadCommand::InitialPass(std::vector<std::string> const& args,
       .Bind("FILES"_s, &UploadArguments::Files)
       .Bind("QUIET"_s, &UploadArguments::Quiet);
 
-  std::vector<std::string> unparsedArguments;
-  UploadArguments arguments = parser.Parse(args, &unparsedArguments);
-  return this->ExecuteHandlerCommand(arguments, unparsedArguments, status);
+  return this->Invoke(parser, args, status, [&](UploadArguments& a) {
+    return this->ExecuteHandlerCommand(a, status);
+  });
 }

Tests/RunCMake/ctest_submit/RepeatRETURN_VALUE-stderr.txt

@@ -1,2 +1,2 @@
 CMake Error at .*/Tests/RunCMake/ctest_submit/RepeatRETURN_VALUE/test.cmake:[0-9]+ \(ctest_submit\):
-  Called with more than one value for RETURN_VALUE
+  ctest_submit called with more than one value for RETURN_VALUE