Merge topic 'ep-update-disconnected'

8cc45e150a ExternalProject: Make hg download method respect UPDATE_DISCONNECTED
1512dc43cb ExternalProject: Avoid reconfigure when updates are disconnected
1d29cf37a1 Tests: Reduce warning noise in ExternalProjectUpdate test

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

Help/release/dev/ep-update-disconnected.rst

@@ -0,0 +1,14 @@
+ep-update-disconnected
+----------------------
+
+* The ``update`` and ``patch`` steps of an :module:`ExternalProject` will now
+  always re-execute if any of their details change, even if
+  ``UPDATE_DISCONNECTED`` was set to true in the call to
+  :command:`ExternalProject_Add`. If using the GIT download method and the
+  ``GIT_TAG`` is changed and the new ``GIT_TAG`` isn't already known locally,
+  this is now a fatal error instead of silently using the previous ``GIT_TAG``.
+
+* When ``UPDATE_DISCONNECTED`` is set to true in a call to
+  :command:`ExternalProject_Add`, the ``configure`` step will no longer
+  re-run on every build. It will only re-run if details of the ``download``,
+  ``update`` or ``patch`` step change.

Modules/ExternalProject.cmake

@@ -448,13 +448,23 @@ External Project Definition
     ``UPDATE_DISCONNECTED <bool>``
       .. versionadded:: 3.2
 
-      When enabled, this option causes the update step to be skipped. It does
-      not, however, prevent the download step. The update step can still be
+      When enabled, this option causes the update step to be skipped (but see
+      below for changed behavior where this is not the case). It does not
+      prevent the download step. The update step can still be
       added as a step target (see :command:`ExternalProject_Add_StepTargets`)
       and called manually. This is useful if you want to allow developers to
       build the project when disconnected from the network (the network may
       still be needed for the download step though).
 
+      .. versionchanged:: 3.27
+
+        When ``UPDATE_DISCONNECTED`` is true, the update step will be executed
+        if any details about the update or download step are changed.
+        Furthermore, if using the git download/update method, the update
+        logic will be modified to skip attempts to contact the remote.
+        If the ``GIT_TAG`` mentions a ref that is not known locally, the
+        update step will halt with a fatal error.
+
       When this option is present, it is generally advisable to make the value
       a cache variable under the developer's control rather than hard-coding
       it. If this option is not present, the default value is taken from the
@@ -3216,7 +3226,7 @@ function(_ep_get_update_disconnected var name)
 endfunction()
 
 function(_ep_add_update_command name)
-  ExternalProject_Get_Property(${name} source_dir tmp_dir)
+  ExternalProject_Get_Property(${name} source_dir stamp_dir tmp_dir)
 
   get_property(cmd_set TARGET ${name} PROPERTY _EP_UPDATE_COMMAND SET)
   get_property(cmd TARGET ${name} PROPERTY _EP_UPDATE_COMMAND)
@@ -3230,6 +3240,7 @@ function(_ep_add_update_command name)
   set(work_dir)
   set(comment)
   set(always)
+  set(file_deps)
 
   if(cmd_set)
     set(work_dir ${source_dir})
@@ -3291,6 +3302,7 @@ function(_ep_add_update_command name)
     endif()
     set(work_dir ${source_dir})
     set(comment "Performing update step for '${name}'")
+    set(comment_disconnected "Performing disconnected update step for '${name}'")
 
     get_property(git_tag
       TARGET ${name}
@@ -3344,8 +3356,10 @@ function(_ep_add_update_command name)
 
     _ep_get_git_submodules_recurse(git_submodules_recurse)
 
+    set(update_script "${tmp_dir}/${name}-gitupdate.cmake")
+    list(APPEND file_deps ${update_script})
     _ep_write_gitupdate_script(
-      "${tmp_dir}/${name}-gitupdate.cmake"
+      "${update_script}"
       "${GIT_EXECUTABLE}"
       "${git_tag}"
       "${git_remote_name}"
@@ -3356,7 +3370,8 @@ function(_ep_add_update_command name)
       "${work_dir}"
       "${git_update_strategy}"
     )
-    set(cmd ${CMAKE_COMMAND} -P ${tmp_dir}/${name}-gitupdate.cmake)
+    set(cmd              ${CMAKE_COMMAND} -Dcan_fetch=YES -P ${update_script})
+    set(cmd_disconnected ${CMAKE_COMMAND} -Dcan_fetch=NO  -P ${update_script})
     set(always 1)
   elseif(hg_repository)
     if(NOT HG_EXECUTABLE)
@@ -3364,6 +3379,7 @@ function(_ep_add_update_command name)
     endif()
     set(work_dir ${source_dir})
     set(comment "Performing update step (hg pull) for '${name}'")
+    set(comment_disconnected "Performing disconnected update step for '${name}'")
 
     get_property(hg_tag
       TARGET ${name}
@@ -3389,9 +3405,23 @@ Update to Mercurial >= 2.1.1.
       ${HG_EXECUTABLE} pull
       COMMAND ${HG_EXECUTABLE} update ${hg_tag}
     )
+    set(cmd_disconnected ${HG_EXECUTABLE} update ${hg_tag})
     set(always 1)
   endif()
 
+  # We use configure_file() to write the update_info_file so that the file's
+  # timestamp is not updated if we don't change the contents
+  if(NOT DEFINED cmd_disconnected)
+    set(cmd_disconnected "${cmd}")
+  endif()
+  set(update_info_file ${stamp_dir}/${name}-update-info.txt)
+  list(APPEND file_deps ${update_info_file})
+  configure_file(
+    "${CMAKE_CURRENT_FUNCTION_LIST_DIR}/ExternalProject/UpdateInfo.txt.in"
+    "${update_info_file}"
+    @ONLY
+  )
+
   get_property(log
     TARGET ${name}
     PROPERTY _EP_LOG_UPDATE
@@ -3425,16 +3455,39 @@ Update to Mercurial >= 2.1.1.
       EXCLUDE_FROM_MAIN \${update_disconnected}
       WORKING_DIRECTORY \${work_dir}
       DEPENDEES download
+      DEPENDS \${file_deps}
       ${log}
       ${uses_terminal}
     )"
   )
+  if(update_disconnected)
+    if(NOT DEFINED comment_disconnected)
+      set(comment_disconnected "${comment}")
+    endif()
+    set(__cmdQuoted)
+    foreach(__item IN LISTS cmd_disconnected)
+      string(APPEND __cmdQuoted " [==[${__item}]==]")
+    endforeach()
+
+    cmake_language(EVAL CODE "
+      ExternalProject_Add_Step(${name} update_disconnected
+        INDEPENDENT TRUE
+        COMMENT \${comment_disconnected}
+        COMMAND ${__cmdQuoted}
+        WORKING_DIRECTORY \${work_dir}
+        DEPENDEES download
+        DEPENDS \${file_deps}
+        ${log}
+        ${uses_terminal}
+      )"
+    )
+  endif()
 
 endfunction()
 
 
 function(_ep_add_patch_command name)
-  ExternalProject_Get_Property(${name} source_dir)
+  ExternalProject_Get_Property(${name} source_dir stamp_dir)
 
   get_property(cmd_set TARGET ${name} PROPERTY _EP_PATCH_COMMAND SET)
   get_property(cmd TARGET ${name} PROPERTY _EP_PATCH_COMMAND)
@@ -3445,6 +3498,15 @@ function(_ep_add_patch_command name)
     set(work_dir ${source_dir})
   endif()
 
+  # We use configure_file() to write the patch_info_file so that the file's
+  # timestamp is not updated if we don't change the contents
+  set(patch_info_file ${stamp_dir}/${name}-patch-info.txt)
+  configure_file(
+    "${CMAKE_CURRENT_FUNCTION_LIST_DIR}/ExternalProject/PatchInfo.txt.in"
+    "${patch_info_file}"
+    @ONLY
+  )
+
   get_property(log
     TARGET ${name}
     PROPERTY _EP_LOG_PATCH
@@ -3466,11 +3528,6 @@ function(_ep_add_patch_command name)
   endif()
 
   _ep_get_update_disconnected(update_disconnected ${name})
-  if(update_disconnected)
-    set(patch_dep download)
-  else()
-    set(patch_dep update)
-  endif()
 
   set(__cmdQuoted)
   foreach(__item IN LISTS cmd)
@@ -3481,11 +3538,28 @@ function(_ep_add_patch_command name)
       INDEPENDENT TRUE
       COMMAND ${__cmdQuoted}
       WORKING_DIRECTORY \${work_dir}
-      DEPENDEES \${patch_dep}
+      EXCLUDE_FROM_MAIN \${update_disconnected}
+      DEPENDEES update
+      DEPENDS \${patch_info_file}
       ${log}
       ${uses_terminal}
     )"
   )
+
+  if(update_disconnected)
+    cmake_language(EVAL CODE "
+      ExternalProject_Add_Step(${name} patch_disconnected
+        INDEPENDENT TRUE
+        COMMAND ${__cmdQuoted}
+        WORKING_DIRECTORY \${work_dir}
+        DEPENDEES update_disconnected
+        DEPENDS \${patch_info_file}
+        ${log}
+        ${uses_terminal}
+      )"
+    )
+  endif()
+
 endfunction()
 
 function(_ep_get_file_deps var name)
@@ -3695,6 +3769,13 @@ function(_ep_add_configure_command name)
   list(APPEND file_deps ${tmp_dir}/${name}-cfgcmd.txt)
   list(APPEND file_deps ${_ep_cache_args_script})
 
+  _ep_get_update_disconnected(update_disconnected ${name})
+  if(update_disconnected)
+    set(dependees patch_disconnected)
+  else()
+    set(dependees patch)
+  endif()
+
   get_property(log
     TARGET ${name}
     PROPERTY _EP_LOG_CONFIGURE
@@ -3724,7 +3805,7 @@ function(_ep_add_configure_command name)
       INDEPENDENT FALSE
       COMMAND ${__cmdQuoted}
       WORKING_DIRECTORY \${binary_dir}
-      DEPENDEES patch
+      DEPENDEES \${dependees}
       DEPENDS \${file_deps}
       ${log}
       ${uses_terminal}

Modules/ExternalProject/PatchInfo.txt.in

@@ -0,0 +1,6 @@
+# This is a generated file and its contents are an internal implementation detail.
+# The update step will be re-executed if anything in this file changes.
+# No other meaning or use of this file is supported.
+
+command=@cmd@
+work_dir=@work_dir@

Modules/ExternalProject/UpdateInfo.txt.in

@@ -0,0 +1,7 @@
+# This is a generated file and its contents are an internal implementation detail.
+# The patch step will be re-executed if anything in this file changes.
+# No other meaning or use of this file is supported.
+
+command (connected)=@cmd@
+command (disconnected)=@cmd_disconnected@
+work_dir=@work_dir@

Modules/ExternalProject/gitupdate.cmake.in

@@ -3,6 +3,15 @@
 
 cmake_minimum_required(VERSION 3.5)
 
+function(do_fetch)
+  message(VERBOSE "Fetching latest from the remote @git_remote_name@")
+  execute_process(
+    COMMAND "@git_EXECUTABLE@" --git-dir=.git fetch --tags --force "@git_remote_name@"
+    WORKING_DIRECTORY "@work_dir@"
+    COMMAND_ERROR_IS_FATAL LAST
+  )
+endfunction()
+
 function(get_hash_for_ref ref out_var err_var)
   execute_process(
     COMMAND "@git_EXECUTABLE@" --git-dir=.git rev-parse "${ref}^0"
@@ -33,17 +42,16 @@ execute_process(
 )
 if(show_ref_output MATCHES "^[a-z0-9]+[ \\t]+refs/remotes/")
   # Given a full remote/branch-name and we know about it already. Since
-  # branches can move around, we always have to fetch.
-  set(fetch_required YES)
+  # branches can move around, we should always fetch, if permitted.
+  if(can_fetch)
+    do_fetch()
+  endif()
   set(checkout_name "@git_tag@")
 
 elseif(show_ref_output MATCHES "^[a-z0-9]+[ \\t]+refs/tags/")
   # Given a tag name that we already know about. We don't know if the tag we
-  # have matches the remote though (tags can move), so we should fetch.
-  set(fetch_required YES)
-  set(checkout_name "@git_tag@")
-
-  # Special case to preserve backward compatibility: if we are already at the
+  # have matches the remote though (tags can move), so we should fetch. As a
+  # special case to preserve backward compatibility, if we are already at the
   # same commit as the tag we hold locally, don't do a fetch and assume the tag
   # hasn't moved on the remote.
   # FIXME: We should provide an option to always fetch for this case
@@ -53,12 +61,20 @@ elseif(show_ref_output MATCHES "^[a-z0-9]+[ \\t]+refs/tags/")
     return()
   endif()
 
+  if(can_fetch)
+    do_fetch()
+  endif()
+  set(checkout_name "@git_tag@")
+
 elseif(show_ref_output MATCHES "^[a-z0-9]+[ \\t]+refs/heads/")
   # Given a branch name without any remote and we already have a branch by that
   # name. We might already have that branch checked out or it might be a
-  # different branch. It isn't safe to use a bare branch name without the
-  # remote, so do a fetch and replace the ref with one that includes the remote.
-  set(fetch_required YES)
+  # different branch. It isn't fully safe to use a bare branch name without the
+  # remote, so do a fetch (if allowed) and replace the ref with one that
+  # includes the remote.
+  if(can_fetch)
+    do_fetch()
+  endif()
   set(checkout_name "@git_remote_name@/@git_tag@")
 
 else()
@@ -70,20 +86,26 @@ else()
 
   elseif(tag_sha STREQUAL "")
     # We don't know about this ref yet, so we have no choice but to fetch.
+    if(NOT can_fetch)
+      message(FATAL_ERROR
+        "Requested git ref \"@git_tag@\" is not present locally, and not "
+        "allowed to contact remote due to UPDATE_DISCONNECTED setting."
+      )
+    endif()
+
     # We deliberately swallow any error message at the default log level
     # because it can be confusing for users to see a failed git command.
     # That failure is being handled here, so it isn't an error.
-    set(fetch_required YES)
-    set(checkout_name "@git_tag@")
     if(NOT error_msg STREQUAL "")
       message(VERBOSE "${error_msg}")
     endif()
+    do_fetch()
+    set(checkout_name "@git_tag@")
 
   else()
     # We have the commit, so we know we were asked to find a commit hash
     # (otherwise it would have been handled further above), but we don't
-    # have that commit checked out yet
-    set(fetch_required NO)
+    # have that commit checked out yet. We don't need to fetch from the remote.
     set(checkout_name "@git_tag@")
     if(NOT error_msg STREQUAL "")
       message(WARNING "${error_msg}")
@@ -92,15 +114,6 @@ else()
   endif()
 endif()
 
-if(fetch_required)
-  message(VERBOSE "Fetching latest from the remote @git_remote_name@")
-  execute_process(
-    COMMAND "@git_EXECUTABLE@" --git-dir=.git fetch --tags --force "@git_remote_name@"
-    WORKING_DIRECTORY "@work_dir@"
-    COMMAND_ERROR_IS_FATAL ANY
-  )
-endif()
-
 set(git_update_strategy "@git_update_strategy@")
 if(git_update_strategy STREQUAL "")
   # Backward compatibility requires REBASE as the default behavior

Modules/FetchContent.cmake

@@ -665,10 +665,16 @@ A number of cache variables can influence the behavior where details from a
   This is a less severe download/update control compared to
   :variable:`FETCHCONTENT_FULLY_DISCONNECTED`.  Instead of bypassing all
   download and update logic, ``FETCHCONTENT_UPDATES_DISCONNECTED`` only
-  disables the update stage.  Therefore, if content has not been downloaded
-  previously, it will still be downloaded when this option is enabled.
-  This can speed up the configure stage, but not as much as
-  :variable:`FETCHCONTENT_FULLY_DISCONNECTED`.  It is ``OFF`` by default.
+  prevents the update step from making connections to remote servers
+  when using the git or hg download methods.  Updates still occur if details
+  about the update step change, but the update is attempted with only the
+  information already available locally (so switching to a different tag or
+  commit that is already fetched locally will succeed, but switching to an
+  unknown commit hash will fail).  The download step is not affected, so if
+  content has not been downloaded previously, it will still be downloaded
+  when this option is enabled.  This can speed up the configure step, but
+  not as much as :variable:`FETCHCONTENT_FULLY_DISCONNECTED`.
+  ``FETCHCONTENT_UPDATES_DISCONNECTED`` is ``OFF`` by default.
 
 .. variable:: FETCHCONTENT_TRY_FIND_PACKAGE_MODE
 
@@ -735,10 +741,11 @@ content name:
 
   This is the per-content equivalent of
   :variable:`FETCHCONTENT_UPDATES_DISCONNECTED`.  If the global option or
-  this option is ``ON``, then updates will be disabled for the named content.
-  Disabling updates for individual content can be useful for content whose
-  details rarely change, while still leaving other frequently changing content
-  with updates enabled.
+  this option is ``ON``, then updates for the git and hg methods will not
+  contact any remote for the named content.  They will only use information
+  already available locally.  Disabling updates for individual content can
+  be useful for content whose details rarely change, while still leaving
+  other frequently changing content with updates enabled.
 
 .. _`fetch-content-examples`:
 

Tests/ExternalProjectUpdate/CMakeLists.txt

@@ -70,7 +70,9 @@ if(NOT TEST_GIT_TAG)
 endif()
 
 if(do_git_tests)
-  set(local_git_repo "../../LocalRepositories/GIT")
+  cmake_path(SET local_git_repo NORMALIZE
+    "${CMAKE_CURRENT_BINARY_DIR}/LocalRepositories/GIT"
+  )
 
   # Unzip/untar the git repository in our source folder so that other
   # projects below may use it to test git args of ExternalProject_Add
@@ -79,6 +81,7 @@ if(do_git_tests)
   ExternalProject_Add(${proj}
     SOURCE_DIR ${CMAKE_CURRENT_BINARY_DIR}/LocalRepositories/GIT
     URL ${CMAKE_CURRENT_SOURCE_DIR}/gitrepo.tgz
+    DOWNLOAD_EXTRACT_TIMESTAMP NO
     BUILD_COMMAND ""
     CONFIGURE_COMMAND "${GIT_EXECUTABLE}" --version
     INSTALL_COMMAND ""

Tests/ExternalProjectUpdate/ExternalProjectUpdateTest.cmake

@@ -1,8 +1,14 @@
 # Set the ExternalProject GIT_TAG to desired_tag, and make sure the
 # resulting checked out version is resulting_sha and rebuild.
 # This check's the correct behavior of the ExternalProject UPDATE_COMMAND.
-# Also verify that a fetch only occurs when fetch_expected is 1.
-macro(check_a_tag desired_tag resulting_sha fetch_expected update_strategy)
+# Also verify that a fetch only occurs when fetch_expected_tsX is 1.
+macro(check_a_tag
+  desired_tag
+  resulting_sha
+  fetch_expected_ts1  # TutorialStep1-GIT
+  fetch_expected_ts2  # TutorialStep2-GIT
+  update_strategy
+)
   message( STATUS "Checking ExternalProjectUpdate to tag: ${desired_tag}" )
 
   # Remove the FETCH_HEAD file, so we can check if it gets replaced with a 'git
@@ -12,7 +18,11 @@ macro(check_a_tag desired_tag resulting_sha fetch_expected update_strategy)
 
   # Give ourselves a marker in the output. It is difficult to tell where we
   # are up to without this
-  message(STATUS "===> check_a_tag ${desired_tag} ${resulting_sha} ${fetch_expected} ${update_strategy}")
+  message(STATUS "===> check_a_tag: "
+    "${desired_tag} ${resulting_sha} "
+    "${fetch_expected_ts1} ${fetch_expected_ts2} "
+    "${update_strategy}"
+  )
 
   # Configure
   execute_process(COMMAND ${CMAKE_COMMAND}
@@ -58,10 +68,10 @@ was expected."
     )
   endif()
 
-  if( NOT EXISTS ${FETCH_HEAD_file} AND ${fetch_expected})
+  if( NOT EXISTS ${FETCH_HEAD_file} AND ${fetch_expected_ts1})
     message( FATAL_ERROR "Fetch did NOT occur when it was expected.")
   endif()
-  if( EXISTS ${FETCH_HEAD_file} AND NOT ${fetch_expected})
+  if( EXISTS ${FETCH_HEAD_file} AND NOT ${fetch_expected_ts1})
     message( FATAL_ERROR "Fetch DID occur when it was not expected.")
   endif()
 
@@ -154,10 +164,10 @@ was expected."
     )
   endif()
 
-  if( NOT EXISTS ${FETCH_HEAD_file} AND ${fetch_expected})
+  if( NOT EXISTS ${FETCH_HEAD_file} AND ${fetch_expected_ts2})
     message( FATAL_ERROR "Fetch did NOT occur when it was expected.")
   endif()
-  if( EXISTS ${FETCH_HEAD_file} AND NOT ${fetch_expected})
+  if( EXISTS ${FETCH_HEAD_file} AND NOT ${fetch_expected_ts2})
     message( FATAL_ERROR "Fetch DID occur when it was not expected.")
   endif()
 endmacro()
@@ -179,16 +189,16 @@ endif()
 file(REMOVE_RECURSE ${ExternalProjectUpdate_BINARY_DIR}/CMakeExternals)
 
 if(do_git_tests)
-  check_a_tag(origin/master b5752a26ae448410926b35c275af3c192a53722e 1 REBASE)
-  check_a_tag(tag1          d1970730310fe8bc07e73f15dc570071f9f9654a 1 REBASE)
+  check_a_tag(origin/master b5752a26ae448410926b35c275af3c192a53722e 1 1 REBASE)
+  check_a_tag(tag1          d1970730310fe8bc07e73f15dc570071f9f9654a 1 0 REBASE)
   # With the Git UPDATE_COMMAND performance patch, this will not require a
   # 'git fetch'
-  check_a_tag(tag1          d1970730310fe8bc07e73f15dc570071f9f9654a 0 REBASE)
-  check_a_tag(tag2          5842b503ba4113976d9bb28d57b5aee1ad2736b7 1 REBASE)
-  check_a_tag(d19707303     d1970730310fe8bc07e73f15dc570071f9f9654a 0 REBASE)
-  check_a_tag(origin/master b5752a26ae448410926b35c275af3c192a53722e 1 REBASE)
+  check_a_tag(tag1          d1970730310fe8bc07e73f15dc570071f9f9654a 0 0 REBASE)
+  check_a_tag(tag2          5842b503ba4113976d9bb28d57b5aee1ad2736b7 1 0 REBASE)
+  check_a_tag(d19707303     d1970730310fe8bc07e73f15dc570071f9f9654a 0 0 REBASE)
+  check_a_tag(origin/master b5752a26ae448410926b35c275af3c192a53722e 1 1 REBASE)
   # This is a remote symbolic ref, so it will always trigger a 'git fetch'
-  check_a_tag(origin/master b5752a26ae448410926b35c275af3c192a53722e 1 REBASE)
+  check_a_tag(origin/master b5752a26ae448410926b35c275af3c192a53722e 1 1 REBASE)
 
   foreach(strategy IN ITEMS CHECKOUT REBASE_CHECKOUT)
     # Move local master back, then apply a change that will cause a conflict
@@ -222,7 +232,7 @@ if(do_git_tests)
       message(FATAL_ERROR "Could not commit conflicting change.")
     endif()
     # This should discard our commit but leave behind an annotated tag
-    check_a_tag(origin/master b5752a26ae448410926b35c275af3c192a53722e 1 ${strategy})
+    check_a_tag(origin/master b5752a26ae448410926b35c275af3c192a53722e 1 1 ${strategy})
   endforeach()
 
   # This file matches a .gitignore rule that the last commit defines. We can't
@@ -232,7 +242,7 @@ if(do_git_tests)
   # doesn't choke on it.
   set(ignoredFile ${ExternalProjectUpdate_BINARY_DIR}/CMakeExternals/Source/TutorialStep1-GIT/ignored_item)
   file(TOUCH ${ignoredFile})
-  check_a_tag(origin/master b5752a26ae448410926b35c275af3c192a53722e 1 REBASE)
+  check_a_tag(origin/master b5752a26ae448410926b35c275af3c192a53722e 1 1 REBASE)
   if(NOT EXISTS ${ignoredFile})
     message(FATAL_ERROR "Ignored file is missing")
   endif()