[BUG]: Warning when using CMake 3.27, CMP0148 policy is ignored

polmes commented 1 year ago

Required prerequisites

[X] Make sure you've read the documentation. Your issue may be addressed there.
[X] Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
[ ] Consider asking first in the Gitter chat room or in a Discussion.

What version (or hash if on master) of pybind11 are you using?

v2.11.1

Problem description

The latest version of pybind11 (v2.11.1) results in a warning when configuring a project (that includes pybind11) with CMake 3.27 that reads:

[cmake] CMake Warning (dev) at extern/pybind11/tools/FindPythonLibsNew.cmake:98 (find_package):
[cmake]   Policy CMP0148 is not set: The FindPythonInterp and FindPythonLibs modules
[cmake]   are removed.  Run "cmake --help-policy CMP0148" for policy details.  Use
[cmake]   the cmake_policy command to set the policy and suppress this warning.
[cmake] 
[cmake] Call Stack (most recent call first):
[cmake]   extern/pybind11/tools/pybind11Tools.cmake:50 (find_package)
[cmake]   extern/pybind11/tools/pybind11Common.cmake:192 (include)
[cmake]   extern/pybind11/CMakeLists.txt:210 (include)
[cmake] This warning is for project developers.  Use -Wno-dev to suppress it.

This issue occurs in pybind11Tools.cmake, which is called from pybind11Common.cmake when it determines that it should be using the "classic" CMake mode that relies on the deprecated FindPythonInterp and FindPythonLibs modules as detailed here.

With the changes from #4719, CMake 3.27 should by default switch to the "new" pybind11ToolsNew.cmake, which relies on the more modern FindPython CMake modules.

However, this is not happening automatically. Furthermore, if one sets either of the following policies on the root project:

cmake_policy(VERSION 3.26)

or

cmake_policy(SET CMP0148 OLD)

the behavior is exactly the same, defaulting to the old pybind11Tools.cmake and printing the same warning about the CMP0148 policy.

Of course, looking at the logic in the code, one can easily figure out that setting

set(PYBIND11_FINDPYTHON ON)

on the root project will get around this issue, and it indeed does, so this is not a critical issue as there is a fairly simple workaround.

Having said that, pybind11 should be able to inherit the CMP0148 policy from the root project and, if one is not provided and the current CMake version is 3.27, it should automatically switch to the pybind11ToolsNew.cmake logic.

As far as I can tell, the reason why this is not happening automatically is that in the pybind11 root CMakeLists.txt, the CMake policy is being set as:

cmake_policy(VERSION 3.26)

for CMake versions >= 3.26.

Thus, when CMake tries to get the current policy value in pybind11Common.cmake with:

cmake_policy(GET CMP0148 _pybind11_missing_old_python)

the check always returns an empty string as policy CMP0148 is not defined if the CMake policy version is 3.26 (was added in 3.27).

This can easily be resolved by updating the root CMakeLists.txt (and possibly other CMakeLists.txt files in the repo) to:

cmake_policy(VERSION 3.27)

for CMake versions >= 3.27.

I can confirm that making the above change forces CMake 3.27 in my project to use the new logic from pybind11ToolsNew.cmake and the warning goes away, exactly the same way as if the PYBIND11_FINDPYTHON flag had been enabled.

I can make a PR with the changes, but I wanted to hear from @henryiii or @rwgk (from the original #4719 PR) if there was any reason why this change was not implemented at that time.

Thanks!

Reproducible example code

No response

Is this a regression? Put the last known working version here if it is.

Not a regression

rwgk commented 1 year ago

I can make a PR with the changes, but I wanted to hear from @henryiii or @rwgk (from the original https://github.com/pybind/pybind11/pull/4719 PR) if there was any reason why this change was not implemented at that time.

TL;DR: I'd say please go ahead and send a PR that resolves the warnings you're seeing.

I don't know a lot about cmake, we'll have to wait for @henryiii for a conclusive answer, but to me this looks like one of those little things that are prone to slip through the cracks. It's also something that seems very difficult to cover with tests, and since it's just a warning, not critical enough to warrant a big effort to cover all possible scenarios. My conclusion: Let's fix the obvious and go from there.

polmes commented 1 year ago

Here you go: #4786. I have bumped up all CMake 3.26 references that I found in both the main code, tests, and documentation to 3.27, thus raising the maximum CMake version supported. I haven't messed with any of the CI build scripts as those were already updated as part of #4719.

All tests pass on my local build, but of course we'll have to wait for the CI pipelines to finish to make sure no regressions were introduced.

henryiii commented 1 year ago

CMake 3.27 should by default switch to the "new" pybind11ToolsNew.cmake

No. It should only switch if a user specifies 3.27 as their minimum or maximum CMake version, thereby acknowledging they are aware of the change. If you don't do that, then it shouldn't change behavior, which would break existing packages. CMake updates do not break existing packages.

99% percent of users should be specifying set(PYBIND11_FINDPYTHON ON) if they understand the consequences of using the modern FindPython. Or you can find_package(Python ...). before finding this package, which might look nicer.

Having said that, pybind11 should be able to inherit the CMP0148 policy from the root project and, if one is not provided and the current CMake version is 3.27, it should automatically switch to the pybind11ToolsNew.cmake logic.

It should not auto-switch if one is not set. Users need to respond to this warning, it's there to help them. However, if it is set, yes, it needs to inherit it, that's a bug.

polmes commented 1 year ago

Sorry, I don't think I was very precise with my language when I wrote that. What I meant is that if the current CMake minimum/maximum version is 3.27, i.e.:

cmake_minimum_required(VERSION 3.27...3.27)

then pybind11 should inherit the CMP0148 policy (which would be set to NEW by CMake 3.27), thus "automatically" switching to the modern FindPython.

Due to the current bug where pybind11 is not inheriting that setting from the root project, that is not happening either, and you are right that that is what the bug actually is.

pybind / pybind11