fix(cmake): upgrade maximum supported CMake version to 3.27

[polmes]

Description

This PR fixes the issue from #4785 by upgrading the maximum supported CMake version from 3.26 to 3.27. This allows the checks for the CMP0148 policy in pybind11Common.cmake to work as expected under CMake 3.27+, defaulting to the modern FindPython CMake modules.

This means that projects using CMake 3.27+ no longer need to set

set(PYBIND11_FINDPYTHON ON)

to avoid warnings when using pybind11.

Additionally, projects that still rely on the "classic" CMake mode can now set

cmake_policy(SET CMP0148 OLD)

and pybind11 will then fall back to the deprecated FindPythonInterp and FindPythonLibs modules logic.

Suggested changelog entry:

* Upgrade maximum supported CMake version to 3.27 to fix CMP0148 warnings

[rwgk]

Looks good to me, but

• as I wrote before I don't know a lot about cmake, so definitely let's wait a bit for @henryiii to comment.
• and WOW, from a general software engineering perspective it seems very unfortunate that we have to change 10 files just to bump the maximum supported cmake version.

Definitely for a separate PR, but is there a way to improve the organization? Could we define the maximum supported version in one place for cmake, and in the documentation refer to that rather than duplicating it there as well?

[henryiii]

Apologies for the delay.

The single most important rule of CMake: upgrading CMake can never change your build. CMake 3.27 produces the exact same output as whatever was set as the maximum or minimum version of cmake. Otherwise, you'd not be able to update your system CMake without breaking builds.

We can't "auto-switch" to FindPython because many packages use pybind11 then expect all the PYTHON_* variables to be set, and will break if they are not. Also, FindPython is still quite new - you need at least 3.17 to get manylinux support, 3.18.2 to get decent PyPy support, or 3.26 for several key bug fixes. That's not a problem if you are using scikit-build-core, since it backports newer FindPython for you to 3.15, but is a big issue if you are making a general package; you might get better results with the classic mechanism for <3.26 or <3.18.

The bug here is that we were supposed to detect if a user had set a minimum or maximum to 3.27, but we are setting the maximum to 3.26 if we are used as a submodule (find_package CONFIG, which is how scikit-build-core recommends using pybind11, works correctly). To fix this, we need to detect this before our cmake requires call, or we could remove the requires call if this is a subpackage.

[henryiii]

Updating all these is a good idea, though. :)

[polmes]

and WOW, from a general software engineering perspective it seems very unfortunate that we have to change 10 files just to bump the maximum supported cmake version.

To be fair, most of the changes were in the documentation and the tests since I figured might as well get that updated, the only place where the change was really necessary was in the root CMakeLists.txt file. It could make sense to create some sort of PYBIND11_CMAKE_MAX_VERSION variable that we can set in the main file and reuse across all tests however.

[polmes]

Also, I see that one of the CI pipelines failed. However, I'm not sure how the error in that particular macOS build:

/usr/local/opt/python@3.11/bin/python3.11: No module named pytest

is related to my changes. Was that unexpected?

[rwgk]

is related to my changes. Was that unexpected?

Sorry I missed this question before: I don't think I've seen that failure, i.e. unexpected, but I also believe it's not related to the changes in this PR. Could you please git merge master, git push, to see if that resolves the issue? (please don't rebase or force push; that only makes it more tricky to follow the history; and when we merge this PR it will be squashed then)

Do you want me to make any other changes then? (like maybe setting a CMake variable to control the maximum CMake version

I have a split mind: generally I think it's best to solve one problem at a time (i.e. have one PR for the bug fix, another for the better organization) ... but in this particular case, since you're replacing all 3.26 with 3.27 anyway, how much does it add to this PR to define and use PYBIND11_CMAKE_MAX_VERSION instead?

[polmes]

I have a split mind: generally I think it's best to solve one problem at a time (i.e. have one PR for the bug fix, another for the better organization) ... but in this particular case, since you're replacing all 3.26 with 3.27 anyway, how much does it add to this PR to define and use PYBIND11_CMAKE_MAX_VERSION instead?

I thought it would be pretty straightforward, but because the test CMakeLists.txt files are all creating new CMake "projects" that are designed to then include pybind11, simply setting a variable at the root level of the project will not propagate to the test subdirectories.

There are definitely workarounds like having a separate file that defines the version and we parse that in each individual CMakeLists.txt file, but I don't know if that's something we want to do and I don't think it's very elegant.

I have also merged master into this branch. Let's see what happens with the CI pipelines.

[polmes]

Ok, the Mac / brew build failed again. Upon further inspection it looks like this is indeed caused by the changes here. Comparing with a previous successful pipeline, after the changes to prefer the modern FindPython on CMake 3.27+, the build fails because it is using a different Python installation.

Successful pipeline during CMake configure:

CMake Warning (dev) at tools/FindPythonLibsNew.cmake:98 (find_package):
  Policy CMP0148 is not set: The FindPythonInterp and FindPythonLibs modules
  are removed.  Run "cmake --help-policy CMP0148" for policy details.  Use
  the cmake_policy command to set the policy and suppress this warning.

Call Stack (most recent call first):
  tools/pybind11Tools.cmake:50 (find_package)
  tools/pybind11Common.cmake:188 (include)
  CMakeLists.txt:211 (include)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Found PythonInterp: /usr/local/bin/python3 (found suitable version "3.11.4", minimum required is "3.6") 
-- Found PythonLibs: /Library/Frameworks/Python.framework/Versions/3.11/lib/libpython3.11.dylib
-- PYTHON 3.11.4

Failing pipeline during CMake configure:

-- CMake 3.27.2
-- Found Python: /usr/local/Frameworks/Python.framework/Versions/3.11/bin/python3.11 (found suitable version "3.11.4", minimum required is "3.6") found components: Interpreter Development Development.Module Development.Embed 
-- Python 3.11.4

After that, when it tries to go and run the Python tests via

cd /Users/runner/work/pybind11/pybind11/tests && /usr/local/Frameworks/Python.framework/Versions/3.11/bin/python3.11 -m pytest ...

it fails because it does not find the pytest module in this environment.

However, during the pip install step, there do not appear to be any unusual errors and it says that pytest was installed successfully for that same Python version!

At this point, I'm not quite sure what to do. For some reason:

• Modern FindPython is choosing a different Python installation than the legacy mode, which is quite confusing and appears to be the source of the problem, but I can't really tell why
• Even when using the new version, with all dependencies theoretically installed correctly, CMake does not appear to know that pytest is installed

[rwgk]

I'd try adding (in ci.yml)

--DPYTHON_EXECUTABLE=/usr/local/bin/python3

to see if that helps to get us unstuck. If that's the only thing, maybe report a cmake bug. "brew install llvm" is a pretty unusual environment, it wouldn't be surprising if something subtle goes wrong.

[rwgk]

This seems to be the problem:

• https://github.com/pybind/pybind11/actions/runs/5912963894/job/16036906096?pr=4786

Installing collected packages: tomli, py, pluggy, pep517, packaging, iniconfig, attrs, pytest, build, pytest-timeout
  WARNING: The scripts py.test and pytest are installed in '/Library/Frameworks/Python.framework/Versions/3.11/bin' which is not on PATH.
  Consider adding this directory to PATH or, if you prefer to suppress this warning, use --no-warn-script-location.
  WARNING: The script pyproject-build is installed in '/Library/Frameworks/Python.framework/Versions/3.11/bin' which is not on PATH.
  Consider adding this directory to PATH or, if you prefer to suppress this warning, use --no-warn-script-location.
Successfully installed attrs-23.1.0 build-0.8.0 iniconfig-2.0.0 packaging-23.1 pep517-0.13.0 pluggy-1.2.0 py-1.11.0 pytest-7.0.0 pytest-timeout-2.1.0 

But why is that not a problem without this PR? — I'd feel paranoid enough at this point to open another PR with current master and a no-op trivial change. Does it actually still work? (I guess yes, but I'd do that low-effort sanity check first.)

[polmes]

I thought so too at first, but you can see that same warning on the previous build that was successful, so I'm thinking it's something else.

Looking at the logs, even after I changed the Python executable path for the macos_latest_brew_instal_llvm workflow, the CMake configuration step still reports that it is finding a different Python version:

-- Found Python: /usr/local/Frameworks/Python.framework/Versions/3.11/bin/python3.11 (found suitable version "3.11.4", minimum required is "3.6") found components: Interpreter Development Development.Module Development.Embed 

Furthermore, on this same latest CI run, an additional build failed. In this case, the processed failed during the pip installs with the following error:

ERROR: THESE PACKAGES DO NOT MATCH THE HASHES FROM THE REQUIREMENTS FILE. If you have updated the package versions, please update the hashes. Otherwise, examine the package contents carefully; someone may have tampered with them.
    scipy==1.5.4 from https://files.pythonhosted.org/packages/42/ba/ff96cb42abe35057ed3435edf05e6fa76d9dbc1983c9c5a18162fc154545/scipy-1.5.4-cp39-cp39-win32.whl (from -r tests/requirements.txt (line 8)):
        Expected sha256 d84cadd7d7998433334c99fa55bcba0d8b4aeff0edb123b2a1dfcface538e474
             Got        c343eb4b72a86b7fd631a4908c9373e714aae55ae3a54fdf4ef1f2818941e4b2

[polmes]

Ok, actually after doing some reading, the CMake variable is actually Python_EXECUTABLE, not PYTHON_EXECUTABLE, so I'm going to try this change now real quick and see if that does the trick.

Unless I'm missing something, this means that this line in ci.yml that appears in several workflows

        -DPYTHON_EXECUTABLE=$(python3 -c "import sys; print(sys.executable)")

actually has no effect, so it could be removed without any consequences.

[rwgk]

I just clicked the button to start the GHAs. Sorry it took me a while to get back here. There is a simple trick to get around that: include a trivial whitespace change in your commit that is detected and fixed by our pre-commit checks on push. When the pre-commit bot pushes the commit with the fix, GHA will run automatically. The only thing to remember is to git pull the whitespace fix before continuing with changes in your local repo.

For example, this will do the trick:

-cmake_minimum_required(VERSION 3.5)
+cmake_minimum_required(VERSION  3.5)

[rwgk]

@henryiii do you know if there is a story behind -DPYTHON_EXECUTABLE vs -DPython_EXECUTABLE. Was the old all-upper-case version how it was spelled in the past or something? I looked around a bit but didn't find an answer.

[rwgk]

@polmes wrote:

There are definitely workarounds like having a separate file that defines the version and we parse that in each individual CMakeLists.txt file, but I don't know if that's something we want to do and I don't think it's very elegant.

@henryiii I'm really surprised by the breakdown of such a basic abstraction. Considering that this PR currently is essentially just replacing 3.26 with 3.27, introducing PYBIND11_CMAKE_VERSION_MAX or similar would seem far better. How are other people handling this?

[rwgk]

Regarding this failure:

https://github.com/pybind/pybind11/actions/runs/5913710581/job/16048426600?pr=4786

ERROR: THESE PACKAGES DO NOT MATCH THE HASHES FROM THE REQUIREMENTS FILE. If you have updated the package versions, please update the hashes. Otherwise, examine the package contents carefully; someone may have tampered with them.
    scipy==1.5.4 from https://files.pythonhosted.org/packages/42/ba/ff96cb42abe35057ed3435edf05e6fa76d9dbc1983c9c5a18162fc154545/scipy-1.5.4-cp39-cp39-win32.whl (from -r tests/requirements.txt (line 8)):
        Expected sha256 d84cadd7d7998433334c99fa55bcba0d8b4aeff0edb123b2a1dfcface538e474
             Got        be6f6f739393f1b044509a836a7a56a1738072f152a7239251f4f84b2ff5e853

I'm thinking let's just ignore it and hope it'll get fixed in the scipy project soon.

[henryiii]

For the classic FindPythonInterp, like the rest of CMake, all variables start with PYTHON_. In order to not collide with these, FindPython chose to use Python_ as the prefix for all variables. So if you are using the old system, you have to use PYTHON_*, and new system, Python_*.

We have to be quite careful here to be completely backward compatible. Upgrading your CMake version should never break your build. That's why CMake 3.27 does not remove FindPythonInterp/FindPythonLibs unless you set your minimum or maximum version of CMake to 3.27, which acknowledges that you know about this change. We need to follow the same pattern and avoid changing anything for users who have not opted into the new system.

There are some minor fixes that need to go in and documentation updates, so I'll work on some of those.

There are definitely workarounds like having a separate file that defines the version and we parse that in each individual CMakeLists.txt file

I'd rather stick with simple values - it's not hard at all to search & replace for them, and it keeps them looking like "normal" CMakeLists.txt's, something a user would write for themselves. The difficulty in this upgrade is that setting 3.27 causes a significant change (the removal of FindPythonLibs/FindPythonInterp) which we are having to adjust for. It's a bit tricker for us in testing because we do want to keep testing the old method too.

[polmes]

We have to be quite careful here to be completely backward compatible. Upgrading your CMake version should never break your build. That's why CMake 3.27 does not remove FindPythonInterp/FindPythonLibs unless you set your minimum or maximum version of CMake to 3.27, which acknowledges that you know about this change. We need to follow the same pattern and avoid changing anything for users who have not opted into the new system.

Of course. And I think I see the current issue here where upgrading to the new CMake version could inadvertently switch your project to the new FindPython logic, thus ignoring the CMP0148 policy in the opposite way of the current situation (would always be NEW instead of OLD). Looking at #4806 now to see if that does the trick.

[rwgk]

@henryiii

This PR broke https://github.com/pybind/pybind11_abseil testing.

The latest version of pybind11_abseil is pinned to the pybind11 commit right before this PR. Test log (passed): https://github.com/pybind/pybind11_abseil/actions/runs/7400903531/job/20135518040

pybind/pybind11_abseil#14 pins pybind11_abseil to this PR (everything else is unchanged). Test log (failed): https://github.com/pybind/pybind11_abseil/actions/runs/7400974247/job/20135725540

This command appears to no longer work correctly:

ctest --output-on-failure --extra-verbose

This seems to be the critical diff (line breaks inserted manually to obtain an easily readable diff):

  1: Test command:
  /opt/hostedtoolcache/cmake/3.28.1/x64/cmake-3.28.1-linux-x86_64/bin/cmake
  "-E"
  "env"
  "PYTHONPATH=$PYTHONPATH:/home/runner/work/pybind11_abseil/pybind11_abseil/tmp_build"
- "/home/runner/work/pybind11_abseil/pybind11_abseil/venv/bin/python"
  "/home/runner/work/pybind11_abseil/pybind11_abseil/pybind11_abseil/tests/cpp_capsule_tools_testing_test.py"

Does this ring any bells?

I already tried setting Python_EXECUTABLE in all CMakeLists.txt files (https://github.com/pybind/pybind11_abseil/pull/14/commits) but that did not help.

[henryiii]

PYTHON_INCLUDE_DIR looks suspicious. That won't be set anymore, and isn't a good idea to use - target based is better than directory based variables in modern CMake.

Can look more in a hour or two.

[david-crouse]

I am working on getting this fixed today. @henryiii do you have any updates on your end?

[rwgk]

To close the loop here: @david-crouse found this solution:

https://github.com/pybind/pybind11_abseil/blob/ecbbf718f1267f6600411a4d93840715f18455d6/pybind11_abseil/tests/CMakeLists.txt#L11-L13

if(NOT DEFINED PYTHON_EXECUTABLE)
  set(PYTHON_EXECUTABLE ${PYBIND11_PYTHON_EXECUTABLE_LAST})
endif()

@henryiii is it intentional that only PYBIND11_PYTHON_EXECUTABLE_LAST is defined (but not PYTHON_EXECUTABLE anymore)?

[henryiii]

PYBIND11_PYTHON_EXECUTABLE_LAST handling changed a bit, might be worth checking how pybind11 does this. That's not a public value, though, no one should be using it. FindPython (CMake 3.15+, and the only one if you set 3.27 as a maximum) does not define PYTHON_EXECUTABLE, but only Python_EXECUTABLE.

[henryiii]

Ahh, you added this. No, the solution would be to set PYTHON_EXECUTABLE from Python_EXECUTABLE (or, more future-ready, the other way around, since PYTHON_EXECUTABLE is the one going away).

[rwgk]

or, more future-ready, the other way around, since PYTHON_EXECUTABLE is the one going away

Thanks, done: pybind/pybind11_abseil@52f2739#diff-67de6e1b7373ae9e30ad001b2f8b58e98aafd53263a12584c10d2913c6ecf63b

The revised if block for backward compatibility is the work of @david-crouse. After some back and forth we got it to work with all cmake versions we usually test with (not sure exactly what versions).