From 404cddb7bbb0e05bba2f3023f0ee2fa302c35124 Mon Sep 17 00:00:00 2001 From: Craig Scott Date: Sun, 21 Feb 2021 17:40:21 +1100 Subject: ExternalProject: Fix misuse of IS_NEWER_THAN in timestamp checks When using a file system which only has second resolution timestamps, there is a reasonably high likelihood of timestamps being the same. The IS_NEWER_THAN test returns true when timestamps are the same, so don't redo downloads when they match exactly. --- Modules/ExternalProject/gitclone.cmake.in | 3 ++- Modules/ExternalProject/hgclone.cmake.in | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) 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: " -- cgit v0.12 From b0da6712439fe44d84a158dceb5f946cc30ef68a Mon Sep 17 00:00:00 2001 From: Craig Scott Date: Thu, 18 Feb 2021 12:31:48 +1100 Subject: FetchContent: Don't update timestamps if files don't change The refactoring in 17e5516e60 (FetchContent: Invoke steps directly and avoid a separate sub-build, 2021-01-29) uses a different way of writing out the step scripts and updating time stamps when steps are executed. That inadvertently always wrote out the scripts for custom commands, even when the contents didn't change. This caused their timestamp to always be updated, resulting in those steps always being seen as out-of-date and needing to be re-executed. The way timestamps were checked to determine whether to re-execute a step also did not adequately account for file systems which only have second-resolution timestamps. The IS_NEWER_THAN if condition also returns true when timestamps are the same, so one needs to use the negative form to get a true "is newer than" test. ExternalProject is not susceptible to this problem because it uses file(GENERATE) to write out the script files and that only updates the file's timestamp if the contents change. It also mostly leaves timestamp checking to the build tool. --- Modules/ExternalProject.cmake | 26 ++++++++++++--- Tests/RunCMake/FetchContent/RunCMakeTest.cmake | 30 +++++++++++++++++ Tests/RunCMake/FetchContent/TimeStamps-stdout.txt | 2 ++ Tests/RunCMake/FetchContent/TimeStamps.cmake | 14 ++++++++ .../FetchContent/TimeStampsRerun-check.cmake | 38 ++++++++++++++++++++++ Tests/RunCMake/FetchContent/TimeStampsRerun.cmake | 1 + 6 files changed, 107 insertions(+), 4 deletions(-) create mode 100644 Tests/RunCMake/FetchContent/TimeStamps-stdout.txt create mode 100644 Tests/RunCMake/FetchContent/TimeStamps.cmake create mode 100644 Tests/RunCMake/FetchContent/TimeStampsRerun-check.cmake create mode 100644 Tests/RunCMake/FetchContent/TimeStampsRerun.cmake 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/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) -- cgit v0.12 From d7c80305410b728fce426ff0891ac9a28c4168e5 Mon Sep 17 00:00:00 2001 From: Craig Scott Date: Sun, 21 Feb 2021 18:04:28 +1100 Subject: FortranCInterface: Fix misuse of IS_NEWER_THAN in timestamp check When using a file system which only has second resolution timestamps, there is a reasonably high likelihood of timestamps being the same. The IS_NEWER_THAN test returns true when timestamps are the same, so don't redo detection when they match exactly. --- Modules/FortranCInterface/Detect.cmake | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/Modules/FortranCInterface/Detect.cmake b/Modules/FortranCInterface/Detect.cmake index 998faf1..9e5726b 100644 --- a/Modules/FortranCInterface/Detect.cmake +++ b/Modules/FortranCInterface/Detect.cmake @@ -6,14 +6,17 @@ configure_file(${FortranCInterface_SOURCE_DIR}/Input.cmake.in # Detect the Fortran/C interface on the first run or when the # configuration changes. -if(${FortranCInterface_BINARY_DIR}/Input.cmake - IS_NEWER_THAN ${FortranCInterface_BINARY_DIR}/Output.cmake - OR ${FortranCInterface_SOURCE_DIR}/Output.cmake.in - IS_NEWER_THAN ${FortranCInterface_BINARY_DIR}/Output.cmake - OR ${FortranCInterface_SOURCE_DIR}/CMakeLists.txt - IS_NEWER_THAN ${FortranCInterface_BINARY_DIR}/Output.cmake - OR ${CMAKE_CURRENT_LIST_FILE} - IS_NEWER_THAN ${FortranCInterface_BINARY_DIR}/Output.cmake +if(NOT EXISTS ${FortranCInterface_BINARY_DIR}/Output.cmake + OR NOT EXISTS ${FortranCInterface_BINARY_DIR}/Input.cmake + OR NOT EXISTS ${FortranCInterface_BINARY_DIR}/Output.cmake.in + OR NOT ${FortranCInterface_BINARY_DIR}/Output.cmake + IS_NEWER_THAN ${FortranCInterface_BINARY_DIR}/Input.cmake + OR NOT ${FortranCInterface_SOURCE_DIR}/Output.cmake + IS_NEWER_THAN ${FortranCInterface_BINARY_DIR}/Output.cmake.in + OR NOT ${FortranCInterface_BINARY_DIR}/Output.cmake + IS_NEWER_THAN ${FortranCInterface_SOURCE_DIR}/CMakeLists.txt + OR NOT ${FortranCInterface_BINARY_DIR}/Output.cmake + IS_NEWER_THAN ${CMAKE_CURRENT_LIST_FILE} ) message(CHECK_START "Detecting Fortran/C Interface") else() -- cgit v0.12 From 28501fca94f5bb54807f82ec5eb4e1885d6c7b11 Mon Sep 17 00:00:00 2001 From: Craig Scott Date: Sun, 21 Feb 2021 18:37:31 +1100 Subject: PCH PDB: Fix misuse of IS_NEWER_THAN in timestamp check When using a file system which only has second resolution timestamps, there is a reasonably high likelihood of timestamps being the same. The IS_NEWER_THAN test returns true when timestamps are the same, so don't retry copying the PCH PDB file when they match exactly. --- Source/cmLocalGenerator.cxx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Source/cmLocalGenerator.cxx b/Source/cmLocalGenerator.cxx index 34b9649..fe31af1 100644 --- a/Source/cmLocalGenerator.cxx +++ b/Source/cmLocalGenerator.cxx @@ -2698,8 +2698,9 @@ void cmLocalGenerator::CopyPchCompilePdb( } file << "foreach(retry RANGE 1 30)\n"; - file << " if (EXISTS \"" << from_file << "\" AND \"" << from_file - << " \" IS_NEWER_THAN \"" << dest_file << "\")\n"; + file << " if (EXISTS \"" << from_file << "\" AND (NOT EXISTS \"" + << dest_file << "\" OR NOT \"" << dest_file << " \" IS_NEWER_THAN \"" + << from_file << "\"))\n"; file << " execute_process(COMMAND ${CMAKE_COMMAND} -E copy"; file << " \"" << from_file << "\"" << " \"" << to_dir << "\" RESULT_VARIABLE result " -- cgit v0.12 From 8c93e3232f0db066ce613317db940a631f7d0d5e Mon Sep 17 00:00:00 2001 From: Craig Scott Date: Sun, 21 Feb 2021 17:46:38 +1100 Subject: GoogleTest: Fix misuse of IS_NEWER_THAN in timestamp check When using a file system which only has second resolution timestamps, there is a reasonably high likelihood of timestamps being the same. The IS_NEWER_THAN test returns true when timestamps are the same, so don't redo test discovery when they match exactly. --- Modules/GoogleTest.cmake | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Modules/GoogleTest.cmake b/Modules/GoogleTest.cmake index 2ea9e74..80d8e23 100644 --- a/Modules/GoogleTest.cmake +++ b/Modules/GoogleTest.cmake @@ -504,7 +504,8 @@ function(gtest_discover_tests TARGET) string(CONCAT ctest_include_content "if(EXISTS \"$\")" "\n" - " if(\"$\" IS_NEWER_THAN \"${ctest_tests_file}\")" "\n" + " if(NOT EXISTS \"${ctest_tests_file}\" OR" "\n" + " NOT \"${ctest_tests_file}\" IS_NEWER_THAN \"$\")" "\n" " include(\"${_GOOGLETEST_DISCOVER_TESTS_SCRIPT}\")" "\n" " gtest_discover_tests_impl(" "\n" " TEST_EXECUTABLE" " [==[" "$" "]==]" "\n" -- cgit v0.12