Merge topic 'instrumentation-content-lifetime'

b8014633dc Experimental: Update the Instrumentation UUID
ba3c278da2 instrumentation: Don't remove content files older than an index
4683db44a1 instrumentation: Write index files to data/index/ subdirectory

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

Help/dev/experimental.rst

@@ -127,15 +127,15 @@ In order to activate support for the :command:`cmake_instrumentation` command,
 set
 
 * variable ``CMAKE_EXPERIMENTAL_INSTRUMENTATION`` to
-* value ``2555e60c-9119-46fb-af73-8c54e9b6b326``.
+* value ``ec7aa2dc-b87f-45a3-8022-fe01c5f59984``.
 
 To enable instrumentation at the user-level, files should be placed under
 either
-``<CMAKE_CONFIG_DIR>/instrumentation-2555e60c-9119-46fb-af73-8c54e9b6b326`` or
-``<CMAKE_BINARY_DIR>/.cmake/instrumentation-2555e60c-9119-46fb-af73-8c54e9b6b326``.
+``<CMAKE_CONFIG_DIR>/instrumentation-ec7aa2dc-b87f-45a3-8022-fe01c5f59984`` or
+``<CMAKE_BINARY_DIR>/.cmake/instrumentation-ec7aa2dc-b87f-45a3-8022-fe01c5f59984``.
 
 To include instrumentation data in CTest XML files (for submission to CDash),
 you need to set the following environment variables:
 
 * ``CTEST_USE_INSTRUMENTATION=1``
-* ``CTEST_EXPERIMENTAL_INSTRUMENTATION=2555e60c-9119-46fb-af73-8c54e9b6b326``
+* ``CTEST_EXPERIMENTAL_INSTRUMENTATION=ec7aa2dc-b87f-45a3-8022-fe01c5f59984``

Help/manual/cmake-instrumentation.7.rst

@@ -158,6 +158,10 @@ subdirectories:
   files, they should never be removed by other processes. Data collected here
   remains until after `Indexing`_ occurs and all `Callbacks`_ are executed.
 
+``data/index/``
+  A subset of the collected data, containing any
+  `v1 Index Files <v1 Index File_>`_.
+
 ``data/content/``
   A subset of the collected data, containing any
   :ref:`cmake_instrumentation Configure Content` files.
@@ -276,8 +280,8 @@ Example:
 
 In this example, after every ``cmake --build`` or ``cmake --install``
 invocation, an index file ``index-<timestamp>.json`` will be generated in
-``<build>/.cmake/instrumentation/v1/data`` containing a list of data snippet
-files created since the previous indexing. The commands
+``<build>/.cmake/instrumentation/v1/data/index`` containing a list of data
+snippet files created since the previous indexing. The commands
 ``/usr/bin/python callback.py index-<timestamp>.json`` and
 ``/usr/bin/cmake -P callback.cmake arg index-<timestamp>.json`` will be executed
 in that order. The index file will contain the ``staticSystemInformation`` data

Source/cmExperimental.cxx

@@ -65,7 +65,7 @@ cmExperimental::FeatureData const LookupTable[] = {
     cmExperimental::TryCompileCondition::Never },
   // Instrumentation
   { "Instrumentation",
-    "2555e60c-9119-46fb-af73-8c54e9b6b326",
+    "ec7aa2dc-b87f-45a3-8022-fe01c5f59984",
     "CMAKE_EXPERIMENTAL_INSTRUMENTATION",
     "CMake's support for collecting instrumentation data is experimental. It "
     "is meant only for experimentation and feedback to CMake developers.",

Source/cmInstrumentation.cxx

@@ -229,46 +229,51 @@ void cmInstrumentation::WriteCustomContent()
   }
 }
 
-std::string cmInstrumentation::GetLatestFile(std::string const& dataSubdir)
+std::string cmInstrumentation::GetFileByTimestamp(
+  cmInstrumentation::LatestOrOldest order, std::string const& dataSubdir,
+  std::string const& exclude)
 {
   std::string fullDir = cmStrCat(this->timingDirv1, "/data/", dataSubdir);
-  std::string latestFile;
+  std::string result;
   if (cmSystemTools::FileExists(fullDir)) {
     cmsys::Directory d;
     if (d.Load(fullDir)) {
       for (unsigned int i = 0; i < d.GetNumberOfFiles(); i++) {
         std::string fname = d.GetFileName(i);
-        if (fname != "." && fname != ".." && fname > latestFile) {
-          latestFile = fname;
+        if (fname != "." && fname != ".." && fname != exclude &&
+            (result.empty() ||
+             (order == LatestOrOldest::Latest && fname > result) ||
+             (order == LatestOrOldest::Oldest && fname < result))) {
+          result = fname;
         }
       }
     }
   }
-  return latestFile;
+  return result;
 }
 
 void cmInstrumentation::RemoveOldFiles(std::string const& dataSubdir)
 {
   std::string const dataSubdirPath =
     cmStrCat(this->timingDirv1, "/data/", dataSubdir);
+  std::string oldIndex =
+    this->GetFileByTimestamp(LatestOrOldest::Oldest, "index");
+  if (!oldIndex.empty()) {
+    oldIndex = cmStrCat(this->timingDirv1, "/data/index/", oldIndex);
+  }
   if (cmSystemTools::FileExists(dataSubdirPath)) {
-    std::string latestFile = this->GetLatestFile(dataSubdir);
+    std::string latestFile =
+      this->GetFileByTimestamp(LatestOrOldest::Latest, dataSubdir);
     cmsys::Directory d;
     if (d.Load(dataSubdirPath)) {
       for (unsigned int i = 0; i < d.GetNumberOfFiles(); i++) {
         std::string fname = d.GetFileName(i);
         std::string fpath = d.GetFilePath(i);
         if (fname != "." && fname != ".." && fname < latestFile) {
-          if (dataSubdir == "trace") {
-            // Check if this trace file shares a name with any existing index
-            // files, in which case it is listed by that index file and a
-            // callback is running, so we shouldn't delete it yet.
-            std::string index = "index-";
-            std::string json = ".json";
-            std::string timestamp = fname.substr(
-              index.size(), fname.size() - index.size() - json.size() - 1);
-            if (cmSystemTools::FileExists(cmStrCat(
-                  this->timingDirv1, "/data/index-", timestamp, ".json"))) {
+          if (!oldIndex.empty()) {
+            int compare;
+            cmSystemTools::FileTimeCompare(oldIndex, fpath, &compare);
+            if (compare == 1) {
               continue;
             }
           }
@@ -319,33 +324,22 @@ int cmInstrumentation::CollectTimingData(cmInstrumentationQuery::Hook hook)
   std::string const& directory = cmStrCat(this->timingDirv1, "/data");
   std::string suffix_time = ComputeSuffixTime();
   std::string const& index_name = cmStrCat("index-", suffix_time, ".json");
-  std::string index_path = cmStrCat(directory, '/', index_name);
+  std::string index_path = cmStrCat(directory, "/index/", index_name);
   cmSystemTools::Touch(index_path, true);
 
   // Gather Snippets
   using snippet = std::pair<std::string, std::string>;
   std::vector<snippet> files;
   cmsys::Directory d;
-  std::string last_index;
+  std::string last_index_name =
+    this->GetFileByTimestamp(LatestOrOldest::Latest, "index", index_name);
   if (d.Load(directory)) {
     for (unsigned int i = 0; i < d.GetNumberOfFiles(); i++) {
       std::string fpath = d.GetFilePath(i);
       std::string fname = d.GetFile(i);
-      if (fname.rfind('.', 0) == 0 || fname == index_name ||
-          d.FileIsDirectory(i)) {
+      if (fname.rfind('.', 0) == 0 || d.FileIsDirectory(i)) {
         continue;
       }
-      if (fname.rfind("index-", 0) == 0) {
-        if (last_index.empty()) {
-          last_index = fpath;
-        } else {
-          int compare;
-          cmSystemTools::FileTimeCompare(fpath, last_index, &compare);
-          if (compare == 1) {
-            last_index = fpath;
-          }
-        }
-      }
       files.push_back(snippet(std::move(fname), std::move(fpath)));
     }
   }
@@ -362,11 +356,13 @@ int cmInstrumentation::CollectTimingData(cmInstrumentationQuery::Hook hook)
     this->InsertStaticSystemInformation(index);
   }
   for (auto const& file : files) {
-    if (last_index.empty()) {
+    if (last_index_name.empty()) {
       index["snippets"].append(file.first);
     } else {
       int compare;
-      cmSystemTools::FileTimeCompare(file.second, last_index, &compare);
+      std::string last_index_path =
+        cmStrCat(directory, "/index/", last_index_name);
+      cmSystemTools::FileTimeCompare(file.second, last_index_path, &compare);
       if (compare == 1) {
         index["snippets"].append(file.first);
       }
@@ -381,7 +377,7 @@ int cmInstrumentation::CollectTimingData(cmInstrumentationQuery::Hook hook)
   }
 
   // Write index file
-  this->WriteInstrumentationJson(index, "data", index_name);
+  this->WriteInstrumentationJson(index, "data/index", index_name);
 
   // Execute callbacks
   for (auto& cb : this->callbacks) {
@@ -670,7 +666,8 @@ int cmInstrumentation::InstrumentCommand(
   root["workingDir"] = cmSystemTools::GetLogicalWorkingDirectory();
 
   // Add custom configure content
-  std::string contentFile = this->GetLatestFile("content");
+  std::string contentFile =
+    this->GetFileByTimestamp(LatestOrOldest::Latest, "content");
   if (!contentFile.empty()) {
     root["configureContent"] = cmStrCat("content/", contentFile);
   }

Source/cmInstrumentation.h

@@ -63,7 +63,6 @@ public:
                       std::vector<std::vector<std::string>> const& callback);
   void AddCustomContent(std::string const& name, Json::Value const& contents);
   void WriteCustomContent();
-  std::string GetLatestFile(std::string const& dataSubdir);
   void ClearGeneratedQueries();
   int CollectTimingData(cmInstrumentationQuery::Hook hook);
   int SpawnBuildDaemon();
@@ -99,6 +98,14 @@ private:
                         Json::Value const& snippetData);
   size_t AssignTargetToTraceThread(std::vector<uint64_t>& workers,
                                    uint64_t timeStart, uint64_t duration);
+  enum LatestOrOldest
+  {
+    Latest,
+    Oldest
+  };
+  std::string GetFileByTimestamp(LatestOrOldest latestOrOldest,
+                                 std::string const& dataSubdir,
+                                 std::string const& exclude = "");
   std::string binaryDir;
   std::string timingDirv1;
   std::string userTimingDirv1;

Tests/RunCMake/ConfigDir/check-reply.cmake

@@ -1,6 +1,6 @@
 if (NOT EXISTS ${RunCMake_TEST_BINARY_DIR}/.cmake/api/v1/reply)
   set(RunCMake_TEST_FAILED "Failed to read FileAPI query from user config directory")
 endif()
-if (NOT EXISTS ${RunCMake_TEST_BINARY_DIR}/.cmake/instrumentation-2555e60c-9119-46fb-af73-8c54e9b6b326/v1/data)
+if (NOT EXISTS ${RunCMake_TEST_BINARY_DIR}/.cmake/instrumentation-ec7aa2dc-b87f-45a3-8022-fe01c5f59984/v1/data)
   set(RunCMake_TEST_FAILED "Failed to read Instrumentation query from user config directory")
 endif()

Tests/RunCMake/Instrumentation/RunCMakeTest.cmake

@@ -23,8 +23,9 @@ function(instrument test)
   )
   cmake_parse_arguments(ARGS "${OPTIONS}" "CHECK_SCRIPT;CONFIGURE_ARG" "" ${ARGN})
   set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/${test})
-  set(uuid "2555e60c-9119-46fb-af73-8c54e9b6b326")
+  set(uuid "ec7aa2dc-b87f-45a3-8022-fe01c5f59984")
   set(v1 ${RunCMake_TEST_BINARY_DIR}/.cmake/instrumentation-${uuid}/v1)
+  set(v1 ${v1} PARENT_SCOPE)
   set(query_dir ${CMAKE_CURRENT_LIST_DIR}/query)
 
   # Clear previous instrumentation data
@@ -177,6 +178,19 @@ instrument(cmake-command-custom-content
   CONFIGURE_ARG "-DN=2"
   CHECK_SCRIPT check-custom-content.cmake
 )
+set(indexDir ${v1}/data/index)
+set(fakeIndex ${indexDir}/index-0.json)
+file(MAKE_DIRECTORY ${indexDir})
+file(TOUCH ${fakeIndex})
+# fakeIndex newer than all content files prevents their deletion
+set(EXPECTED_CONTENT_FILES 2)
+instrument(cmake-command-custom-content
+  NO_WARN NO_CONFIGURE MANUAL_HOOK PRESERVE_DATA
+  CHECK_SCRIPT check-custom-content-removed.cmake
+)
+file(REMOVE ${fakeIndex})
+# old content files will be removed if no index file exists
+set(EXPECTED_CONTENT_FILES 1)
 instrument(cmake-command-custom-content
   NO_WARN NO_CONFIGURE MANUAL_HOOK PRESERVE_DATA
   CHECK_SCRIPT check-custom-content-removed.cmake

Tests/RunCMake/Instrumentation/check-custom-content-removed.cmake

@@ -6,6 +6,6 @@ endif()
 
 file(GLOB content_files ${v1}/data/content/*)
 list(LENGTH content_files num)
-if (NOT ${num} EQUAL 1)
-  add_error("Found ${num} custom content files, expected 1.")
+if (NOT ${num} EQUAL ${EXPECTED_CONTENT_FILES})
+  add_error("Found ${num} custom content files, expected ${EXPECTED_CONTENT_FILES}.")
 endif()

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

@@ -15,7 +15,7 @@ macro(hasPostBuildArtifacts)
     set(postBuildRan 1)
   endif()
   if (NOT dataDirClean)
-    file(GLOB data "${v1}/data/*")
+    file(GLOB data "${v1}/data/*json")
     if ("${data}" STREQUAL "")
       set(dataDirClean 1)
     endif()

Tests/RunCMake/Instrumentation/hook.cmake

@@ -110,7 +110,6 @@ if (NOT hasStaticInfo STREQUAL UNEXPECTED)
   json_has_key("${index}" "${staticSystemInformation}" vendorString ${hasStaticInfo})
 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}")

Tests/RunCMake/Instrumentation/project/CMakeLists.txt

@@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.30)
 project(instrumentation)
 enable_testing()
 if (EXISTS ${INSTRUMENT_COMMAND_FILE})
-  set(CMAKE_EXPERIMENTAL_INSTRUMENTATION "2555e60c-9119-46fb-af73-8c54e9b6b326")
+  set(CMAKE_EXPERIMENTAL_INSTRUMENTATION "ec7aa2dc-b87f-45a3-8022-fe01c5f59984")
   include(${INSTRUMENT_COMMAND_FILE})
 endif()
 

Tests/RunCMake/ctest_instrumentation/CMakeLists.txt.in

@@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.10)
 @CASE_CMAKELISTS_PREFIX_CODE@
 project(CTestInstrumentation@CASE_NAME@)
 if(USE_INSTRUMENTATION)
-  set(CMAKE_EXPERIMENTAL_INSTRUMENTATION "2555e60c-9119-46fb-af73-8c54e9b6b326")
+  set(CMAKE_EXPERIMENTAL_INSTRUMENTATION "ec7aa2dc-b87f-45a3-8022-fe01c5f59984")
 endif()
 include(CTest)
 add_executable(main main.c)

Tests/RunCMake/ctest_instrumentation/InstrumentationInCTestXML-check.cmake

@@ -1,4 +1,4 @@
-set(timingDir "${RunCMake_TEST_BINARY_DIR}/.cmake/instrumentation-2555e60c-9119-46fb-af73-8c54e9b6b326/v1")
+set(timingDir "${RunCMake_TEST_BINARY_DIR}/.cmake/instrumentation-ec7aa2dc-b87f-45a3-8022-fe01c5f59984/v1")
 file(READ "${timingDir}/query/generated/query-0.json" jsonData)
 string(JSON options GET "${jsonData}" options)
 if (options MATCHES cdashVerbose AND NOT ${RunCMake_USE_VERBOSE_INSTRUMENTATION})

Tests/RunCMake/ctest_instrumentation/RunCMakeTest.cmake

@@ -10,7 +10,7 @@ function(run_InstrumentationInCTestXML CASE_NAME)
     set(RunCMake_USE_VERBOSE_INSTRUMENTATION FALSE)
   endif()
   if(ARGS_USE_INSTRUMENTATION_ENV_VARS)
-    set(ENV{CTEST_EXPERIMENTAL_INSTRUMENTATION} "2555e60c-9119-46fb-af73-8c54e9b6b326")
+    set(ENV{CTEST_EXPERIMENTAL_INSTRUMENTATION} "ec7aa2dc-b87f-45a3-8022-fe01c5f59984")
     set(ENV{CTEST_USE_INSTRUMENTATION} "1")
     set(RunCMake_USE_INSTRUMENTATION TRUE)
   else()

Tests/RunCMake/ctest_labels_for_subprojects/RunCMakeTest.cmake

@@ -38,7 +38,7 @@ file(COPY "${CTEST_RUNCMAKE_SOURCE_DIRECTORY}/MyThirdPartyDependency"
   if(USE_INSTRUMENTATION)
     set(CASE_CMAKELISTS_SUFFIX_CODE [[
   add_subdirectory(MyThirdPartyDependency)
-  set(CMAKE_EXPERIMENTAL_INSTRUMENTATION "2555e60c-9119-46fb-af73-8c54e9b6b326")
+  set(CMAKE_EXPERIMENTAL_INSTRUMENTATION "ec7aa2dc-b87f-45a3-8022-fe01c5f59984")
   cmake_instrumentation(DATA_VERSION 1 API_VERSION 1)
     ]])
     set(RunCMake-check-file CTestScriptVariableCommandLine-check.cmake)