Merge topic 'instrumentation-windows'

f7756853c5 instrumentation: Exclude (pre|post)Build hooks with MSYS Makefiles
455944508e instrumentation: Remove ninja-specific references
7dbe092d77 instrumentation: Support preBuild and postBuild hooks on Windows
6d6c8303f9 Tests: Teach RunCMake.Instrumentation to tolerate make tool output

Acked-by: Kitware Robot <kwrobot@kitware.com>
Merge-request: !10932

Help/manual/cmake-instrumentation.7.rst

@@ -189,13 +189,16 @@ key is required, but all other fields are optional.
   should be one of the following:
 
   * ``postGenerate``
-  * ``preBuild`` (called when ``ninja``  or ``make`` is invoked; unavailable on Windows)
-  * ``postBuild`` (called when ``ninja`` or ``make`` completes; unavailable on Windows)
+  * ``preBuild`` (called when ``ninja``  or ``make`` is invoked)
+  * ``postBuild`` (called when ``ninja`` or ``make`` completes)
   * ``preCMakeBuild`` (called when ``cmake --build`` is invoked)
   * ``postCMakeBuild`` (called when ``cmake --build`` completes)
   * ``postInstall``
   * ``postTest``
 
+  ``preBuild`` and ``postBuild`` are not supported with the
+  :generator:`MSYS Makefiles` generator.
+
 ``options``
   A list of strings used to enable certain optional behavior, including the
   collection of certain additional data. Elements in this list should be one of

Source/cmGlobalNinjaGenerator.cxx

@@ -1772,8 +1772,7 @@ void cmGlobalNinjaGenerator::WriteBuiltinTargets(std::ostream& os)
   this->WriteTargetRebuildManifest(os);
   this->WriteTargetClean(os);
   this->WriteTargetHelp(os);
-#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32)
-  // FIXME(#26668) This does not work on Windows
+#ifndef CMAKE_BOOTSTRAP
   if (this->GetCMakeInstance()
         ->GetInstrumentation()
         ->HasPreOrPostBuildHook()) {
@@ -1854,8 +1853,7 @@ void cmGlobalNinjaGenerator::WriteTargetRebuildManifest(std::ostream& os)
   }
   reBuild.ImplicitDeps.push_back(this->CMakeCacheFile);
 
-#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32)
-  // FIXME(#26668) This does not work on Windows
+#ifndef CMAKE_BOOTSTRAP
   if (this->GetCMakeInstance()
         ->GetInstrumentation()
         ->HasPreOrPostBuildHook()) {
@@ -2208,8 +2206,7 @@ void cmGlobalNinjaGenerator::WriteTargetHelp(std::ostream& os)
   }
 }
 
-#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32)
-// FIXME(#26668) This does not work on Windows
+#ifndef CMAKE_BOOTSTRAP
 void cmGlobalNinjaGenerator::WriteTargetInstrument(std::ostream& os)
 {
   // Write rule
@@ -2218,11 +2215,13 @@ void cmGlobalNinjaGenerator::WriteTargetInstrument(std::ostream& os)
     rule.Command = cmStrCat(
       '"', cmSystemTools::GetCTestCommand(), "\" --start-instrumentation \"",
       this->GetCMakeInstance()->GetHomeOutputDirectory(), '"');
+#  ifndef _WIN32
     /*
      * On Unix systems, Ninja will prefix the command with `/bin/sh -c`.
      * Use exec so that Ninja is the parent process of the command.
      */
     rule.Command = cmStrCat("exec ", rule.Command);
+#  endif
     rule.Description = "Collecting build metrics";
     rule.Comment = "Rule to initialize instrumentation daemon.";
     rule.Restat = "1";

Source/cmGlobalNinjaGenerator.h

@@ -535,8 +535,7 @@ private:
   void WriteTargetRebuildManifest(std::ostream& os);
   bool WriteTargetCleanAdditional(std::ostream& os);
   void WriteTargetClean(std::ostream& os);
-#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32)
-  // FIXME(#26668) This does not work on Windows
+#ifndef CMAKE_BOOTSTRAP
   void WriteTargetInstrument(std::ostream& os);
 #endif
   void WriteTargetHelp(std::ostream& os);

Source/cmInstrumentation.cxx

@@ -612,27 +612,31 @@ std::string cmInstrumentation::ComputeSuffixTime()
 }
 
 /*
- * Called by ctest --start-instrumentation as part of the START_INSTRUMENTATION
- * rule when using the Ninja generator.
- * This creates a detached process which waits for the Ninja process to die
- * before running the postBuild hook. In this way, the postBuild hook triggers
- * after every ninja invocation, regardless of whether the build passed or
- * failed.
+ * Called by ctest --start-instrumentation.
+ *
+ * This creates a detached process which waits for the parent process (i.e.,
+ * the build system) to die before running the postBuild hook. In this way, the
+ * postBuild hook triggers after every invocation of the build system,
+ * regardless of whether the build passed or failed.
  */
 int cmInstrumentation::SpawnBuildDaemon()
 {
+  // Do not inherit handles from the parent process, so that the daemon is
+  // fully detached. This helps prevent deadlock between the two.
+  uv_disable_stdio_inheritance();
+
   // preBuild Hook
   this->CollectTimingData(cmInstrumentationQuery::Hook::PreBuild);
 
   // postBuild Hook
   if (this->HasHook(cmInstrumentationQuery::Hook::PostBuild)) {
-    auto ninja_pid = uv_os_getppid();
-    if (ninja_pid) {
+    auto ppid = uv_os_getppid();
+    if (ppid) {
       std::vector<std::string> args;
       args.push_back(cmSystemTools::GetCTestCommand());
       args.push_back("--wait-and-collect-instrumentation");
       args.push_back(this->binaryDir);
-      args.push_back(std::to_string(ninja_pid));
+      args.push_back(std::to_string(ppid));
       auto builder = cmUVProcessChainBuilder().SetDetached().AddCommand(args);
       auto chain = builder.Start();
       uv_run(&chain.GetLoop(), UV_RUN_DEFAULT);

Source/cmLocalUnixMakefileGenerator3.cxx

@@ -74,21 +74,22 @@ std::string cmSplitExtension(std::string const& in, std::string& base)
   return ext;
 }
 
-#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32)
+#ifndef CMAKE_BOOTSTRAP
 // Helper function to add the Start Instrumentation command
 void addInstrumentationCommand(cmInstrumentation* instrumentation,
                                std::vector<std::string>& commands)
 {
-  // FIXME(#26668) This does not work on Windows
   if (instrumentation->HasPreOrPostBuildHook()) {
     std::string instrumentationCommand =
       "$(CTEST_COMMAND) --start-instrumentation $(CMAKE_BINARY_DIR)";
+#  ifndef _WIN32
     /*
      * On Unix systems, Make will prefix the command with `/bin/sh -c`.
      * Use exec so that Make is the parent process of the command.
      * Add a `;` to convince BSD make to not optimize out the shell.
      */
     instrumentationCommand = cmStrCat("exec ", instrumentationCommand, " ;");
+#  endif
     commands.push_back(instrumentationCommand);
   }
 }
@@ -687,11 +688,12 @@ void cmLocalUnixMakefileGenerator3::WriteMakeVariables(
                     "CMAKE_COMMAND = "
                  << cmakeShellCommand << "\n";
 
-#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32)
-  // FIXME(#26668) This does not work on Windows
+#ifndef CMAKE_BOOTSTRAP
   if (this->GetCMakeInstance()
         ->GetInstrumentation()
-        ->HasPreOrPostBuildHook()) {
+        ->HasPreOrPostBuildHook() &&
+      // FIXME(#27079): This does not work for MSYS Makefiles.
+      this->GlobalGenerator->GetName() != "MSYS Makefiles") {
     std::string ctestShellCommand =
       getShellCommand(cmSystemTools::GetCTestCommand());
     makefileStream << "# The CTest executable.\n"
@@ -849,9 +851,12 @@ void cmLocalUnixMakefileGenerator3::WriteSpecialTargetsBottom(
 
     std::vector<std::string> no_depends;
     commands.push_back(std::move(runRule));
-#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32)
-    addInstrumentationCommand(this->GetCMakeInstance()->GetInstrumentation(),
-                              commands);
+#ifndef CMAKE_BOOTSTRAP
+    // FIXME(#27079): This does not work for MSYS Makefiles.
+    if (this->GlobalGenerator->GetName() != "MSYS Makefiles") {
+      addInstrumentationCommand(this->GetCMakeInstance()->GetInstrumentation(),
+                                commands);
+    }
 #endif
     if (!this->IsRootMakefile()) {
       this->CreateCDCommand(commands, this->GetBinaryDirectory(),
@@ -1855,9 +1860,12 @@ void cmLocalUnixMakefileGenerator3::WriteLocalAllRules(
         this->ConvertToOutputFormat(cmakefileName, cmOutputConverter::SHELL),
         " 1");
       commands.push_back(std::move(runRule));
-#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32)
-      addInstrumentationCommand(this->GetCMakeInstance()->GetInstrumentation(),
-                                commands);
+#ifndef CMAKE_BOOTSTRAP
+      // FIXME(#27079): This does not work for MSYS Makefiles.
+      if (this->GlobalGenerator->GetName() != "MSYS Makefiles") {
+        addInstrumentationCommand(
+          this->GetCMakeInstance()->GetInstrumentation(), commands);
+      }
 #endif
     }
     this->CreateCDCommand(commands, this->GetBinaryDirectory(),

Tests/RunCMake/Instrumentation/RunCMakeTest.cmake

@@ -66,7 +66,9 @@ function(instrument test)
     run_cmake_command(${test}-build ${CMAKE_COMMAND} --build . --config Debug)
   endif()
   if (ARGS_BUILD_MAKE_PROGRAM)
+    set(RunCMake_TEST_OUTPUT_MERGE 1)
     run_cmake_command(${test}-make-program ${RunCMake_MAKE_PROGRAM})
+    unset(RunCMake_TEST_OUTPUT_MERGE)
   endif()
   if (ARGS_INSTALL)
     run_cmake_command(${test}-install ${CMAKE_COMMAND} --install . --prefix install --config Debug)
@@ -127,8 +129,27 @@ instrument(cmake-command-resets-generated NO_WARN
   CHECK_SCRIPT check-data-dir.cmake
 )
 
-# FIXME(#26668) This does not work on Windows
-if (UNIX)
+if(RunCMake_GENERATOR STREQUAL "MSYS Makefiles")
+  # FIXME(#27079): This does not work for MSYS Makefiles.
+  set(Skip_BUILD_MAKE_PROGRAM_Case 1)
+elseif(RunCMake_GENERATOR STREQUAL "NMake Makefiles")
+ execute_process(
+   COMMAND "${RunCMake_MAKE_PROGRAM}" -?
+   OUTPUT_VARIABLE nmake_out
+   ERROR_VARIABLE nmake_out
+   RESULT_VARIABLE nmake_res
+   OUTPUT_STRIP_TRAILING_WHITESPACE
+   )
+   if(nmake_res EQUAL 0 AND nmake_out MATCHES "Program Maintenance Utility[^\n]+Version ([1-9][0-9.]+)")
+     set(nmake_version "${CMAKE_MATCH_1}")
+   else()
+     message(FATAL_ERROR "'nmake -?' reported:\n${nmake_out}")
+   endif()
+   if(nmake_version VERSION_LESS 9)
+     set(Skip_BUILD_MAKE_PROGRAM_Case 1)
+   endif()
+endif()
+if(NOT Skip_BUILD_MAKE_PROGRAM_Case)
   instrument(cmake-command-make-program NO_WARN
     BUILD_MAKE_PROGRAM
     CHECK_SCRIPT check-make-program-hooks.cmake)