instrumentation: Reset queries on repeat configure steps from CMake GUI

Prevent repeat configure calls from the CMake GUI or `ccmake` tool from
loading query duplicates, which caused many duplicate callbacks to be
invoked on each subsequent hook.

Also ensure that multiple subsequent configure steps invoked by CMake
GUI and `ccmake` each have an associated snippet file during generate.

Fixes: #27651
Co-Authored-By: Tyler Yankee <tyler.yankee@kitware.com>

Help/manual/cmake-instrumentation.7.rst

@@ -338,6 +338,15 @@ following:
 These files remain in the build tree until after `Indexing`_ occurs and any
 user-specified `Callbacks`_ are executed.
 
+.. note::
+
+  Configure and generate snippet files are not written by CMake until the
+  generate step is complete. When using :manual:`cmake-gui(1)` or
+  :manual:`ccmake(1)`, triggering only configure step(s) without generating the
+  project files will not generate any configure snippets. Once the generate
+  step is run, there will be one configure snippet for each time the configure
+  step was run.
+
 Snippet files have a filename with the syntax
 ``<role>-<hash>-<timestamp>.json`` and contain the following data:
 
@@ -433,7 +442,9 @@ Snippet files have a filename with the syntax
   ``cmakeContent``
     The path to a `v1 CMake Content File`_ located under ``data``, which
     contains information about the CMake configure and generate steps
-    responsible for generating the ``command`` in this snippet.
+    responsible for generating the ``command`` in this snippet. When using
+    :manual:`cmake-gui(1)` or :manual:`ccmake(1)`, this field may be ``null``
+    for all configure steps up to the most recent one before the generate step.
 
   ``showOnly``
     A boolean representing whether the

Source/cmInstrumentation.cxx

@@ -4,6 +4,7 @@
 #include <chrono>
 #include <ctime>
 #include <iomanip>
+#include <iterator>
 #include <set>
 #include <sstream>
 #include <stdexcept>
@@ -108,6 +109,7 @@ cmInstrumentation::cmInstrumentation(std::string const& binary_dir,
 
 void cmInstrumentation::LoadQueries()
 {
+  this->ResetQueries();
   auto const readJSONQueries = [this](std::string const& dir) {
     if (cmSystemTools::FileIsDirectory(dir) && this->ReadJSONQueries(dir)) {
       this->hasQuery = true;
@@ -120,6 +122,16 @@ void cmInstrumentation::LoadQueries()
   }
 }
 
+void cmInstrumentation::ResetQueries()
+{
+  this->hasQuery = false;
+  this->options.clear();
+  this->hooks.clear();
+  this->callbacks.clear();
+  this->queryFiles.clear();
+  this->errorMsg.clear();
+}
+
 void cmInstrumentation::CheckCDashVariable()
 {
   std::string envVal;
@@ -311,6 +323,7 @@ void cmInstrumentation::ClearGeneratedQueries()
   if (cmSystemTools::FileIsDirectory(dir)) {
     cmSystemTools::RemoveADirectory(dir);
   }
+  this->writtenJsonQueries = 0;
 }
 
 bool cmInstrumentation::HasQuery() const
@@ -730,15 +743,21 @@ int cmInstrumentation::InstrumentCommand(
 
   // Don't write configure snippet until generate time
   if (command_type == "configure") {
-    this->configureSnippetData = root;
-    this->configureSnippetName = file_name;
+    this->configureSnippetData[file_name] = root;
   } else {
     // Add reference to CMake content and write out configure snippet after
     // generate
     if (command_type == "generate") {
-      addCMakeContent(this->configureSnippetData);
-      this->WriteInstrumentationJson(this->configureSnippetData, "data",
-                                     this->configureSnippetName);
+      for (auto it = this->configureSnippetData.begin();
+           it != this->configureSnippetData.end(); ++it) {
+        if (std::next(it) != this->configureSnippetData.end()) {
+          it->second["cmakeContent"] = Json::nullValue;
+        } else {
+          addCMakeContent(it->second);
+        }
+        this->WriteInstrumentationJson(it->second, "data", it->first);
+      }
+      this->configureSnippetData.clear();
     }
     this->WriteInstrumentationJson(root, "data", file_name);
   }

Source/cmInstrumentation.h

@@ -111,6 +111,7 @@ private:
                               Json::Value const& snippetData);
   size_t AssignTargetToTraceThread(std::vector<uint64_t>& workers,
                                    uint64_t timeStart, uint64_t duration);
+  void ResetQueries();
   std::string binaryDir;
   std::string timingDirv1;
   std::string userTimingDirv1;
@@ -127,8 +128,7 @@ private:
   bool ranSystemChecks = false;
   bool ranOSCheck = false;
   Json::Value customContent = Json::objectValue;
-  Json::Value configureSnippetData;
-  std::string configureSnippetName;
+  std::map<std::string, Json::Value> configureSnippetData;
 #ifndef CMAKE_BOOTSTRAP
   std::unique_ptr<cmsys::SystemInformation> systemInformation;
   cmsys::SystemInformation& GetSystemInformation();

Source/cmake.cxx

@@ -2701,6 +2701,7 @@ int cmake::ActualConfigure()
     this->ConfigureLog = cm::make_unique<cmConfigureLog>(
       cmStrCat(this->GetHomeOutputDirectory(), "/CMakeFiles"_s),
       this->FileAPI->GetConfigureLogVersions());
+    this->Instrumentation->ClearGeneratedQueries();
     this->Instrumentation->CheckCDashVariable();
   }
 #endif
@@ -2968,7 +2969,6 @@ void cmake::InitializeInstrumentation()
     this->Instrumentation = cm::make_unique<cmInstrumentation>(
       this->State->GetBinaryDirectory(),
       cmInstrumentation::LoadQueriesAfter::No);
-    this->Instrumentation->ClearGeneratedQueries();
   }
 #endif
 }

Tests/CMakeGUI/CMakeGUITest.cmake

@@ -34,6 +34,18 @@ function(run_cmake_gui_test name)
   if(EXISTS "${_cmakepresets_in}")
     configure_file("${_cmakepresets_in}" "${_workdir}/src/CMakePresets.json" @ONLY)
   endif()
+  set(_instrumentation_files
+    "${_srcdir}/main.cxx"
+    "${_srcdir}/lib.h"
+    "${_srcdir}/lib.cxx"
+    "${_srcdir}/callback.cmake"
+  )
+  foreach(_instrumentation_file IN LISTS _instrumentation_files)
+    if(EXISTS "${_instrumentation_file}.in")
+      cmake_path(GET _instrumentation_file FILENAME _instrumentation_file_name)
+      configure_file("${_instrumentation_file}.in" "${_workdir}/src/${_instrumentation_file_name}" @ONLY)
+    endif()
+  endforeach()
   if(_rcgt_DO_CONFIGURE)
     if(NOT _rcgt_GENERATOR)
       set(_rcgt_GENERATOR "${CMakeGUITest_GENERATOR}")
@@ -170,3 +182,11 @@ run_cmake_gui_test(presetArg:noExist
     "--preset=noExist"
   )
 run_cmake_gui_test(changingPresets)
+
+if("${CMakeGUITest_GENERATOR}" MATCHES "Make|Ninja|FASTBuild")
+  run_cmake_gui_test(instrumentation)
+  set(instrumentation_build_dir
+    "${CMakeGUITest_BINARY_DIR}/instrumentation/build"
+    )
+  include(${CMAKE_CURRENT_LIST_DIR}/instrumentation/check-data-dir.cmake)
+endif()

Tests/CMakeGUI/CMakeGUITest.cxx

@@ -79,6 +79,28 @@ void CMakeGUITest::tryConfigure(int expectedResult, int timeout)
   QCOMPARE(configureDoneSignalArguments.at(0).toInt(), expectedResult);
 }
 
+void CMakeGUITest::tryGenerate(int expectedResult, int timeout)
+{
+  auto* cmake = this->m_window->findChild<QCMakeThread*>()->cmakeInstance();
+
+  CatchShow catchMessages;
+  catchMessages.setCallback<QMessageBox>([](QMessageBox* box) {
+    if (box->text().contains("Error in generation process")) {
+      box->accept();
+    }
+  });
+
+  QSignalSpy generateDoneSpy(cmake, &QCMake::generateDone);
+  QVERIFY(generateDoneSpy.isValid());
+  QMetaObject::invokeMethod(
+    this->m_window, [this]() { this->m_window->GenerateButton->click(); },
+    Qt::QueuedConnection);
+  QVERIFY(generateDoneSpy.wait(timeout));
+
+  QList<QVariant> generateDoneSignalArguments = generateDoneSpy.takeFirst();
+  QCOMPARE(generateDoneSignalArguments.at(0).toInt(), expectedResult);
+}
+
 void CMakeGUITest::sourceBinaryArgs()
 {
   QFETCH(QString, sourceDir);
@@ -151,6 +173,21 @@ void CMakeGUITest::simpleConfigure_data()
                         << -1;
 }
 
+void CMakeGUITest::instrumentation()
+{
+  this->m_window->SourceDirectory->setText(CMakeGUITest_BINARY_DIR
+                                           "/instrumentation/src");
+  this->m_window->BinaryDirectory->setCurrentText(CMakeGUITest_BINARY_DIR
+                                                  "/instrumentation/build");
+
+  // Wait a bit for everything to update
+  loopSleep();
+
+  this->tryConfigure();
+  this->tryConfigure();
+  this->tryGenerate();
+}
+
 void CMakeGUITest::environment()
 {
   auto* cmake = this->m_window->findChild<QCMakeThread*>()->cmakeInstance();

Tests/CMakeGUI/CMakeGUITest.h

@@ -16,12 +16,14 @@ private:
   CMakeSetupDialog* m_window = nullptr;
 
   void tryConfigure(int expectedResult = 0, int timeout = 60000);
+  void tryGenerate(int expectedResult = 0, int timeout = 60000);
 
 private slots:
   void sourceBinaryArgs();
   void sourceBinaryArgs_data();
   void simpleConfigure();
   void simpleConfigure_data();
+  void instrumentation();
   void environment();
   void presetArg();
   void presetArg_data();

Tests/CMakeGUI/instrumentation/CMakeLists.txt.in

@@ -0,0 +1,12 @@
+cmake_minimum_required(VERSION 4.3)
+project(instrumentation)
+
+add_executable(main main.cxx)
+add_library(lib lib.cxx)
+target_link_libraries(main lib)
+
+cmake_instrumentation(
+  API_VERSION 1
+  DATA_VERSION 1
+  CALLBACK ${CMAKE_COMMAND} -P ${CMAKE_CURRENT_LIST_DIR}/callback.cmake
+)

Tests/CMakeGUI/instrumentation/callback.cmake.in

@@ -0,0 +1,2 @@
+file(TOUCH "instrumentation/build/callback-count.txt")
+file(APPEND "instrumentation/build/callback-count.txt" " \n")

Tests/CMakeGUI/instrumentation/check-data-dir.cmake

@@ -0,0 +1,57 @@
+set(instrumentation_v1 "${instrumentation_build_dir}/.cmake/instrumentation/v1")
+
+file(GLOB_RECURSE queries LIST_DIRECTORIES false ${instrumentation_v1}/query/*)
+list(LENGTH queries n_queries)
+
+if (NOT n_queries EQUAL 1)
+  message(FATAL_ERROR "Expected one instrumentation query, got: ${n_queries}")
+endif()
+
+file(GLOB snippets_configure ${instrumentation_v1}/data/configure-*)
+list(LENGTH snippets_configure n_snippets_configure)
+
+if (NOT n_snippets_configure EQUAL 2)
+  message(FATAL_ERROR "Expected two configure snippets, got: ${n_snippets_configure}")
+endif()
+
+file(GLOB snippets_generate ${instrumentation_v1}/data/generate-*)
+list(LENGTH snippets_generate n_snippets_generate)
+
+if (NOT n_snippets_generate EQUAL 1)
+  message(FATAL_ERROR "Expected one generate snippet, got: ${n_snippets_generate}")
+endif()
+
+file(GLOB content ${instrumentation_v1}/data/content/*)
+list(LENGTH content n_content)
+if (NOT n_content EQUAL 1)
+  message(FATAL_ERROR "Expected one content file, got: ${n_content}")
+endif()
+
+# The earliest configure snippet should not have an associated content file,
+# because we immediately re-configured, so the corresponding generation never
+# occurred.
+list(GET snippets_configure 0 first_configure)
+file(READ "${first_configure}" configure_json)
+string(JSON configure_content_file
+  ERROR_VARIABLE configure_content_error
+  GET "${configure_json}" "cmakeContent")
+if (configure_content_error)
+  message(FATAL_ERROR "Failed to get cmakeContent from ${first_configure}: ${configure_content_error}")
+endif()
+if (NOT configure_content_file STREQUAL "")
+  message(FATAL_ERROR "Expected null for cmakeContent in ${first_configure}, got: ${configure_content_file}")
+endif()
+
+execute_process(
+  COMMAND
+    "${CMAKE_CTEST_COMMAND}" --collect-instrumentation
+      "${instrumentation_build_dir}"
+)
+if (NOT EXISTS "${instrumentation_build_dir}/callback-count.txt")
+  message(FATAL_ERROR "Expected a callback output file at: ${instrumentation_build_dir}/callback-count.txt")
+endif()
+file(STRINGS "${instrumentation_build_dir}/callback-count.txt" callback_lines)
+list(LENGTH callback_lines n_callbacks)
+if (NOT n_callbacks EQUAL 1)
+  message(FATAL_ERROR "Expected one callback execution, got: ${n_callbacks}")
+endif()

Tests/CMakeGUI/instrumentation/lib.cxx.in

@@ -0,0 +1,4 @@
+int lib()
+{
+  return 0;
+}

Tests/CMakeGUI/instrumentation/lib.h.in

@@ -0,0 +1 @@
+int lib();

Tests/CMakeGUI/instrumentation/main.cxx.in

@@ -0,0 +1,5 @@
+#include "lib.h"
+int main()
+{
+  return lib();
+}