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 <kwrobot@kitware.com>
Merge-request: !8498

5 files changed, 158 insertions, 44 deletions

diff --git a/Modules/ExternalProject.cmake b/Modules/ExternalProject.cmake
index 1fdd754..bac126c 100644
--- a/Modules/ExternalProject.cmake
+++ b/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}
diff --git a/Modules/ExternalProject/PatchInfo.txt.in b/Modules/ExternalProject/PatchInfo.txt.in
new file mode 100644
index 0000000..112953c
--- /dev/null
+++ b/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@
diff --git a/Modules/ExternalProject/UpdateInfo.txt.in b/Modules/ExternalProject/UpdateInfo.txt.in
new file mode 100644
index 0000000..67ee434
--- /dev/null
+++ b/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@
diff --git a/Modules/ExternalProject/gitupdate.cmake.in b/Modules/ExternalProject/gitupdate.cmake.in
index 50f0167..eb3cda7 100644
--- a/Modules/ExternalProject/gitupdate.cmake.in
+++ b/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
diff --git a/Modules/FetchContent.cmake b/Modules/FetchContent.cmake
index 74ac8aa..56fc0ed 100644
--- a/Modules/FetchContent.cmake
+++ b/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`: