pybind / pybind11

Seamless operability between C++11 and Python
https://pybind11.readthedocs.io/
Other
15.7k stars 2.1k forks source link

Check for PYTHONLIBS_FOUND leaves undefined variables. #926

Closed jdumas closed 7 years ago

jdumas commented 7 years ago

Hi,

I noticed that due to #236, FindPythonLibsNew.cmake contains the following line in the beginning:

if(PYTHONLIBS_FOUND)
    return()
endif()

Now, an unfortunate side effect of this, is that in such a situation the rest of the script is not run, and the variables PYTHON_PREFIX, PYTHON_MODULE_EXTENSION, etc. are not set.

This is a problem e.g. when building nanogui as part of the libigl build. Indeed, because find_package(PythonInterp REQUIRED) is already called in the libigl's CMake script, the condition above is triggered and the variables PYTHON_PREFIX, PYTHON_MODULE_EXTENSION, etc. are left undefined in the build of target nanogui-python.

I stumbled upon this issue because import nanogui doesn't work from python if the file has no extension, but it works if the target is compiled as nanogui.so instead.

I'm not sure what would be the workaround here, because I am not familiar with bug #236 ...

dean0x7d commented 7 years ago

A quick workaround would be to call pybind11's Python search before any other similar find_package(Python*) command:

list(APPEND CMAKE_MODULE_PATH "<pybind11_dir>/tools")
find_package(PythonLibsNew REQUIRED)

# ... safe to call other `find_package(Python* ...)` at any point after this

The underlying issue is that FindPythonLibsNew is a superset of FindPythonLibs with regard to variables and naming which causes problems when the two are mixed. This is a bit hard to resolve while maintaining backwards compatibility, but it has come up repeatedly and should be resolved. I'll try to work something out when I have a bit more time. In the meantime, I hope the workaround above can work for you.

jdumas commented 7 years ago

Well, I don't understand why you can't make it work the way it is right now (i.e. calling find_package(PythonInterp) before find_package(PythonLibsNew) should work no matter what).

If the goal is to avoid running the line find_package(PythonInterp), then the way I'd do it for example, is having FindPythonLibsNew.cmakedefine a variable PYTHONLIBSNEW_FOUND when it is run. In addition, if PYTHONLIBS_FOUND is set, then you would skip L58 to L62, and simply run all the mumbo-jumbo that comes afterwards.

Reading through #236 it looks like the problem comes when you are building a python interpreter as part of your cmake build. In which case, I'm sorry but it should be the caller's responsibility to prevent FindPythonLibsNew.cmake from being run, e.g. by setting PYTHONLIBSNEW_FOUND manually, and defining the variables that it would have defined on its own for the sake of the rest of the build.

Alternatively, you could check whether the executable ${PYTHON_EXECUTABLE} exists or not before running the command, and spawn an error/warning message, saying you are skipping this part (that way you would retain backward compatibility).

dean0x7d commented 7 years ago

Good points. Setting PYTHONLIBS_FOUND manually is already the recommendation in #236. In that case, PYTHON_MODULE_EXTENSION will also need to be specified manually, so the quick fix in PR #928 should resolve the LibsNew <-> Libs ordering issue and maintain compatibility with #236.

Can you try #928 and see if it works for you?