From f588421b58f23b192670ad686e2797a09489a096 Mon Sep 17 00:00:00 2001 From: Craig Scott Date: Tue, 28 May 2024 22:56:25 +1000 Subject: FetchContent: Enforce FETCHCONTENT_FULLY_DISCONNECTED requirements FETCHCONTENT_FULLY_DISCONNECTED should only be set to true if each dependency's source directory has already been populated. Previously, this wasn't being checked, but now it is (subject to a new policy). --- Help/manual/cmake-policies.7.rst | 1 + Help/policy/CMP0170.rst | 30 ++++++++++++ .../enforce-fc-fully-disconnected-requirements.rst | 9 ++++ Modules/FetchContent.cmake | 56 ++++++++++++++++++++-- Source/cmPolicies.h | 5 +- Tests/RunCMake/CMP0170/CMP0170-NEW-result.txt | 1 + Tests/RunCMake/CMP0170/CMP0170-NEW-stderr.txt | 14 ++++++ Tests/RunCMake/CMP0170/CMP0170-NEW-stdout.txt | 2 + Tests/RunCMake/CMP0170/CMP0170-NEW.cmake | 2 + Tests/RunCMake/CMP0170/CMP0170-OLD-stdout.txt | 3 ++ Tests/RunCMake/CMP0170/CMP0170-OLD.cmake | 2 + Tests/RunCMake/CMP0170/CMP0170-WARN-stderr.txt | 28 +++++++++++ Tests/RunCMake/CMP0170/CMP0170-WARN-stdout.txt | 3 ++ Tests/RunCMake/CMP0170/CMP0170-WARN.cmake | 1 + Tests/RunCMake/CMP0170/CMP0170.cmake | 18 +++++++ Tests/RunCMake/CMP0170/CMakeLists.txt | 3 ++ Tests/RunCMake/CMP0170/RunCMakeTest.cmake | 5 ++ Tests/RunCMake/CMakeLists.txt | 1 + 18 files changed, 178 insertions(+), 6 deletions(-) create mode 100644 Help/policy/CMP0170.rst create mode 100644 Help/release/dev/enforce-fc-fully-disconnected-requirements.rst create mode 100644 Tests/RunCMake/CMP0170/CMP0170-NEW-result.txt create mode 100644 Tests/RunCMake/CMP0170/CMP0170-NEW-stderr.txt create mode 100644 Tests/RunCMake/CMP0170/CMP0170-NEW-stdout.txt create mode 100644 Tests/RunCMake/CMP0170/CMP0170-NEW.cmake create mode 100644 Tests/RunCMake/CMP0170/CMP0170-OLD-stdout.txt create mode 100644 Tests/RunCMake/CMP0170/CMP0170-OLD.cmake create mode 100644 Tests/RunCMake/CMP0170/CMP0170-WARN-stderr.txt create mode 100644 Tests/RunCMake/CMP0170/CMP0170-WARN-stdout.txt create mode 100644 Tests/RunCMake/CMP0170/CMP0170-WARN.cmake create mode 100644 Tests/RunCMake/CMP0170/CMP0170.cmake create mode 100644 Tests/RunCMake/CMP0170/CMakeLists.txt create mode 100644 Tests/RunCMake/CMP0170/RunCMakeTest.cmake 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. CMP0169: FetchContent_Populate(depName) single-argument signature is deprecated. CMP0168: FetchContent implements steps directly instead of through a sub-build. CMP0167: The FindBoost module is removed. 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 a4bd7fe..820d3e6 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 ` 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 @@ -1937,8 +1943,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() @@ -1955,7 +1965,7 @@ function(FetchContent_Populate contentName) endfunction() -function(__FetchContent_Populate contentName) +function(__FetchContent_Populate contentName cmp0170) string(TOLOWER ${contentName} contentNameLower) @@ -2047,6 +2057,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}) @@ -2305,7 +2347,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 8c047d0..5c1e5f6 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 -- cgit v0.12