SyneRBI / SIRF-SuperBuild

SIRF CMake SuperBuild
http://www.ccpsynerbi.ac.uk
Apache License 2.0
15 stars 17 forks source link

problems with finding Python #48

Closed KrisThielemans closed 3 years ago

KrisThielemans commented 6 years ago

CMake's FindPythonInterp doesn't necessarily find the Python that you expect. this currently breaks Travis for Python 3.5.

see comments in #32 for potential resolution

KrisThielemans commented 6 years ago

hopefully fixed with de8179e4341a86a61fee4af6a81a5e5662bf18f8

KrisThielemans commented 6 years ago

This is still a pain on OSX. It seems it can only be made to reliably work by setting PYTHON_EXECUTABLE, PYTHON_LIBRARY and PYTHON_INCLUDE_DIR. (Note that currently in our .travis.yml we don't set PYTHON_EXECUTABLE which seems dangerous).

There are a zillion posts on this and it seems that the relevant modules in CMake need a maintainer. Here's for instance one CMake issue.

I wonder if the following wouldn't help a bit

find_package(PythonInterp ${PYVER})
FIND_PACKAGE(PythonLibs ${PYTHON_VERSION_STRING} EXACT)

Obviously, there's still a potential conflict if someone has 2 versions of Python installed that have exactly the same version, but at least it should reduce the likelihood.

By the way, relying on python-config doesn't work as this is an optional script which isn't installed by Conda for instance, leading to potential conflicts between the conda python version and the system python-config.

Also, we currently still have

FIND_PACKAGE(PythonLibs)

statement in CMakeLists.txt files in sub-folders, which might be a bad idea (probably not, as presumably variables will be cached already). In any case, they're redundant so I think we should remove them.

KrisThielemans commented 4 years ago

see also #313

KrisThielemans commented 4 years ago

Similar to recent updates, we need to create Python_CMAKE_ARGS replacing lines like https://github.com/SyneRBI/SIRF-SuperBuild/blob/aff6f779622307956741ecbd064ab24aa47bcac9/SuperBuild/External_SIRF.cmake#L95-L97 This way we can do it consistently for all CMake-based packages.

A difficulty is the different naming (Python_EXECUTABLE vs PYTHON_EXECUTABLE) necessary for a dependency depending if they use old-style or new-style FindPython. At present, I guess none of our dependencies uses the new-style yet, but it'll come. The simplest might be to duplicate. It is somewhat terrible, and will lead to some warnings about unused variables, but the superbuild has no control over what version of a dependency the user is asking for, so it seems the only solution.

KrisThielemans commented 4 years ago

note that this involve cython as well

paskino commented 3 years ago

My machine runs CentOS7 and I normally build the SuperBuild in a special conda environment and I never had any problem with finding the right python (once the environment is active).

The biggest pain IMHO is the Ubuntu 18.04 VM where both python 2.7 and 3.6, aliased as python and python3 are present. In that case it seems the strategy we use works.

KrisThielemans commented 3 years ago

The biggest pain IMHO is the Ubuntu 18.04 VM

that has to be because you haven't used a Mac yet 😉 (neither have I!)

It's great to know that it works. I think cleaning up the code as indicated above would still be worthwhile (i.e. have an External_Python.cmake with the relevant lines). Not urgent clearly.

KrisThielemans commented 3 years ago

Clean-up happens in #473. After that, I believe this should be closed.

KrisThielemans commented 3 years ago

473 is merged