Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
2 of 3 tasks
polmes opened this issue Aug 12, 2023 · 4 comments
Open
2 of 3 tasks

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

polmes opened this issue Aug 12, 2023 · 4 comments
Labels
triage New bug, unverified

Comments

@polmes
Copy link
Contributor

polmes commented Aug 12, 2023

Required prerequisites

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

@polmes polmes added the triage New bug, unverified label Aug 12, 2023
@rwgk
Copy link
Collaborator

rwgk commented Aug 13, 2023

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.

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
Copy link
Contributor Author

polmes commented Aug 14, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

polmes commented Aug 15, 2023

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.

henryiii pushed a commit to polmes/pybind11 that referenced this issue Nov 1, 2023
henryiii pushed a commit to polmes/pybind11 that referenced this issue Nov 15, 2023
henryiii pushed a commit to polmes/pybind11 that referenced this issue Nov 29, 2023
henryiii added a commit that referenced this issue Dec 14, 2023
* Upgrade maximum supported CMake version to 3.27 to fix warning with CMP0148 policy (#4785)

* Update `macos_brew_install_llvm` pipeline to use expected Python installation

* Fix `Python_EXECUTABLE` Cmake variable typo

* Apply suggestions from code review

* fix: use FindPython for CMake 3.18+ by default for pybind11's tests

Signed-off-by: Henry Schreiner <[email protected]>

* tests: fix issues with finding Python

Signed-off-by: Henry Schreiner <[email protected]>

* tests: also set executable on subdir tests

Signed-off-by: Henry Schreiner <[email protected]>

* fix(cmake): correct logic for FindPython

Signed-off-by: Henry Schreiner <[email protected]>

* Update ci.yml

* Revert "Update ci.yml"

This reverts commit 33798ad.

---------

Signed-off-by: Henry Schreiner <[email protected]>
Co-authored-by: Henry Schreiner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage New bug, unverified
Projects
None yet
Development

No branches or pull requests

3 participants