Merge topic 'instrumentation-build-lock'

a249e820a8 instrumentation: Add file lock for build daemon
9b65be6da5 instrumentation: Don't load query files before configure

Acked-by: Kitware Robot <[email protected]>
Acked-by: buildbot <[email protected]>
Merge-request: !11025

Help/manual/cmake-instrumentation.7.rst

@@ -198,7 +198,8 @@ key is required, but all other fields are optional.
   * ``postTest``
 
   ``preBuild`` and ``postBuild`` are not supported with the
-  :generator:`MSYS Makefiles` generator.
+  :generator:`MSYS Makefiles` generator. Additionally, they will not be
+  triggered when the build tool is invoked by ``cmake --build``.
 
 ``options``
   A list of strings used to enable certain optional behavior, including the

Source/cmInstrumentation.cxx

@@ -19,6 +19,8 @@
 
 #include "cmCryptoHash.h"
 #include "cmExperimental.h"
+#include "cmFileLock.h"
+#include "cmFileLockResult.h"
 #include "cmInstrumentationQuery.h"
 #include "cmJSONState.h"
 #include "cmStringAlgorithms.h"
@@ -626,7 +628,11 @@ int cmInstrumentation::SpawnBuildDaemon()
   uv_disable_stdio_inheritance();
 
   // preBuild Hook
-  this->CollectTimingData(cmInstrumentationQuery::Hook::PreBuild);
+  if (this->LockBuildDaemon()) {
+    // Release lock before spawning the build daemon, to prevent blocking it.
+    this->lock.Release();
+    this->CollectTimingData(cmInstrumentationQuery::Hook::PreBuild);
+  }
 
   // postBuild Hook
   if (this->HasHook(cmInstrumentationQuery::Hook::PostBuild)) {
@@ -645,6 +651,16 @@ int cmInstrumentation::SpawnBuildDaemon()
   return 0;
 }
 
+// Prevent multiple build daemons from running simultaneously
+bool cmInstrumentation::LockBuildDaemon()
+{
+  std::string const lockFile = cmStrCat(this->timingDirv1, "/.build.lock");
+  if (!cmSystemTools::FileExists(lockFile)) {
+    cmSystemTools::Touch(lockFile, true);
+  }
+  return this->lock.Lock(lockFile, 0).IsOk();
+}
+
 /*
  * Always called by ctest --wait-and-collect-instrumentation in a detached
  * process. Waits for the given PID to end before running the postBuild hook.
@@ -653,6 +669,11 @@ int cmInstrumentation::SpawnBuildDaemon()
  */
 int cmInstrumentation::CollectTimingAfterBuild(int ppid)
 {
+  // Check if another process is already instrumenting the build.
+  // This lock will be released when the process exits at the end of the build.
+  if (!this->LockBuildDaemon()) {
+    return 0;
+  }
   std::function<int()> waitForBuild = [ppid]() -> int {
     while (0 == uv_kill(ppid, 0)) {
       cmSystemTools::Delay(100);

Source/cmInstrumentation.h

@@ -15,6 +15,8 @@
 #include <cm/optional>
 
 #include <cm3p/json/value.h>
+
+#include "cmFileLock.h"
 #ifndef CMAKE_BOOTSTRAP
 #  include <cmsys/SystemInformation.hxx>
 #endif
@@ -61,6 +63,7 @@ public:
   void ClearGeneratedQueries();
   int CollectTimingData(cmInstrumentationQuery::Hook hook);
   int SpawnBuildDaemon();
+  bool LockBuildDaemon();
   int CollectTimingAfterBuild(int ppid);
   void AddHook(cmInstrumentationQuery::Hook hook);
   void AddOption(cmInstrumentationQuery::Option option);
@@ -103,4 +106,5 @@ private:
   cmsys::SystemInformation& GetSystemInformation();
 #endif
   int writtenJsonQueries = 0;
+  cmFileLock lock;
 };

Source/cmake.cxx

@@ -2664,7 +2664,6 @@ int cmake::ActualConfigure()
     this->ConfigureLog = cm::make_unique<cmConfigureLog>(
       cmStrCat(this->GetHomeOutputDirectory(), "/CMakeFiles"_s),
       this->FileAPI->GetConfigureLogVersions());
-    this->Instrumentation->LoadQueries();
     this->Instrumentation->CheckCDashVariable();
   }
 #endif
@@ -3109,10 +3108,6 @@ int cmake::Generate()
         << ms.count() / 1000.0L << "s)";
     this->UpdateProgress(msg.str(), -1);
   }
-#if !defined(CMAKE_BOOTSTRAP)
-  this->Instrumentation->CollectTimingData(
-    cmInstrumentationQuery::Hook::PostGenerate);
-#endif
   if (!this->GraphVizFile.empty()) {
     std::cout << "Generate graphviz: " << this->GraphVizFile << '\n';
     this->GenerateGraphViz(this->GraphVizFile);
@@ -3132,6 +3127,8 @@ int cmake::Generate()
   this->SaveCache(this->GetHomeOutputDirectory());
 
 #if !defined(CMAKE_BOOTSTRAP)
+  this->Instrumentation->CollectTimingData(
+    cmInstrumentationQuery::Hook::PostGenerate);
   this->GlobalGenerator->WriteInstallJson();
   this->FileAPI->WriteReplies(cmFileAPI::IndexFor::Success);
 #endif
@@ -4044,6 +4041,9 @@ int cmake::Build(int jobs, std::string dir, std::vector<std::string> targets,
   };
 
 #if !defined(CMAKE_BOOTSTRAP)
+  // Block the instrumentation build daemon from spawning during this build.
+  // This lock will be released when the process exits at the end of the build.
+  instrumentation.LockBuildDaemon();
   int buildresult =
     instrumentation.InstrumentCommand("cmakeBuild", args, doBuild);
   instrumentation.CollectTimingData(

Tests/RunCMake/Instrumentation/RunCMakeTest.cmake

@@ -67,6 +67,8 @@ function(instrument test)
   endif()
   if (ARGS_BUILD_MAKE_PROGRAM)
     set(RunCMake_TEST_OUTPUT_MERGE 1)
+    # Force reconfigure to test for double preBuild & postBuild hooks
+    file(TOUCH ${RunCMake_TEST_BINARY_DIR}/CMakeCache.txt)
     run_cmake_command(${test}-make-program ${RunCMake_MAKE_PROGRAM})
     unset(RunCMake_TEST_OUTPUT_MERGE)
   endif()
@@ -128,6 +130,10 @@ instrument(cmake-command-resets-generated NO_WARN
   COPY_QUERIES_GENERATED
   CHECK_SCRIPT check-data-dir.cmake
 )
+instrument(cmake-command-cmake-build NO_WARN
+  BUILD
+  CHECK_SCRIPT check-no-make-program-hooks.cmake
+)
 
 if(RunCMake_GENERATOR STREQUAL "MSYS Makefiles")
   # FIXME(#27079): This does not work for MSYS Makefiles.

Tests/RunCMake/Instrumentation/check-no-make-program-hooks.cmake

@@ -0,0 +1,6 @@
+if (EXISTS ${v1}/preBuild.hook OR EXISTS ${v1}/postBuild.hook)
+  set(RunCMake_TEST_FAILED "Got unexpected preBuild/postBuild hooks.")
+endif()
+if (NOT EXISTS ${v1}/postCMakeBuild.hook)
+  set(RunCMake_TEST_FAILED "Missing expected postCMakeBuild hook.")
+endif()

Tests/RunCMake/Instrumentation/hook.cmake

@@ -76,6 +76,9 @@ endif()
 
 get_filename_component(dataDir ${index} DIRECTORY)
 get_filename_component(v1 ${dataDir} DIRECTORY)
+if (EXISTS ${v1}/${hook}.hook)
+  add_error("Received multiple triggers of the same hook: ${hook}")
+endif()
 file(WRITE ${v1}/${hook}.hook "${ERROR_MESSAGE}")
 
 if (NOT ERROR_MESSAGE MATCHES "^$")

Tests/RunCMake/Instrumentation/query/cmake-command-cmake-build.cmake

@@ -0,0 +1,7 @@
+file(TO_CMAKE_PATH "${CMAKE_SOURCE_DIR}/../hook.cmake" hook_path)
+cmake_instrumentation(
+  API_VERSION 1
+  DATA_VERSION 1
+  HOOKS preBuild postBuild postCMakeBuild
+  CALLBACK ${CMAKE_COMMAND} -P ${hook_path} 0
+)