pybind / pybind11

Seamless operability between C++11 and Python
https://pybind11.readthedocs.io/
Other
15.18k stars 2.06k forks source link

fix(cmake): correctly detect FindPython policy and better warning #4806

Closed henryiii closed 10 months ago

henryiii commented 11 months ago

Description

This is a fix for https://github.com/pybind/pybind11/issues/4785. We were not detecting the status of this policy correctly.

I'll likely update documentation a bit too later (not this PR).

Suggested changelog entry:

- Correctly detect CMake FindPython removal when used as a subdirectory.
henryiii commented 11 months ago

We don't really test this, since you need to include the repo as a submodule, etc - @polmes, is it possible for you to test this locally? What I'm doing here is a little weird, so would be nice to see if it works.

polmes commented 11 months ago

Just tested this locally. It doesn't work as implemented because of the line

cmake_policy(VERSION 3.26)

that comes after setting the CMP0148 policy to the root project's value. After that line, the value is reset and if you try to retrieve its value (i.e., cmake_policy(GET CMP0148 ...)), it will be empty.

However, moving the following code block you added

if(_pybind11_cmp0148)
  cmake_policy(SET CMP0148 ${_pybind11_cmp0148})
  unset(_pybind11_cmp0148)
endif()

below the cmake_policy version call does the trick. With this change, I was just able to confirm that in CMake 3.27 the following scenarios work as follows (which I believe is as intended):


Having said that, this made me think. Can't we go a step further and just get rid of all

if(${CMAKE_VERSION} VERSION_LESS 3.XX)
  cmake_policy(VERSION ${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION})
else()
  cmake_policy(VERSION 3.XX)
endif()

calls in pybind11's CMake code altogether? This way we would not be blindly setting policies and overriding other values from the root project. Instead, we can deal with each individual policy as needed (just like it was done here for CMP0148). The advantage from this switch would be that we no longer need to worry about "upgrading" or keeping track of the maximum CMake version supported anywhere, but I might be missing some other downside. It is also a bit odd to keep that line that sets the maximum CMake policy version to 3.26 when we are actually supporting features from 3.27.

(Keep in mind that a quick search in pybind11 reveals no other uses of cmake_policy directly.)

I can make a PR with these changes if you want to review what that would look like.

henryiii commented 10 months ago

Ahh, yes, I put this too high up. Thanks! Will move.

Can't we go a step further and just get rid of all

I suggested this originally, but was a bit more worried about doing it, since it's unusual, as far as I know, to not have a "cmake_minimum_policy(VERSION 3.5....3.26)" statement (this block really is just that, but written out due to a bug in an old MSVC fork of CMake). This statement always blindly sets all policies. Then again, CMake wasn't really designed to use add_subdirectory to include projects like this, even though it's now become fairly common.

Usually, if a new policy that affects us pops up, then this protects our code to keep it still working, even if a project including us updates their max (or min) version. The downside is that it becomes inconsistent; if a user sets max to 3.19, for example, they probably want everything to follow that, while we will still follow 3.26 (currently). But a user doesn't have to update this value if it breaks something.

Given there are downsides to both, I think what we are doing now is more common, so probably the least surprising. I think. If there was any indication that others are doing it the "only set if main project" way, we could switch, I just don't really want to be first in this. :)

It is also a bit odd to keep that line that sets the maximum CMake policy version to 3.26 when we are actually supporting features from 3.27.

This absolutely should be updated to 3.27 (like in #4786), but best to do one thing at a time.