Merge topic 'enforce-fc-fully-disconnected-requirements'

f588421b58 FetchContent: Enforce FETCHCONTENT_FULLY_DISCONNECTED requirements

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

18 files changed, 178 insertions, 6 deletions

diff --git a/Help/manual/cmake-policies.7.rst b/Help/manual/cmake-policies.7.rst
index 7d2440e..7155404 100644
--- a/Help/manual/cmake-policies.7.rst
+++ b/Help/manual/cmake-policies.7.rst
@@ -57,6 +57,7 @@ Policies Introduced by CMake 3.30
 .. toctree::
    :maxdepth: 1
 
+   CMP0170: FETCHCONTENT_FULLY_DISCONNECTED requirements are enforced. </policy/CMP0170>
    CMP0169: FetchContent_Populate(depName) single-argument signature is deprecated. </policy/CMP0169>
    CMP0168: FetchContent implements steps directly instead of through a sub-build. </policy/CMP0168>
    CMP0167: The FindBoost module is removed. </policy/CMP0167>
diff --git a/Help/policy/CMP0170.rst b/Help/policy/CMP0170.rst
new file mode 100644
index 0000000..9d7860a
--- /dev/null
+++ b/Help/policy/CMP0170.rst
@@ -0,0 +1,30 @@
+CMP0170
+-------
+
+.. versionadded:: 3.30
+
+When ``FETCHCONTENT_FULLY_DISCONNECTED`` is set to true,
+:command:`FetchContent_MakeAvailable` and :command:`FetchContent_Populate`
+enforce the constraint that their source directory must already be populated.
+The requirement has always been documented, but it was not checked or enforced
+with CMake 3.29 or older.  This sometimes led to hard-to-trace errors when a
+project expected a dependency to have been populated, but its population was
+silently skipped.
+
+CMake 3.30 and above prefers to check and enforce the constraint.
+This policy provides compatibility for situations where the user cannot easily
+prevent ``FETCHCONTENT_FULLY_DISCONNECTED`` from being inappropriately set
+to true.
+
+The ``OLD`` behavior of this policy allows ``FETCHCONTENT_FULLY_DISCONNECTED``
+to be set to true even if a dependency's source directory has not been
+populated.
+The ``NEW`` behavior halts with a fatal error if
+``FETCHCONTENT_FULLY_DISCONNECTED`` is set to true and a dependency population
+would be skipped, but that dependency's source directory doesn't exist.
+
+.. |INTRODUCED_IN_CMAKE_VERSION| replace:: 3.30
+.. |WARNS_OR_DOES_NOT_WARN| replace:: warns
+.. include:: STANDARD_ADVICE.txt
+
+.. include:: DEPRECATED.txt
diff --git a/Help/release/dev/enforce-fc-fully-disconnected-requirements.rst b/Help/release/dev/enforce-fc-fully-disconnected-requirements.rst
new file mode 100644
index 0000000..2db759c
--- /dev/null
+++ b/Help/release/dev/enforce-fc-fully-disconnected-requirements.rst
@@ -0,0 +1,9 @@
+enforce-fc-fully-disconnected-requirements
+------------------------------------------
+
+* When :variable:`FETCHCONTENT_FULLY_DISCONNECTED` is set to true,
+  :command:`FetchContent_MakeAvailable` and the single-argument form of
+  :command:`FetchContent_Populate` require that the dependency's source
+  directory has already been populated. CMake 3.29 and earlier did not
+  check this requirement, but it is now enforced, subject to policy
+  :policy:`CMP0170`.
diff --git a/Modules/FetchContent.cmake b/Modules/FetchContent.cmake
index b34476e..28541ed 100644
--- a/Modules/FetchContent.cmake
+++ b/Modules/FetchContent.cmake
@@ -484,7 +484,8 @@ Commands
   - ``TEST_COMMAND``
 
   With this form, the :variable:`FETCHCONTENT_FULLY_DISCONNECTED` and
-  :variable:`FETCHCONTENT_UPDATES_DISCONNECTED` variables are ignored.
+  :variable:`FETCHCONTENT_UPDATES_DISCONNECTED` variables and policy
+  :policy:`CMP0170` are ignored.
 
   When this form of ``FetchContent_Populate()`` returns, the following
   variables will be set in the scope of the caller:
@@ -699,6 +700,11 @@ A number of cache variables can influence the behavior where details from a
     :ref:`dependency provider <dependency_providers>` and populate the
     dependency from local content instead.
 
+  .. versionchanged:: 3.30
+    The constraint that the source directory has already been populated when
+    ``FETCHCONTENT_FULLY_DISCONNECTED`` is true is now enforced.
+    See policy :policy:`CMP0170`.
+
 .. variable:: FETCHCONTENT_UPDATES_DISCONNECTED
 
   This is a less severe download/update control compared to
@@ -1943,8 +1949,12 @@ function(FetchContent_Populate contentName)
     endif()
   endif()
 
-  cmake_parse_arguments(PARSE_ARGV 0 __arg "" "" "")
-  set(__argsQuoted)
+  cmake_policy(GET CMP0170 cmp0170
+    PARENT_SCOPE # undocumented, do not use outside of CMake
+  )
+
+  cmake_parse_arguments(PARSE_ARGV 1 __arg "" "" "")
+  set(__argsQuoted "[==[${contentName}]==] [==[${cmp0170}]==]")
   foreach(__item IN LISTS __arg_UNPARSED_ARGUMENTS __doDirectArgs)
     string(APPEND __argsQuoted " [==[${__item}]==]")
   endforeach()
@@ -1961,7 +1971,7 @@ function(FetchContent_Populate contentName)
 
 endfunction()
 
-function(__FetchContent_Populate contentName)
+function(__FetchContent_Populate contentName cmp0170)
 
   string(TOLOWER ${contentName} contentNameLower)
 
@@ -2053,6 +2063,38 @@ function(__FetchContent_Populate contentName)
     else()
       set(${contentNameLower}_SOURCE_DIR "${FETCHCONTENT_BASE_DIR}/${contentNameLower}-src")
     endif()
+    if(NOT IS_ABSOLUTE "${${contentNameLower}_SOURCE_DIR}")
+      message(WARNING
+        "Relative source directory specified. This is not safe, as it depends "
+        "on the calling directory scope.\n"
+        "  ${${contentNameLower}_SOURCE_DIR}"
+      )
+      set(${contentNameLower}_SOURCE_DIR
+        "${CMAKE_CURRENT_BINARY_DIR}/${${contentNameLower}_SOURCE_DIR}"
+      )
+    endif()
+    if(NOT EXISTS "${${contentNameLower}_SOURCE_DIR}")
+      if(cmp0170 STREQUAL "")
+        set(cmp0170 WARN)
+      endif()
+      string(CONCAT msg
+        "FETCHCONTENT_FULLY_DISCONNECTED is set to true, which requires the "
+        "source directory for dependency ${contentName} to already be populated. "
+        "This generally means it must not be set to true the first time CMake "
+        "is run in a build directory. The following source directory should "
+        "already be populated, but it doesn't exist:\n"
+        "  ${${contentNameLower}_SOURCE_DIR}\n"
+        "Policy CMP0170 controls enforcement of this requirement."
+      )
+      if(cmp0170 STREQUAL "NEW")
+        message(FATAL_ERROR "${msg}")
+      elseif(NOT cmp0170 STREQUAL "OLD")
+        # Note that this is a user warning, not a project author warning.
+        # The user has set FETCHCONTENT_FULLY_DISCONNECTED in a scenario
+        # where that is not allowed.
+        message(WARNING "${msg}")
+      endif()
+    endif()
 
     if(savedDetails_BINARY_DIR)
       set(${contentNameLower}_BINARY_DIR ${savedDetails_BINARY_DIR})
@@ -2320,7 +2362,11 @@ macro(FetchContent_MakeAvailable)
 
     FetchContent_GetProperties(${__cmake_contentName})
     if(NOT ${__cmake_contentNameLower}_POPULATED)
-      __FetchContent_Populate(${__cmake_contentName})
+      cmake_policy(GET CMP0170 __cmake_fc_cmp0170
+        PARENT_SCOPE # undocumented, do not use outside of CMake
+      )
+      __FetchContent_Populate(${__cmake_contentName} "${__cmake_fc_cmp0170}")
+      unset(__cmake_fc_cmp0170)
       __FetchContent_setupFindPackageRedirection(${__cmake_contentName})
 
       # Only try to call add_subdirectory() if the populated content
diff --git a/Source/cmPolicies.h b/Source/cmPolicies.h
index 076066f..d893c44 100644
--- a/Source/cmPolicies.h
+++ b/Source/cmPolicies.h
@@ -522,7 +522,10 @@ class cmMakefile;
   SELECT(POLICY, CMP0169,                                                     \
          "FetchContent_Populate(depName) single-argument signature is "       \
          "deprecated.",                                                       \
-         3, 30, 0, cmPolicies::WARN)
+         3, 30, 0, cmPolicies::WARN)                                          \
+  SELECT(POLICY, CMP0170,                                                     \
+         "FETCHCONTENT_FULLY_DISCONNECTED requirements are enforced.", 3, 30, \
+         0, cmPolicies::WARN)
 
 #define CM_SELECT_ID(F, A1, A2, A3, A4, A5, A6) F(A1)
 #define CM_FOR_EACH_POLICY_ID(POLICY)                                         \
diff --git a/Tests/RunCMake/CMP0170/CMP0170-NEW-result.txt b/Tests/RunCMake/CMP0170/CMP0170-NEW-result.txt
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/Tests/RunCMake/CMP0170/CMP0170-NEW-result.txt
@@ -0,0 +1 @@
+1
diff --git a/Tests/RunCMake/CMP0170/CMP0170-NEW-stderr.txt b/Tests/RunCMake/CMP0170/CMP0170-NEW-stderr.txt
new file mode 100644
index 0000000..f786bd6
--- /dev/null
+++ b/Tests/RunCMake/CMP0170/CMP0170-NEW-stderr.txt
@@ -0,0 +1,14 @@
+CMake Error at .*/Modules/FetchContent\.cmake:[0-9]+ \(message\):
+  FETCHCONTENT_FULLY_DISCONNECTED is set to true, which requires the source
+  directory for dependency t1 to already be populated\.  This generally means
+  it must not be set to true the first time CMake is run in a build
+  directory\.  The following source directory should already be populated, but
+  it doesn't exist:[
+]+    .*/Tests/RunCMake/CMP0170/IdoNotExist[
+]+  Policy CMP0170 controls enforcement of this requirement\.
+Call Stack \(most recent call first\):
+  .*/Modules/FetchContent\.cmake:[0-9]+:EVAL:1 \(__FetchContent_Populate\)
+  .*/Modules/FetchContent\.cmake:[0-9]+ \(cmake_language\)
+  CMP0170\.cmake:[0-9]+ \(FetchContent_Populate\)
+  CMP0170-NEW\.cmake:[0-9]+ \(include\)
+  CMakeLists\.txt:[0-9]+ \(include\)
diff --git a/Tests/RunCMake/CMP0170/CMP0170-NEW-stdout.txt b/Tests/RunCMake/CMP0170/CMP0170-NEW-stdout.txt
new file mode 100644
index 0000000..2dc34a6
--- /dev/null
+++ b/Tests/RunCMake/CMP0170/CMP0170-NEW-stdout.txt
@@ -0,0 +1,2 @@
+-- Starting population
+-- Configuring incomplete, errors occurred!
diff --git a/Tests/RunCMake/CMP0170/CMP0170-NEW.cmake b/Tests/RunCMake/CMP0170/CMP0170-NEW.cmake
new file mode 100644
index 0000000..60b386a
--- /dev/null
+++ b/Tests/RunCMake/CMP0170/CMP0170-NEW.cmake
@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0170 NEW)
+include(CMP0170.cmake)
diff --git a/Tests/RunCMake/CMP0170/CMP0170-OLD-stdout.txt b/Tests/RunCMake/CMP0170/CMP0170-OLD-stdout.txt
new file mode 100644
index 0000000..82c260b
--- /dev/null
+++ b/Tests/RunCMake/CMP0170/CMP0170-OLD-stdout.txt
@@ -0,0 +1,3 @@
+-- Starting population
+-- Configuring done \([0-9]+\.[0-9]s\)
+-- Generating done \([0-9]+\.[0-9]s\)
diff --git a/Tests/RunCMake/CMP0170/CMP0170-OLD.cmake b/Tests/RunCMake/CMP0170/CMP0170-OLD.cmake
new file mode 100644
index 0000000..812cca0
--- /dev/null
+++ b/Tests/RunCMake/CMP0170/CMP0170-OLD.cmake
@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0170 OLD)
+include(CMP0170.cmake)
diff --git a/Tests/RunCMake/CMP0170/CMP0170-WARN-stderr.txt b/Tests/RunCMake/CMP0170/CMP0170-WARN-stderr.txt
new file mode 100644
index 0000000..1253abd
--- /dev/null
+++ b/Tests/RunCMake/CMP0170/CMP0170-WARN-stderr.txt
@@ -0,0 +1,28 @@
+CMake Warning at .*/Modules/FetchContent\.cmake:[0-9]+ \(message\):
+  FETCHCONTENT_FULLY_DISCONNECTED is set to true, which requires the source
+  directory for dependency t1 to already be populated\.  This generally means
+  it must not be set to true the first time CMake is run in a build
+  directory\.  The following source directory should already be populated, but
+  it doesn't exist:[
+]+    .*/Tests/RunCMake/CMP0170/IdoNotExist[
+]+  Policy CMP0170 controls enforcement of this requirement\.
+Call Stack \(most recent call first\):
+  .*/Modules/FetchContent\.cmake:[0-9]+:EVAL:1 \(__FetchContent_Populate\)
+  .*/Modules/FetchContent\.cmake:[0-9]+ \(cmake_language\)
+  CMP0170\.cmake:[0-9]+ \(FetchContent_Populate\)
+  CMP0170-WARN\.cmake:[0-9]+ \(include\)
+  CMakeLists\.txt:[0-9]+ \(include\)
+[
+]+CMake Warning at .*/Modules/FetchContent\.cmake:[0-9]+ \(message\):
+  FETCHCONTENT_FULLY_DISCONNECTED is set to true, which requires the source
+  directory for dependency t2 to already be populated\.  This generally means
+  it must not be set to true the first time CMake is run in a build
+  directory\.  The following source directory should already be populated, but
+  it doesn't exist:[
+]+    .*/Tests/RunCMake/CMP0170/IdoNotExist[
+]+  Policy CMP0170 controls enforcement of this requirement\.
+Call Stack \(most recent call first\):
+  .*/Modules/FetchContent\.cmake:[0-9]+ \(__FetchContent_Populate\)
+  CMP0170\.cmake:[0-9]+ \(FetchContent_MakeAvailable\)
+  CMP0170-WARN\.cmake:[0-9]+ \(include\)
+  CMakeLists\.txt:[0-9]+ \(include\)
diff --git a/Tests/RunCMake/CMP0170/CMP0170-WARN-stdout.txt b/Tests/RunCMake/CMP0170/CMP0170-WARN-stdout.txt
new file mode 100644
index 0000000..82c260b
--- /dev/null
+++ b/Tests/RunCMake/CMP0170/CMP0170-WARN-stdout.txt
@@ -0,0 +1,3 @@
+-- Starting population
+-- Configuring done \([0-9]+\.[0-9]s\)
+-- Generating done \([0-9]+\.[0-9]s\)
diff --git a/Tests/RunCMake/CMP0170/CMP0170-WARN.cmake b/Tests/RunCMake/CMP0170/CMP0170-WARN.cmake
new file mode 100644
index 0000000..1754c24
--- /dev/null
+++ b/Tests/RunCMake/CMP0170/CMP0170-WARN.cmake
@@ -0,0 +1 @@
+include(CMP0170.cmake)
diff --git a/Tests/RunCMake/CMP0170/CMP0170.cmake b/Tests/RunCMake/CMP0170/CMP0170.cmake
new file mode 100644
index 0000000..4e63e25
--- /dev/null
+++ b/Tests/RunCMake/CMP0170/CMP0170.cmake
@@ -0,0 +1,18 @@
+cmake_policy(SET CMP0168 NEW)  # Faster, don't need to test with sub-build
+cmake_policy(SET CMP0169 OLD)  # So we can test FetchContent_Populate() directly
+
+set(FETCHCONTENT_FULLY_DISCONNECTED TRUE)
+
+include(FetchContent)
+
+message(STATUS "Starting population")
+
+FetchContent_Declare(t1
+  SOURCE_DIR "${CMAKE_CURRENT_LIST_DIR}/IdoNotExist"
+)
+FetchContent_Populate(t1)
+
+FetchContent_Declare(t2
+  SOURCE_DIR "${CMAKE_CURRENT_LIST_DIR}/IdoNotExist"
+)
+FetchContent_MakeAvailable(t2)
diff --git a/Tests/RunCMake/CMP0170/CMakeLists.txt b/Tests/RunCMake/CMP0170/CMakeLists.txt
new file mode 100644
index 0000000..94e43ba
--- /dev/null
+++ b/Tests/RunCMake/CMP0170/CMakeLists.txt
@@ -0,0 +1,3 @@
+cmake_minimum_required(VERSION 3.29)
+project(${RunCMake_TEST} NONE)
+include(${RunCMake_TEST}.cmake)
diff --git a/Tests/RunCMake/CMP0170/RunCMakeTest.cmake b/Tests/RunCMake/CMP0170/RunCMakeTest.cmake
new file mode 100644
index 0000000..0ba64e3
--- /dev/null
+++ b/Tests/RunCMake/CMP0170/RunCMakeTest.cmake
@@ -0,0 +1,5 @@
+include(RunCMake)
+
+run_cmake(CMP0170-WARN)
+run_cmake(CMP0170-OLD)
+run_cmake(CMP0170-NEW)
diff --git a/Tests/RunCMake/CMakeLists.txt b/Tests/RunCMake/CMakeLists.txt
index e87a734..94382fc 100644
--- a/Tests/RunCMake/CMakeLists.txt
+++ b/Tests/RunCMake/CMakeLists.txt
@@ -176,6 +176,7 @@ add_RunCMake_test(CMP0160)
 add_RunCMake_test(CMP0163)
 add_RunCMake_test(CMP0165)
 add_RunCMake_test(CMP0169)
+add_RunCMake_test(CMP0170)
 
 # The test for Policy 65 requires the use of the
 # CMAKE_SHARED_LIBRARY_LINK_CXX_FLAGS variable, which both the VS and Xcode