Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FindNewPython's forces check against PYTHON_EXECUTABLE during configuration phase #236

Closed
tony opened this issue Jun 11, 2016 · 11 comments
Closed

Comments

@tony
Copy link

tony commented Jun 11, 2016

A recent change to pybind11 does checks against a python interpreter. The verification done for FindPythonLibs doesn’t apply in the case of superbuilt python (ExternalProject_Add) since the PYTHON_EXECUTABLE isn't ready until compile time.

In my circumstances, I already have ${PYTHON_LIBRARY} and ${PYTHON_INCLUDE_DIR} set. CMake's FindPythonLibs allows you to specify your versions:

If you'd like to specify the installation of Python to use, you should
modify the following cache variables:

     PYTHON_LIBRARY             - path to the python library
     PYTHON_INCLUDE_DIR         - path to where Python.h is found

Expected behavior: FindPythonLibsNew.cmake should skip execute_process(COMMAND "${PYTHON_EXECUTABLE}" "-c" and/or fallback to find_package(PythonLibs) behavior if PYTHON_LIBRARY and PYTHON_INCLUDE_DIR is already set.

@tony tony changed the title FindNewPython's checks breaks FindPythonLibs FindNewPython's forces check against PYTHON_EXECUTABLE during configuration phase Jun 11, 2016
@wjakob
Copy link
Member

wjakob commented Jun 11, 2016

Since this is a fairly specialized use case, I wonder if you could you propose a small change to the build system that will accomplish this?

@dean0x7d
Copy link
Member

What's the actual build error that results from this? I'm wondering if the failure point is in FindPythonLibsNew.cmake or pybind11_add_module.

@tony
Copy link
Author

tony commented Jun 11, 2016

@dean0x7d

During add_subdirectory(pybind11), I get the same with and without pybind11_add_module.

❯ cmake ..  -GNinja -DCMAKE_BUILD_TYPE=Debug
-- WITH_PYMALLOC
-- PYTHON_INCLUDE_DIR: /Users/me/study/c++/kakoune/build/superbuild/Python-install/include/python3.5
-- PYTHON_LIBRARY: /Users/me/study/c++/kakoune/build/superbuild/Python-install/lib/libpython3.5m.dylib
CMake Error at pybind11/tools/FindPythonLibsNew.cmake:93 (message):
  Python config failure:

Call Stack (most recent call first):
  pybind11/CMakeLists.txt:26 (find_package)


-- Configuring incomplete, errors occurred!
See also "/Users/me/study/c++/kakoune/build/CMakeFiles/CMakeOutput.log".

@wjakob @dean0x7d I'm preparing more details

@dean0x7d
Copy link
Member

Ah, I see now. And I guess the easiest fix would be as you suggested: to skip the function in case the variables are already defined. The only issue is that FindPythonLibsNew.cmake also produces the module prefix and extension, so you'd need to supply those manually as well.
To be exact these 4 variables would need to be supplied manually to replace the functionality of FindPythonLibsNew.cmake.

@tony
Copy link
Author

tony commented Jun 11, 2016

  1. set(PythonLibsNew_FIND_REQUIRED CACHE BOOL FALSE) in the my project's CMakeLists.txt won't work, since find_package(PythonLibsNew... is REQUIRED.

  2. add_directory(pybind11) runs the pybind11/CMakeLists.txt and therefore FindPythonLibsNew, regardless of my already having my PYTHON_INCLUDE_DIR and PYTHON_LIBRARY (Is anything else needed by pybind11?)

    ExternalProject_Add was considered, it allows waiting until a dependency (python) is built (such as by python-cmake-buildsystem). However, it won't get pybind11_add_module and stuff. The only way to make it work would be to refactor cmake to run in two-runs (as it would seem) only to settle the PYTHON_EXECUTABLE not being there yet.

    ExternalProject_Add would be used to defer running add_subdirectory(pybind11) on the first time cmake is used. During the first step, python builds. Then cmake would could be reran and add_subdirectory(pybind11) would work.

I technically could do the above if it was a must, but its an inconvenience unless pybind11 really requires it being there as a dependency.

@dean0x7d I am going to follow up your reply @#236 (comment) next.

@dean0x7d
Copy link
Member

For a concrete fix, I propose adding the following to the top of FindPythonLibsNew.cmake:

if(PYTHONLIBS_FOUND)
    return()
endif()

So if the user supplies PYTHONLIBS_FOUND, the search is skipped and it's the user's responsibility to supply the required variables.

@tony
Copy link
Author

tony commented Jun 11, 2016

PYTHON_MODULE_EXTENSION. That is supposed to be stuff like so, dylib, lib?

As for PYTHON_MODULE_PREFIX that is just the sys.prefix path to the python framework?

@tony
Copy link
Author

tony commented Jun 11, 2016

@dean0x7d I am trying your suggestion in #236 (comment) and will reply shortly.

@dean0x7d
Copy link
Member

PYTHON_MODULE_EXTENSION can usually be set to .so on Linux/OSX and .pyd on Windows. That should work in most cases.

PYTHON_MODULE_PREFIX is not the same as PYTHON_PREFIX / sys.prefix.
The module prefix determines the module file name. It just replaces the default 'lib' prefix with an empty string, so you get example.so instead of libexample.so.

@tony
Copy link
Author

tony commented Jun 11, 2016

@dean0x7d

I can confirm that the solution you suggested at #236 (comment) works.

I was able to test it with (PYTHON_INCLUDE_DIR and PYTHON_LIBRARY already found):

set(PYTHONLIBS_FOUND TRUE)
set(PYTHON_MODULE_EXTENSION .so)
set(PYTHON_MODULE_PREFIX "")
add_subdirectory(pybind11)

@dean0x7d
Copy link
Member

dean0x7d commented Jun 12, 2016

@tony Good to hear that it works.
@wjakob I have the required change in this commit dean0x7d@e782eb8. If the solution is acceptable, I can summit a PR.

@wjakob wjakob closed this as completed in e782eb8 Jun 12, 2016
wjakob added a commit that referenced this issue Jun 12, 2016
Skip FindPythonLibsNew if PYTHONLIBS_FOUND is defined (fix #236)
tpeulen pushed a commit to Fluorescence-Tools/LabelLib that referenced this issue Dec 6, 2018
tpeulen pushed a commit to Fluorescence-Tools/LabelLib that referenced this issue Dec 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants