Merge topic 'fix-IS_NEWER_THAN-usage' into release-3.20

b0da671243 FetchContent: Don't update timestamps if files don't change
404cddb7bb ExternalProject: Fix misuse of IS_NEWER_THAN in timestamp checks

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

8 files changed, 111 insertions, 6 deletions

diff --git a/Modules/ExternalProject.cmake b/Modules/ExternalProject.cmake
index 4a9809b..987b69a 100644
--- a/Modules/ExternalProject.cmake
+++ b/Modules/ExternalProject.cmake
@@ -2579,11 +2579,24 @@ function(_ep_write_command_script
   endif()
 
   if(genex_supported)
-    # Only written at generation phase
+    # Only written at generation phase. This will only change the file's
+    # timestamp if the contents change.
     file(GENERATE OUTPUT "${script_filename}" CONTENT "${script_content}")
   else()
-    # Written immediately, needed if script has to be invoked in configure phase
-    file(WRITE "${script_filename}" "${script_content}")
+    # Update the file immediately, needed if script has to be invoked in the
+    # configure phase (e.g. via FetchContent). We need to be careful to avoid
+    # updating the timestamp if the file contents don't change. The file(WRITE)
+    # command always updates the file, so avoid it if we don't need to call it.
+    set(doWrite TRUE)
+    if(EXISTS "${script_filename}")
+      file(READ "${script_filename}" existing_content)
+      if(existing_content STREQUAL script_content)
+        set(doWrite FALSE)
+      endif()
+    endif()
+    if(doWrite)
+      file(WRITE "${script_filename}" "${script_content}")
+    endif()
   endif()
 
 endfunction()
@@ -3916,7 +3929,12 @@ function(_ep_do_preconfigure_steps_now name)
 
     if(NOT need_to_run)
       foreach(dep_file ${script_file} ${_EPdepends_${STEP}})
-        if(NOT EXISTS ${dep_file} OR ${dep_file} IS_NEWER_THAN ${stamp_file})
+        # IS_NEWER_THAN is also true if the timestamps are the same. On some
+        # file systems, we only have second resolution timestamps and the
+        # likelihood of having the same timestamp is high. Use the negative
+        # form to ensure we actually get a true "is newer than" test.
+        if(NOT EXISTS ${dep_file} OR
+           NOT ${stamp_file} IS_NEWER_THAN ${dep_file})
           set(need_to_run TRUE)
           break()
         endif()
diff --git a/Modules/ExternalProject/gitclone.cmake.in b/Modules/ExternalProject/gitclone.cmake.in
index a2e900c..edbdd72 100644
--- a/Modules/ExternalProject/gitclone.cmake.in
+++ b/Modules/ExternalProject/gitclone.cmake.in
@@ -7,7 +7,8 @@ set(quiet "@quiet@")
 set(script_dir "@CMAKE_CURRENT_FUNCTION_LIST_DIR@/ExternalProject")
 include(${script_dir}/captured_process_setup.cmake)
 
-if(NOT "@gitclone_infofile@" IS_NEWER_THAN "@gitclone_stampfile@")
+if(EXISTS "@gitclone_stampfile@" AND EXISTS "@gitclone_infofile@" AND
+   "@gitclone_stampfile@" IS_NEWER_THAN "@gitclone_infofile@")
   if(NOT quiet)
     message(STATUS
       "Avoiding repeated git clone, stamp file is up to date: "
diff --git a/Modules/ExternalProject/hgclone.cmake.in b/Modules/ExternalProject/hgclone.cmake.in
index 5561955..9a574d2 100644
--- a/Modules/ExternalProject/hgclone.cmake.in
+++ b/Modules/ExternalProject/hgclone.cmake.in
@@ -7,7 +7,8 @@ set(quiet "@quiet@")
 set(script_dir "@CMAKE_CURRENT_FUNCTION_LIST_DIR@/ExternalProject")
 include(${script_dir}/captured_process_setup.cmake)
 
-if(NOT "@hgclone_infofile@" IS_NEWER_THAN "@hgclone_stampfile@")
+if(EXISTS "@hgclone_stampfile@" AND EXISTS "@hgclone_infofile@" AND
+   "@hgclone_stampfile@" IS_NEWER_THAN "@hgclone_infofile@")
   if(NOT quiet)
     message(STATUS
       "Avoiding repeated hg clone, stamp file is up to date: "
diff --git a/Tests/RunCMake/FetchContent/RunCMakeTest.cmake b/Tests/RunCMake/FetchContent/RunCMakeTest.cmake
index 13013fa..d7fd009 100644
--- a/Tests/RunCMake/FetchContent/RunCMakeTest.cmake
+++ b/Tests/RunCMake/FetchContent/RunCMakeTest.cmake
@@ -27,6 +27,36 @@ run_cmake_with_options(ManualSourceDirectoryRelative
   -D "FETCHCONTENT_SOURCE_DIR_WITHPROJECT:STRING=WithProject"
 )
 
+function(run_FetchContent_TimeStamps)
+  set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/TimeStamps)
+  file(REMOVE_RECURSE "${RunCMake_TEST_BINARY_DIR}")
+  file(MAKE_DIRECTORY "${RunCMake_TEST_BINARY_DIR}")
+
+  # First run should execute the commands
+  run_cmake(TimeStamps)
+
+  # Ensure that the file checks we use in the TimeStampsRerun-check.cmake script
+  # will not be defeated by file systems with only one second resolution.
+  # The IS_NEWER_THAN check returns TRUE if the timestamps of the two files are
+  # the same, which has been observed where filesystems only have one second
+  # resolution.
+  set(cmpTimeStamp   ${RunCMake_TEST_BINARY_DIR}/cmpTimeStamp.txt)
+  set(checkTimeStamp ${RunCMake_TEST_BINARY_DIR}/cmpTimeStampCheck.txt)
+  file(TOUCH ${cmpTimeStamp})
+  file(TOUCH ${checkTimeStamp})
+  if("${cmpTimeStamp}" IS_NEWER_THAN "${checkTimeStamp}")
+    execute_process(
+      COMMAND ${CMAKE_COMMAND} -E sleep 1.125
+      COMMAND_ERROR_IS_FATAL LAST
+    )
+  endif()
+
+  # Run again with no changes, no commands should re-execute
+  set(RunCMake_TEST_NO_CLEAN 1)
+  run_cmake(TimeStampsRerun)
+endfunction()
+run_FetchContent_TimeStamps()
+
 function(run_FetchContent_DirOverrides)
   set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/DirOverrides-build)
   file(REMOVE_RECURSE "${RunCMake_TEST_BINARY_DIR}")
diff --git a/Tests/RunCMake/FetchContent/TimeStamps-stdout.txt b/Tests/RunCMake/FetchContent/TimeStamps-stdout.txt
new file mode 100644
index 0000000..2ba1ff4
--- /dev/null
+++ b/Tests/RunCMake/FetchContent/TimeStamps-stdout.txt
@@ -0,0 +1,2 @@
+.* *download executed
+.* *patch executed
diff --git a/Tests/RunCMake/FetchContent/TimeStamps.cmake b/Tests/RunCMake/FetchContent/TimeStamps.cmake
new file mode 100644
index 0000000..33874f9
--- /dev/null
+++ b/Tests/RunCMake/FetchContent/TimeStamps.cmake
@@ -0,0 +1,14 @@
+include(FetchContent)
+
+# Do nothing for an update because it would result in always re-running the
+# patch step. We want to test that a patch step that only depends on the
+# download step is not re-run unnecessarily.
+FetchContent_Declare(customCommands
+  PREFIX            ${CMAKE_CURRENT_BINARY_DIR}
+  DOWNLOAD_COMMAND  "${CMAKE_COMMAND}" -E echo "download executed"
+  UPDATE_COMMAND    ""
+  PATCH_COMMAND     "${CMAKE_COMMAND}" -E echo "patch executed"
+)
+
+set(FETCHCONTENT_QUIET FALSE)
+FetchContent_MakeAvailable(customCommands)
diff --git a/Tests/RunCMake/FetchContent/TimeStampsRerun-check.cmake b/Tests/RunCMake/FetchContent/TimeStampsRerun-check.cmake
new file mode 100644
index 0000000..c12a5f4
--- /dev/null
+++ b/Tests/RunCMake/FetchContent/TimeStampsRerun-check.cmake
@@ -0,0 +1,38 @@
+set(cmpFile   ${RunCMake_TEST_BINARY_DIR}/cmpTimeStamp.txt)
+set(scriptDir ${RunCMake_TEST_BINARY_DIR}/tmp)
+set(stampDir  ${RunCMake_TEST_BINARY_DIR}/src/customcommands-stamp)
+
+set(errorMessages)
+if(NOT EXISTS "${cmpFile}")
+  list(APPEND errorMessages "  ${cmpFile} is missing")
+else()
+  foreach(script IN ITEMS mkdirs download patch)
+    set(scriptFile "${scriptDir}/customcommands-${script}.cmake")
+    if(NOT EXISTS "${scriptFile}")
+      list(APPEND errorMessages "  ${scriptFile} is missing")
+    elseif(NOT "${cmpFile}" IS_NEWER_THAN "${scriptFile}")
+      list(APPEND errorMessages "  ${scriptFile} was unexectedly updated")
+    endif()
+  endforeach()
+
+  # special case, not a script, has different extension
+  set(repoInfoFile "${scriptDir}/customcommands-download-repoinfo.txt")
+  if(NOT EXISTS "${repoInfoFile}")
+    list(APPEND errorMessages "  ${repoInfoFile} is missing")
+  elseif(NOT "${cmpFile}" IS_NEWER_THAN "${repoInfoFile}")
+    list(APPEND errorMessages "  ${repoInfoFile} was unexectedly updated")
+  endif()
+
+  foreach(step IN ITEMS download patch)
+    set(stampFile "${stampDir}/customcommands-${step}")
+    if(NOT EXISTS "${stampFile}")
+      list(APPEND errorMessages "  ${stampFile} is missing")
+    elseif(NOT "${cmpFile}" IS_NEWER_THAN "${stampFile}")
+      list(APPEND errorMessages "  ${stampFile} was unexectedly updated")
+    endif()
+  endforeach()
+endif()
+
+if(errorMessages)
+  list(JOIN errorMessages "\n" RunCMake_TEST_FAILED)
+endif()
diff --git a/Tests/RunCMake/FetchContent/TimeStampsRerun.cmake b/Tests/RunCMake/FetchContent/TimeStampsRerun.cmake
new file mode 100644
index 0000000..e13667a
--- /dev/null
+++ b/Tests/RunCMake/FetchContent/TimeStampsRerun.cmake
@@ -0,0 +1 @@
+include(${CMAKE_CURRENT_LIST_DIR}/TimeStamps.cmake)