Browse Source

instrumentation: Support preBuild and postBuild hooks on Windows

* Use `uv_disable_stdio_inheritance` to resolve the deadlock between the
parent build system process and `ctest
--wait-and-collect-instrumentation` on Windows.
* Remove Windows gating from preBuild and postBuild indexing and update
tests and documentation accordingly.

Fixes: #26668
Tyler Yankee 3 months ago
parent
commit
7dbe092d77

+ 2 - 2
Help/manual/cmake-instrumentation.7.rst

@@ -189,8 +189,8 @@ 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``

+ 5 - 6
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";

+ 1 - 2
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);

+ 4 - 0
Source/cmInstrumentation.cxx

@@ -621,6 +621,10 @@ std::string cmInstrumentation::ComputeSuffixTime()
  */
 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);
 

+ 6 - 6
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,8 +688,7 @@ 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()) {
@@ -849,7 +849,7 @@ void cmLocalUnixMakefileGenerator3::WriteSpecialTargetsBottom(
 
     std::vector<std::string> no_depends;
     commands.push_back(std::move(runRule));
-#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32)
+#ifndef CMAKE_BOOTSTRAP
     addInstrumentationCommand(this->GetCMakeInstance()->GetInstrumentation(),
                               commands);
 #endif
@@ -1855,7 +1855,7 @@ void cmLocalUnixMakefileGenerator3::WriteLocalAllRules(
         this->ConvertToOutputFormat(cmakefileName, cmOutputConverter::SHELL),
         " 1");
       commands.push_back(std::move(runRule));
-#if !defined(CMAKE_BOOTSTRAP) && !defined(_WIN32)
+#ifndef CMAKE_BOOTSTRAP
       addInstrumentationCommand(this->GetCMakeInstance()->GetInstrumentation(),
                                 commands);
 #endif

+ 18 - 2
Tests/RunCMake/Instrumentation/RunCMakeTest.cmake

@@ -129,8 +129,24 @@ 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 "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)