pybind / cmake_example

Example pybind11 module built with a CMake-based build system
Other
613 stars 218 forks source link

Fix incorrect letter case for Python_EXECUTABLE option #102

Closed mnixry closed 1 year ago

mnixry commented 1 year ago

CMake's variables are case-sensitive. In some cases, the full uppercased PYTHON_EXECUTABLE will not work and leads to compile failing (for example, causes NumPy not found) According to document, it should be Python_EXECUTABLE (Although it looks weird).

henryiii commented 1 year ago

You are looking at the wrong document. It's https://cmake.org/cmake/help/latest/module/FindPythonInterp.html (and this is why they went with the weird caps on the replacement).

mnixry commented 1 year ago

You are looking at the wrong document. It's cmake.org/cmake/help/latest/module/FindPythonInterp.html (and this is why they went with the weird caps on the replacement).

Oh, thank you for you remind. I had assumed it is using FindPython not FindPythonInterp.

However, according to the document, it seems the FindPythonInterp has been deprecated since CMake >= 3.12, and the document of Pybind11 has changed the recommended Python-finding module to FindPython.

So I think update the make list to the FindPython will be a better choice

henryiii commented 1 year ago

You should use https://github.com/pybind/scikit_build_example if you want to use FindPython. Scikit-build-core backports FindPython so you can use it on 3.15+ and still get things like PyPy support. That also fixes a bunch of issues present with the setuptools hacks in cmake_example, like support for platforms without cmake/ninja wheels (Cygwin, ClearLinux, BSD, Android, etc).

If someone is really stuck with this example, they are probably stuck with really old CMake, etc. You really want CMake 3.19+ (if you have pybind11) or the (unreleased) CMake 3.26+ (if you want to use it directly) to use FindPython.

mnixry commented 1 year ago

Thank you for your explanation. I think scikit-build maybe the things I actually needed.

I will close this PR now.