Skip to content

fix Linux library search path #519

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

Closed
wants to merge 1 commit into from

Conversation

dvirtz
Copy link

@dvirtz dvirtz commented Nov 21, 2016

This fixes issue #516 by getting the path to the python library from sysconfig.

@dean0x7d
Copy link
Member

Looks like the library is only found on one of the linux configurations on Travis, but not on the others.

Perhaps it's a virtualenv issue and could be resolved similarly to the Windows path? (See CMAKE_HOST_WIN32 just above.)

@jagerman
Copy link
Member

It's not a virtualenv issue. FindPythonLibsNew is really, really broken on a modern Debian/Ubuntu system (e.g. it really isn't valid to guess that libpythonX.Ym.so will live under /usr/lib or /usr/lib64: on a modern Debian or Ubuntu system, it doesn't, but rather lives in an arch-specific path that the linker knows about, e.g. /usr/lib/x86_64-linux-gnu). Guessing this path, as the current non-Windows/OSX code is trying to do, simply isn't feasible. It's been working so far only because FindPythonLibsNew hasn't been actually doing anything: it falls back to just letting the linker figure it out.

I'm trying to look at the history for it, and I read through the rational in #207. While I think most of those changes were great, I think using FindPythonLibsNew introduced some serious regressions in FindPythonLibs that have been long-since fixed in cmake's upstream FindPythonLibs. (FindPythonLibs in current cmake correctly finds the arch-specific library).

The useful part of FindPythonLibsNew seems, mainly, to be in providing PYTHON_MODULE_EXTENSION/PYTHON_MODULE_PREFIX. Could we revert to stock cmake FindPythonLibs, and just extract the bit needed to get these, or are there also deeper issues being fixed here? Are those issues fixed in current cmake's FindPythonLibs (i.e. could we ship that, using it on older cmakes, with a separate extension to extract the module extension/prefix)?

@jagerman
Copy link
Member

As for the issue in #99, it looks like that would be solved by making sure we call find_package(PythonInterp) before find_package(PythonLibs), as cmake's FindPythonLibs.cmake mentions:

# If calling both ``find_package(PythonInterp)`` and
# ``find_package(PythonLibs)``, call ``find_package(PythonInterp)`` first to
# get the currently active Python version by default with a consistent version
# of PYTHON_LIBRARIES.

The code prior to #207 was doing this in the reverse order, which seems likely to be what led to #99.

@wjakob
Copy link
Member

wjakob commented Nov 22, 2016

Thee default CMake find_package(PythonInterp) and find_package(PythonLibs) have been a source of no end of pain in the past. Even if they have improved dramatically in recent versions of CMake (which I'm not sure they have), the problem is still that they will always remain broken for older CMake versions that are still in use.

Asking Python for where stuff is -- i.e. the approach taken by FindPythonLibsNew seems like the inherently right approach to me. So if there is a problem with it, I'm in favor of fixing FindPythonLibsNew rather than switching back to the old system.

Looking at the PR, thing I'm wondering is whether we even need "${PYTHON_PREFIX}/lib64" "${PYTHON_PREFIX}/lib" in the lines

set(_PYTHON_LIBS_SEARCH "${PYTHON_PREFIX}/lib64" "${PYTHON_PREFIX}/lib" "${PYTHON_LIBRARY_PATH}")

(and similar a few lines below)

@dean0x7d
Copy link
Member

dean0x7d commented Nov 22, 2016

@jagerman At first glance, it does seem like the following should work:

find_package(PythonInterp)
find_package(PythonLibs PYTHON_VERSION EXACT REQUIRED)

And I really really wish it did. It would have saved me so many headaches. The issue is that the two find modules work completely independently with only a common version number. But it's easy to install two Python distributions with the exact same version number (e.g. on Linux, you can have the system python and conda). In that case FindPythonLibs will fly off and and return the first library it finds matching the version number, but it's not going to care that it doesn't actually match the Python executable returned by FindPythonInterp (everything blows up at runtime because of the exe/lib mismatch).

This is especially problematic on macOS because it's very common to install Python from homebrew instead of using the bundled system variant. CMake will happily find the homebrew exe because it's first in the path, but by default it will return the system lib, thus requiring manual intervention. It becomes even worse if you have 3 different distributions installed (system, homebrew, conda -- all needed for testing) in which case the default CMake scripts will really go nuts. This has not changed even with the newer CMake versions.

There is an additional layer to this on Windows where 32-bit and 64-bit variants of Python are still used. The default CMake scripts can again return a mismatched library because it only looks at the version -- FindPythonInterp and FindPythonLibs don't cooperate in any way. There is also the possibility of using the wrong compiler bitness (FindPythonLibsNew shows a nice error message in this case).

There are probably other issues here that I'm forgetting... or selectively erasing from memory because of how frustrating the whole thing was to deal with (for a cross-platform, Python-distribution agnostic build system).

FindPythonLibsNew resolves these problems beautifully -- asking the executable for the information is much better than going around it and searching various paths. The module is a bit outdated at this point (the Continuum Analytics copyright is from 2012) but the approach is still the correct one and it's much better to fix this one issue with it, rather than go back to the builtin CMake modules and dance around their many issues.

Sorry for the lengthy rant. Back to the issue at hand: the Python executable should have a good hint at where the library is. Perhaps get_config_var('LIBPL') or get_config_var('LDLIBRARY') might be more appropriate? These aren't very well documented unfortunately.

@jagerman
Copy link
Member

@dean0x7d Thanks for the lengthy description; I appreciate the problems.

I think the right places to look are:

  • get_config_var('LIBDIR') + "/" + get_config_var('MULTIARCH')
  • get_config_var('LIBDIR')

where the first one is only checked if MULTIARCH is set.

@jagerman
Copy link
Member

(PR incoming)

@jagerman jagerman mentioned this pull request Nov 22, 2016
@wjakob
Copy link
Member

wjakob commented Nov 22, 2016

Closing in favor of #523.

@wjakob wjakob closed this Nov 22, 2016
@dvirtz
Copy link
Author

dvirtz commented Nov 23, 2016

@jagerman Thanks for resolving it.

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

Successfully merging this pull request may close these issues.

4 participants