conan-provider.cmake: introduce a new policy scope, set min cmake version

mustafakemalgilor commented 1 month ago

cmake has the notion of "policy scopes" to limit the scope of the policies. this patch introduces a module-level policy scope and sets the minimum required cmake version to 3.24, so the cmake features depend on policy settings such as if(... IN_LIST ...) behaves as expected even if the consuming project does not declare a cmake minimum version requirement, or the declared version is less than the Conan's expected minimum version (e.g. cmake_minimum_required(VERSION 3.0).

this change now enforces the version requirement so the code would fail immediately if the cmake version is less than the minimum.

Below is a simple reproducer:

reproducer.tar.gz

Extract the reproducer.tar.gz file, go into conan-cmake-repr folder, and type cmake --preset configure. The cmake output should contain the error given below:

CMake Error at conan-provider.cmake:540 (if):
  if given arguments:

    "default" "IN_LIST" "CONAN_HOST_PROFILE" "OR" "default" "IN_LIST" "CONAN_BUILD_PROFILE"

  Unknown arguments specified
Call Stack (most recent call first):
  CMakeLists.txt:6 (find_package)

To verify that the patch fixes the issue, simply replace the conan-provider.cmake with the conan-provider.cmake from the patch.

CLAassistant commented 1 month ago

All committers have signed the CLA.

mustafakemalgilor commented 1 month ago

@memsharded I discovered this by accident while migrating one of my projects from Conan 1.x to Conan 2. I thought that having a CMake version check without interfering with consuming project's settings would be a nice thing to have. I'd appreciate it if you could review this and let me know what you think.

memsharded commented 1 month ago

Hi @mustafakemalgilor

Thanks for your contribution

I have been testing your reproducer and I have checked the errors and that it is fixed with your proposal. I am a bit concerned about possible side effects or unexpected breakages for users. I have been trying to look for information about the scoping of code provided by CMake dependency providers, but I haven't found anything explicit. It seems that even if this code is "delayed" until "find_package()" is called, when it is called it applies the right scope for the called code.

Do you have any feedback or further insights about what could go wrong? Possible risks or side effects? Thanks for your feedback!

mustafakemalgilor commented 1 month ago

Hi @mustafakemalgilor

Thanks for your contribution

I have been testing your reproducer and I have checked the errors and that it is fixed with your proposal. I am a bit concerned about possible side effects or unexpected breakages for users. I have been trying to look for information about the scoping of code provided by CMake dependency providers, but I haven't found anything explicit. It seems that even if this code is "delayed" until "find_package()" is called, when it is called it applies the right scope for the called code.

Do you have any feedback or further insights about what could go wrong? Possible risks or side effects? Thanks for your feedback!

Hi @memsharded

I have no concerns regarding this as it is the intended use of the feature. To further elaborate on it; this is the commit that introduces the policy scope feature for include/find_package. It's been quite a long time since this feature was introduced, and there are a handful of official CMake modules utilizing this feature so I'd say it's pretty safe to use it.

There's also policy scope test case in CMake's test suite so that might be also helpful to understand the interaction between the policies, dependency providers, and the user code.

I think even explicit push-pop here might be overkill because it seems CMake is already doing that for both include() and find_package() automatically so everything should work fine -- quoting CMake Policy Stack Doc:

CMake also manages a new entry for scripts loaded by include() and find_package() commands except when invoked with the NO_POLICY_SCOPE option (see also policy CMP0011).

Nevertheless, I think having explicit push/pop won't hurt. The documentation for CMP0011 mentions that the policy behavior is set to OLD by default for cmake_minimum_required(VERSION 2.6.2) and below, in which case CMake recommends having the explicit push-pop. I don't know how much CMake v2 is relevant today, but you can never know what you can find in the wild.

mustafakemalgilor commented 1 month ago

Do you have any feedback or further insights about what could go wrong? Possible risks or side effects? Thanks for your feedback!

I also noticed that the official dependency provider examples are also using the cmake_minimum_required() call: https://cmake.org/cmake/help/latest/command/cmake_language.html#provider-examples

mustafakemalgilor commented 1 month ago

Added a test case, just to be sure.

memsharded commented 1 month ago

https://github.com/Kitware/CMake/commit/c332e0bf3c4e619358322bd0d0961af45653eb5b

Yes, but it is a bit different that a find_package() creates a new scope, than the file that contains the provider that contains the find_package replacement has its own scope. I understand that xxx-config.cmake have their own scope because of that, but it is not the find_package() provider alternative the one pushing a new scope, it was the file containing such provider.

https://github.com/Kitware/CMake/blob/master/Tests/PolicyScope/CMakeLists.txt

Same with the test, it is covering explicitly include() and find_package() but not the provider case.

I am not opposed to this, I am just trying to understand if there are any risks, and it was not that evident, or explicitly stated about providers+scopes.

I think even explicit push-pop here might be overkill because it seems CMake is already doing that for both include()

So basically the question is that if the -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES=conan_provider.cmake is using exactly the same include() mechanism that the CMakeList.txt would use. If that is the case, I think we can move forward, even without the PUSH-POP, to keep it clear, if it works since ancient versions like 2.6.2, no need to make it explicit.

mustafakemalgilor commented 1 month ago

So basically the question is that if the -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES=conan_provider.cmake is using exactly the same include() mechanism that the CMakeList.txt would use.

I see your concern. So I did some research and came up with the following test code:

test-code.tar.gz

To summarize, there are three conan-provider files to test different approaches and their effects:

one with policy push-pop (conan-provider-with-policy-push-pop.cmake)
one without policy push-pop (conan-provider-without-policy-push-pop.cmake)
one with block()/endblock() instead of push-pop (conan-provider-with-block.cmake)

Here's the summary of the results:

conan-provider-with-policy-push-pop.cmake:
- confines the policy, but leaks the CMAKE_MINIMUM_REQUIRED_VERSION variable to user code
- cmake configure on user code without a cmake_minimum_required(VERSION) would succeed, but CMake would issue a WARNING
conan-provider-without-policy-push-pop.cmake:
- leaks both the policy settings and the CMAKE_MINIMUM_REQUIRED_VERSION variable to user code
- cmake configure on user code without a cmake_minimum_required(VERSION) would fail with an ERROR
conan-provider-with-block.cmake:
- confines both the policy and the variables, nothing leaks
- cmake configure on user code without a cmake_minimum_required(VERSION) would succeed, but CMake would issue a WARNING

My conclusion is:

The absolute best is block()/endblock(), but requires a cmake_minimum_requires version bump to 3.25
policy push/pop approach is also acceptable as it only leaks CMAKE_MINIMUM_REQUIRED_VERSION and policies behave as expected
without policy push/pop approach is a no-go as it both leaks the policy settings and the variable, even if the "cmake configure" fails in the end

The test commands are as follows:

❯ cmake --version
cmake version 3.28.3

`cmake configure` without any dependency providers:

cmake -B build -DCMAKE_BUILD_TYPE=Release
# output:
# -- before project() ==> user cmake min req version is: 
# -- before project() ==> POLICY 0057 is

`cmake configure` with `conan-provider-with-policy-push-pop` dependency provider:

cmake -B build -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES=conan-provider-with-policy-push-pop.cmake -DCMAKE_BUILD_TYPE=Release
# -- before project() ==> user cmake min req version is: 
# -- before project() ==> POLICY 0057 is 
# CMake Warning (dev) at CMakeLists.txt:9 (project):
#   cmake_minimum_required() should be called prior to this top-level # project()
#   call.  Please see the cmake-commands(7) manual for usage # documentation of
#   both commands.
# This warning is for project developers.  Use -Wno-dev to suppress it.
# 
# --    in dependency provider() ==> user cmake min req version is: 3.24
# --    in dependency provider() ==> POLICY 0057 is NEW
# /*...*/
# -- after project() ==> user cmake min req version is: 3.24
# -- after project() ==> POLICY 0057 is 
# /*...*/
# CMake Warning (dev) in CMakeLists.txt:
#   No cmake_minimum_required command is present.  A line of code such as
# 
#     cmake_minimum_required(VERSION 3.28)
# 
#   should be added at the top of the file.  The version specified may be lower
#   if you wish to support older CMake versions for this project.  For more
#   information run "cmake --help-policy CMP0000".
# This warning is for project developers.  Use -Wno-dev to suppress it.

`cmake configure` with `conan-provider-without-policy-push-pop` dependency provider:

cmake -B build -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES=conan-provider-without-policy-push-pop.cmake -DCMAKE_BUILD_TYPE=Release

# -- before project() ==> user cmake min req version is: 
# -- before project() ==> POLICY 0057 is 
# CMake Warning (dev) at CMakeLists.txt:9 (project):
#   cmake_minimum_required() should be called prior to this top-level project()
#   call.  Please see the cmake-commands(7) manual for usage documentation of
#   both commands.
# This warning is for project developers.  Use -Wno-dev to suppress it.
# 
# --  in dependency provider() ==> user cmake min req version is: 3.24
# --  in dependency provider() ==> POLICY 0057 is NEW
# /*...*/
# -- after project() ==> user cmake min req version is: 3.24
# -- after project() ==> POLICY 0057 is NEW
# /*...*/
# CMake Error in CMakeLists.txt:
#   No cmake_minimum_required command is present.  A line of code such as
# 
#     cmake_minimum_required(VERSION 3.28)
# 
#   should be added at the top of the file.  The version specified may be lower
#   if you wish to support older CMake versions for this project.  For more
#   information run "cmake --help-policy CMP0000".
# 
# 
# -- Configuring incomplete, errors occurred!

`cmake configure` with `conan-provider-with-block` dependency provider:

cmake -B build -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES=conan-provider-with-block.cmake -DCMAKE_BUILD_TYPE=Release

# -- before project() ==> user cmake min req version is: 
# -- before project() ==> POLICY 0057 is 
# CMake Warning (dev) at CMakeLists.txt:9 (project):
#   cmake_minimum_required() should be called prior to this top-level project()
#   call.  Please see the cmake-commands(7) manual for usage documentation of
#   both commands.
# This warning is for project developers.  Use -Wno-dev to suppress it.
# 
# --    in dependency provider() ==> user cmake min req version is: 3.24
# --    in dependency provider() ==> POLICY 0057 is NEW
# /*...*/
# -- after project() ==> user cmake min req version is: 
# -- after project() ==> POLICY 0057 is 
# /*...*/
# CMake Warning (dev) in CMakeLists.txt:
#   No cmake_minimum_required command is present.  A line of code such as
# 
#     cmake_minimum_required(VERSION 3.28)
# 
#   should be added at the top of the file.  The version specified may be lower
#   if you wish to support older CMake versions for this project.  For more
#   information run "cmake --help-policy CMP0000".
# This warning is for project developers.  Use -Wno-dev to suppress it.

memsharded commented 1 month ago

Thanks very much for your extensive research and explanation, this is fantastic!

I think I'd tend to go the conan-provider-with-policy-push-pop.cmake: way, leakage is minimum and we don't force an upgrade to 3.25, because we already defined 3.24 as the pre-requisite.

But just very slightly, I am not opposed to bump the 3.24 to 3.25 requisite, lets wait for @jcar87 feedback.

conan-io / cmake-conan