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

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

Merged
merged 2 commits into from Sep 15, 2023

Conversation

henryiii
Copy link
Collaborator

Description

This is a fix for #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.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii
Copy link
Collaborator Author

henryiii commented Aug 21, 2023

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

polmes commented Aug 22, 2023

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):

  • If the root project only sets a CMake minimum version < 3.27 (and no maximum version), then you get the same warning as before
  • If the root project either sets the CMake maximum version to 3.27 via cmake_minimum_version(3.XX...3.27) or cmake_policy(VERSION 3.27), or alternatively just sets the CMP0148 policy to NEW, then the policy is propagated correctly
  • If the root project sets the CMP0148 policy to OLD, then the policy is propagated correctly as well.

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

henryiii commented Aug 30, 2023

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.

@henryiii henryiii merged commit 4fb111b into pybind:master Sep 15, 2023
85 checks passed
@henryiii henryiii deleted the henryiii/fix/cmakewarnfp branch September 15, 2023 21:59
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Sep 15, 2023
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants