pybind11: add version 3.0.0 - #28174
Conversation
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
uilianries
left a comment
There was a problem hiding this comment.
@tttapa Thank you for your PR!
The new major version comes with a ton of changes, but in terms of building, it looks okay: pybind/pybind11@v2.13.6...v3.0.0#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20a
I also made a few changes in your PR, adapting to a lighter revision. Cheers!
tttapa
left a comment
There was a problem hiding this comment.
I've added two comments and pushed corresponding commits to address them. Let me know if any further changes are necessary.
| cmake_minimum_required(VERSION 3.15) | ||
| project(test_package LANGUAGES CXX) | ||
|
|
||
| find_package(PythonInterp REQUIRED) |
There was a problem hiding this comment.
It may be better to use the FindPython module instead of the (long deprecated) FindPythonInterp module:
https://pybind11.readthedocs.io/en/stable/cmake/index.html#modes
| cmake.build() | ||
|
|
||
| @property | ||
| def _python_interpreter(self): |
There was a problem hiding this comment.
This doesn't work for me because I have multiple versions of Python installed. CMake locates Python 3.14, whereas python3 in the PATH is Python 3.10 (this is the system-wide default on Ubuntu 22.04). To fix this, we should use the ${Python_EXECUTABLE} selected by CMake to run the tests.
There was a problem hiding this comment.
@tttapa We have multiple Python versions installed in the CI, too. In order to make sure which version should be used, we manipulate the PATH in the profile:
[buildenv]
PATH=+(path)/opt/pyenv/versions/3.12.7/bin/
[runenv]
PATH=+(path)/opt/pyenv/versions/3.12.7/bin/
So we know Python 3.12 will be found first. Please, try a similar approach. Using a well-configured profile ensures you will have no surprises in case you install a new Python version on your machine. Personally, I use pyenv, so I have a bunch of Python versions installed too, so I used profiles to avoid messing around, as usually I need to run more than one Python version when developing Conan or running specific tests.
There was a problem hiding this comment.
CMake locates Python 3.14, whereas python3 in the PATH is Python 3.10 (this is the system-wide default on Ubuntu 22.04).
Interesting! this seems odd - though I think CMake tries to locate the most recent python it can find rather than the "default in the current context" one.
If you tweak the test package to pass Python_FIND_UNVERSIONED_NAMES set to FIRST, does it now work? We can probably tweak these variables to make it more consistent for everyone:
Python_FIND_UNVERSIONED_NAMESPython_FIND_STRATEGY(though I think it this already behaves as we want for policy level >=3.15
There was a problem hiding this comment.
we manipulate the PATH in the profile
Unfortunately, this trick does not work if multiple Python versions are installed with the same prefix (e.g. /usr or /usr/local).
I'd argue that it is desirable to have the build and tests succeed without requiring users to tweak their profiles (to modify the path or set FindPython hints). One way to achieve this is by using Python_EXECUTABLE in CMake as in 43663bc, but I'm open to alternatives, of course.
There was a problem hiding this comment.
does it work correctly setting Python_FIND_UNVERSIONED_NAMES to First? it should hopefully get CMake to find the first python that it can find PATH
There was a problem hiding this comment.
Yes, adding toolchain.variables["Python_FIND_UNVERSIONED_NAMES"] = "FIRST" also works at 1524c40. CMake then selects Python 3.10 as expected.
|
|
||
| def generate(self): | ||
| tc = CMakeToolchain(self) | ||
| tc.variables["PYBIND11_FINDPYTHON"] = True |
There was a problem hiding this comment.
@tttapa Why do you need PYBIND11_FINDPYTHON only now? As far as I see, it's active by default: https://github.com/pybind/pybind11/blob/v3.0.0/CMakeLists.txt#L86. What error are you trying to mitigate?
There was a problem hiding this comment.
I added it for consistency between the build and the test_package. Setting this variable ensures that they both use FindPython. Otherwise, users may need to provide CMake hints for both FindPythonInterp and FindPython if they want to select a specific interpreter, and a different interpreter may be found during the build and the tests (which AFAICT is fine in the case of pybind11, but it could lead to confusion).
It is indeed active by default in recent versions of pybind11, but the recipe still supports versions <3.12.0, where FindPython is not yet the default.
It also gets rid of the following CMake warning during the build:
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:180 (include)
CMakeLists.txt:206 (include)|
|
||
| def generate(self): | ||
| tc = CMakeToolchain(self) | ||
| tc.variables["PYBIND11_FINDPYTHON"] = True |
There was a problem hiding this comment.
@tttapa Why do you need PYBIND11_FINDPYTHON only now? As far as I see, it's active by default: https://github.com/pybind/pybind11/blob/v3.0.0/CMakeLists.txt#L86. What error are you trying to mitigate?
| project(test_package LANGUAGES CXX) | ||
| enable_testing() | ||
|
|
||
| find_package(Python REQUIRED COMPONENTS Development OPTIONAL_COMPONENTS Interpreter) |
There was a problem hiding this comment.
Interpreter should be required; otherwise, we will testing nothing in case it's missing.
There was a problem hiding this comment.
The interpreter could be missing when cross-compiling, for example. How should such cases be handled? Explicitly checking for CMAKE_CROSSCOMPILING?
Related: pybind/pybind11#5083 and https://gitlab.kitware.com/cmake/cmake/-/issues/26696
There was a problem hiding this comment.
We apply can_run() to avoid running tests when cross-building. Please, check https://github.com/conan-io/conan-center-index/blob/master/docs/package_templates/cmake_package/all/test_package/conanfile.py#L23
There was a problem hiding this comment.
We apply
can_run()to avoid running tests when cross-building. Please, check https://github.com/conan-io/conan-center-index/blob/master/docs/package_templates/cmake_package/all/test_package/conanfile.py#L23
The problem is that if the interpreter isn't found when cross-building, the test project (the bit that can be crossbuilt), won't be crossbuilt if the interpreter is marked as required, so I think the logic as it is is correct.
|
@uilianries do you need me to modify or revert any of these changes? Thanks! |
|
@tttapa Thank you for your PR and detailed changes. |
* pybind11: add version 3.0.0 * Simplify python interpreter location for testing Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Use python3 for unix Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Use Python interpreter found by cmake Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Stop maintaining old versions Signed-off-by: Uilian Ries <uilianr@jfrog.com> * pybind11: use FindPython module instead of deprecated FindPythonInterp * pybind11: use Python interpreter selected by CMake for tests --------- Signed-off-by: Uilian Ries <uilianr@jfrog.com> Co-authored-by: Uilian Ries <uilianr@jfrog.com>
* pybind11: add version 3.0.0 * Simplify python interpreter location for testing Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Use python3 for unix Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Use Python interpreter found by cmake Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Stop maintaining old versions Signed-off-by: Uilian Ries <uilianr@jfrog.com> * pybind11: use FindPython module instead of deprecated FindPythonInterp * pybind11: use Python interpreter selected by CMake for tests --------- Signed-off-by: Uilian Ries <uilianr@jfrog.com> Co-authored-by: Uilian Ries <uilianr@jfrog.com>
* pybind11: add version 3.0.0 * Simplify python interpreter location for testing Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Use python3 for unix Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Use Python interpreter found by cmake Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Stop maintaining old versions Signed-off-by: Uilian Ries <uilianr@jfrog.com> * pybind11: use FindPython module instead of deprecated FindPythonInterp * pybind11: use Python interpreter selected by CMake for tests --------- Signed-off-by: Uilian Ries <uilianr@jfrog.com> Co-authored-by: Uilian Ries <uilianr@jfrog.com>
Summary
Changes to recipe: pybind11/3.0.0
Motivation
Adds the latest version: https://github.com/pybind/pybind11/releases/tag/v3.0.0
Details
The new version has been added to
config.ymlandconandata.yml.One of the patches to
tools/pybind11Common.cmakeis no longer necessary since theif(TARGET ...)guard logic was removed in pybind/pybind11@28dbce4.