Closed polmes closed 7 months ago
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.
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.
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?
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?
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.
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:
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 whypytest
is installedI'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.
This seems to be the problem:
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.)
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
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.
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)
@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.
@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?
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.
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.
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.
@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
https://github.com/pybind/pybind11_abseil/pull/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.
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.
I am working on getting this fixed today. @henryiii do you have any updates on your end?
To close the loop here: @david-crouse found this solution:
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)?
That's not a public value, though, no one should be using it. PYBIND11_PYTHON_EXECUTABLE_LAST
handling changed a bit, might be worth checking how pybind11 does this.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
.
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).
or, more future-ready, the other way around, since
PYTHON_EXECUTABLE
is the one going away
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).
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 modernFindPython
CMake modules.This means that projects using CMake 3.27+ no longer need to set
to avoid warnings when using pybind11.
Additionally, projects that still rely on the "classic" CMake mode can now set
and pybind11 will then fall back to the deprecated
FindPythonInterp
andFindPythonLibs
modules logic.Suggested changelog entry: