[READY] Do not rely on CMake to find Python libraries #481

micbou · 2016-05-07T14:09:01Z

Problem

CMake is not able to find the Python libraries from pyenv. We can observe this on Travis. If we look at the build output of the Python 2.6 run on Linux, we have the following line during the CMake configuration:

-- Found PythonLibs: /usr/lib/libpython2.7.so (found suitable version "2.7.3", minimum required is "2.6")

which, in addition to be the wrong version, is the system library instead of the pyenv one. We confirm this by inspecting the ycm_core.so library from the build:

$ ldd ycm_core.so | grep python
libpython2.7.so.1.0 => /usr/lib/libpython2.7.so.1.0 (0x00007f0521fa1000)

Same issue with the Python 2.7 and 3.3 Linux runs although the library version is correct in these cases.
In the Travis configuration file, there is this comment:

This is because stupid cmake 2.8.11 has a bug preventing it from finding the pyenv pythons (ostensibly; I haven't checked, but online reports say the issue is gone with cmake 3.4).

I tried with CMake 3.5.1 (3.5.2 is the last version) and it is still not working.

That's not all. In some situations, CMake find the wrong Python system libraries (see this workaround on AppVeyor) or cannot find them at all (see this post on ycm-users).

Solution

From this, it is clear that we cannot rely on CMake to find the Python libraries. We need to find them in the build.py script and pass the PYTHON_LIBRARY and PYTHON_INCLUDE_DIR parameters to CMake, like we already do for OS X.
This is implemented by creating a FindPythonLibraries function for each supported platform: Linux, OS X, and Windows. For OS X, we are keeping the logic from CustomPythonCmakeArgs except that we are dropping support for system Python 2.6. For Linux, we need to use the ldconfig tool to find the system Python library on some distributions (e.g. Ubuntu) because it is not installed in its standard location. For Windows, it is straightforward. In all cases, we assume that the Python running the script is the one that will be used to build the library.

This PR also fixes an important issue. When building the ycmd library with Clang support and linking it to a non-system Python libray, the dynamic linker will not be able to find the Python library:

$ ldd ycm_core.so | grep python
libpython3.4m.so.1.0 => not found
$ objdump -x ycm_core.so | grep RPATH
RPATH                $ORIGIN

This is solved by setting the CMAKE_INSTALL_RPATH_USE_LINK_PATH option to true (see this wiki page for details on the option):

> ldd ycm_core.so | grep python
libpython3.4m.so.1.0 => /home/micbou/.pyenv/versions/3.4.4/lib/libpython3.4m.so.1.0 (0x00007f9fb6472000)
> objdump -x ycm_core.so | grep RPATH
RPATH                $ORIGIN:/home/micbou/.pyenv/versions/3.4.4/lib

Closes #196 (we don't care about the Python interpreter when building the ycmd library).
Fixes #479.

This change is

micbou · 2016-05-07T14:40:36Z

We need to clear Travis cache because the Python library from cached pyenv is not dynamic.

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

coveralls · 2016-05-07T15:33:04Z

Coverage remained the same at 84.901% when pulling 83ef584 on micbou:find-python-libs into 8617de8 on Valloric:master.

Valloric · 2016-05-07T18:05:23Z

I just cleared out all Travis caches.

Thanks for doing this! Yet Another Reason to never use Python for anything of importance ever again.

Review status: all files reviewed at latest revision, 2 unresolved discussions.

build.py, line 44 [r1] (raw file):
"but no dynamic" reads weird. Could this be rephrased?

build.py, line 183 [r1] (raw file):
This python_name buildup is repeated from the find-on-linux function. Could you extract it into a function? Similar for other parts if possible, there appears to be some opportunity for helper funcs.

Comments from Reviewable

puremourning · 2016-05-07T18:21:01Z

with one possible bug fix

Review status: all files reviewed at latest revision, 3 unresolved discussions.

build.py, line 213 [r1] (raw file):
Isn't this a tautology? Should it be if p.isfile( python_library )' ?

Comments from Reviewable

micbou · 2016-05-07T19:24:05Z

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

build.py, line 44 [r1] (raw file):
I changed it. Hope it's better formulated this way.

build.py, line 183 [r1] (raw file):
Done for the python_name, python_library_root, and python_include variables.

build.py, line 213 [r1] (raw file):
Yes, good catch.

Comments from Reviewable

coveralls · 2016-05-07T19:26:41Z

Coverage remained the same at 84.901% when pulling 94b6739 on micbou:find-python-libs into 8617de8 on Valloric:master.

coveralls · 2016-05-07T19:37:27Z

Coverage remained the same at 84.901% when pulling 15f3d04 on micbou:find-python-libs into 8617de8 on Valloric:master.

vheon · 2016-05-07T19:45:43Z

Reviewed 6 of 7 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

puremourning · 2016-05-07T19:46:53Z

Reviewed 6 of 7 files at r1, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

coveralls · 2016-05-07T20:02:33Z

Coverage remained the same at 84.901% when pulling 15f3d04 on micbou:find-python-libs into 8617de8 on Valloric:master.

micbou · 2016-05-08T23:04:58Z

PR is ready. When this is merged, I'll send a PR to the YCM repository that updates the Travis configuration.

Previously, coveralls wrote…

Coverage remained the same at 84.901% when pulling 15f3d04 on micbou:find-python-libs into 8617de8 on Valloric:master.

Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

Valloric · 2016-05-09T05:17:49Z

Awesome!

@homu r+

homu · 2016-05-09T05:17:51Z

📌 Commit 15f3d04 has been approved by Valloric

[READY] Do not rely on CMake to find Python libraries ### Problem CMake is not able to find the Python libraries from `pyenv`. We can observe this on Travis. If we look at the build output of the Python 2.6 run on Linux, we have the following line during the CMake configuration: ```sh -- Found PythonLibs: /usr/lib/libpython2.7.so (found suitable version "2.7.3", minimum required is "2.6") ``` which, in addition to be the wrong version, is the system library instead of the `pyenv` one. We confirm this by inspecting the `ycm_core.so` library from the build: ```sh $ ldd ycm_core.so | grep python libpython2.7.so.1.0 => /usr/lib/libpython2.7.so.1.0 (0x00007f0521fa1000) ``` Same issue with the Python 2.7 and 3.3 Linux runs although the library version is correct in these cases. In the Travis configuration file, there is this comment: > This is because stupid cmake 2.8.11 has a bug preventing it from finding the pyenv pythons (ostensibly; I haven't checked, but online reports say the issue is gone with cmake 3.4). I tried with CMake 3.5.1 (3.5.2 is the last version) and it is still not working. That's not all. In some situations, CMake find the wrong Python system libraries (see [this workaround on AppVeyor](https://github.com/Valloric/ycmd/blob/master/ci/appveyor/appveyor_install.bat#L36)) or cannot find them at all (see [this post on ycm-users](https://groups.google.com/forum/#!searchin/ycm-users/windows$20cmake/ycm-users/e6K-grbeUMw/0kzwtAaYCQAJ)). ### Solution From this, it is clear that we cannot rely on CMake to find the Python libraries. We need to find them in the `build.py` script and pass the `PYTHON_LIBRARY` and `PYTHON_INCLUDE_DIR` parameters to CMake, like we already do for OS X. This is implemented by creating a `FindPythonLibraries` function for each supported platform: Linux, OS X, and Windows. For OS X, we are keeping the logic from `CustomPythonCmakeArgs` except that we are dropping support for system Python 2.6. For Linux, we need to use the `ldconfig` tool to find the system Python library on some distributions (e.g. Ubuntu) because it is not installed in its standard location. For Windows, it is straightforward. In all cases, we assume that the Python running the script is the one that will be used to build the library. This PR also fixes an important issue. When building the ycmd library with Clang support and linking it to a non-system Python libray, the dynamic linker will not be able to find the Python library: ```sh $ ldd ycm_core.so | grep python libpython3.4m.so.1.0 => not found $ objdump -x ycm_core.so | grep RPATH RPATH $ORIGIN ``` This is solved by setting the `CMAKE_INSTALL_RPATH_USE_LINK_PATH` option to true (see [this wiki page](https://cmake.org/Wiki/CMake_RPATH_handling) for details on the option): ``` > ldd ycm_core.so | grep python libpython3.4m.so.1.0 => /home/micbou/.pyenv/versions/3.4.4/lib/libpython3.4m.so.1.0 (0x00007f9fb6472000) > objdump -x ycm_core.so | grep RPATH RPATH $ORIGIN:/home/micbou/.pyenv/versions/3.4.4/lib ``` Closes #196 (we don't care about the Python interpreter when building the ycmd library). Fixes #479.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/481)

homu · 2016-05-09T05:18:03Z

⌛ Testing commit 15f3d04 with merge 3b135e1...

homu · 2016-05-09T05:38:16Z

💔 Test failed - status

micbou · 2016-05-09T07:43:54Z

Let's retry without the cache.

@homu r+

Previously, homu (Homu) wrote…

💔 Test failed - status

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

homu · 2016-05-09T07:43:55Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

homu · 2016-05-09T07:43:56Z

📌 Commit 15f3d04 has been approved by micbou

homu · 2016-05-09T07:43:57Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

micbou · 2016-05-09T07:44:14Z

@homu retry

Previously, homu (Homu) wrote…

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

[READY] Do not rely on CMake to find Python libraries ### Problem CMake is not able to find the Python libraries from `pyenv`. We can observe this on Travis. If we look at the build output of the Python 2.6 run on Linux, we have the following line during the CMake configuration: ```sh -- Found PythonLibs: /usr/lib/libpython2.7.so (found suitable version "2.7.3", minimum required is "2.6") ``` which, in addition to be the wrong version, is the system library instead of the `pyenv` one. We confirm this by inspecting the `ycm_core.so` library from the build: ```sh $ ldd ycm_core.so | grep python libpython2.7.so.1.0 => /usr/lib/libpython2.7.so.1.0 (0x00007f0521fa1000) ``` Same issue with the Python 2.7 and 3.3 Linux runs although the library version is correct in these cases. In the Travis configuration file, there is this comment: > This is because stupid cmake 2.8.11 has a bug preventing it from finding the pyenv pythons (ostensibly; I haven't checked, but online reports say the issue is gone with cmake 3.4). I tried with CMake 3.5.1 (3.5.2 is the last version) and it is still not working. That's not all. In some situations, CMake find the wrong Python system libraries (see [this workaround on AppVeyor](https://github.com/Valloric/ycmd/blob/master/ci/appveyor/appveyor_install.bat#L36)) or cannot find them at all (see [this post on ycm-users](https://groups.google.com/forum/#!searchin/ycm-users/windows$20cmake/ycm-users/e6K-grbeUMw/0kzwtAaYCQAJ)). ### Solution From this, it is clear that we cannot rely on CMake to find the Python libraries. We need to find them in the `build.py` script and pass the `PYTHON_LIBRARY` and `PYTHON_INCLUDE_DIR` parameters to CMake, like we already do for OS X. This is implemented by creating a `FindPythonLibraries` function for each supported platform: Linux, OS X, and Windows. For OS X, we are keeping the logic from `CustomPythonCmakeArgs` except that we are dropping support for system Python 2.6. For Linux, we need to use the `ldconfig` tool to find the system Python library on some distributions (e.g. Ubuntu) because it is not installed in its standard location. For Windows, it is straightforward. In all cases, we assume that the Python running the script is the one that will be used to build the library. This PR also fixes an important issue. When building the ycmd library with Clang support and linking it to a non-system Python libray, the dynamic linker will not be able to find the Python library: ```sh $ ldd ycm_core.so | grep python libpython3.4m.so.1.0 => not found $ objdump -x ycm_core.so | grep RPATH RPATH $ORIGIN ``` This is solved by setting the `CMAKE_INSTALL_RPATH_USE_LINK_PATH` option to true (see [this wiki page](https://cmake.org/Wiki/CMake_RPATH_handling) for details on the option): ``` > ldd ycm_core.so | grep python libpython3.4m.so.1.0 => /home/micbou/.pyenv/versions/3.4.4/lib/libpython3.4m.so.1.0 (0x00007f9fb6472000) > objdump -x ycm_core.so | grep RPATH RPATH $ORIGIN:/home/micbou/.pyenv/versions/3.4.4/lib ``` Closes #196 (we don't care about the Python interpreter when building the ycmd library). Fixes #479.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/481)

homu · 2016-05-09T07:44:27Z

⌛ Testing commit 15f3d04 with merge b338481...

homu · 2016-05-09T08:15:29Z

☀️ Test successful - status

[READY] Remove architecture option in scripts With the changes from PR #481, the `--arch` option cannot be used anymore to specify the architecture on Windows because the architecture of the Python library is now the same as the Python interpreter running the `build.py` script and this architecture must match the one used to compile ycmd.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/484)

[READY] Update Travis configuration Update ycmd to include PR ycm-core/ycmd#481 and update Travis configuration according to this PR.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2157)

micbou · 2016-05-16T16:00:48Z

Thanks for the heads up. We'll take care of that in the next iteration.

micbou · 2016-05-19T10:17:29Z

@ff2000 Could you try PR #499 and tell us if it works for you?

micbou force-pushed the find-python-libs branch from 0e0d318 to bd7822b Compare May 7, 2016 14:36

micbou force-pushed the find-python-libs branch 2 times, most recently from 74a83be to 83ef584 Compare May 7, 2016 15:04

micbou force-pushed the find-python-libs branch 2 times, most recently from 94b6739 to c2db194 Compare May 7, 2016 19:15

Do not rely on CMake to find Python libraries

15f3d04

micbou force-pushed the find-python-libs branch from c2db194 to 15f3d04 Compare May 7, 2016 19:19

micbou changed the title ~~[RFC] Do not rely on CMake to find Python libraries~~ [READY] Do not rely on CMake to find Python libraries May 8, 2016

homu merged commit 15f3d04 into ycm-core:master May 9, 2016

This was referenced May 9, 2016

make cmake use env python #196

Closed

[READY] Add debugging instructions and tools #454

Merged

micbou mentioned this pull request May 9, 2016

[READY] Update Travis configuration ycm-core/YouCompleteMe#2157

Merged

micbou deleted the find-python-libs branch May 9, 2016 09:52

micbou mentioned this pull request May 9, 2016

[READY] Remove architecture option in scripts #484

Merged

dalejung mentioned this pull request May 9, 2016

post install changes required for anaconda distribution. #485

Closed

puneetmnit mentioned this pull request May 24, 2016

Auto merge of #2157 - micbou:find-python-libs, r=Valloric ycm-core/YouCompleteMe#2181

Closed

4 tasks

micbou mentioned this pull request Aug 4, 2016

ldconfig requires root in Debian ycm-core/YouCompleteMe#2270

Closed

micbou mentioned this pull request Jul 31, 2017

Ubuntu 16.04: ycmd crashing because of install.py picking wrong Python version ycm-core/YouCompleteMe#2729

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[READY] Do not rely on CMake to find Python libraries #481

[READY] Do not rely on CMake to find Python libraries #481

micbou commented May 7, 2016 •

edited

Loading

micbou commented May 7, 2016

coveralls commented May 7, 2016

Valloric commented May 7, 2016

puremourning commented May 7, 2016

micbou commented May 7, 2016

coveralls commented May 7, 2016

coveralls commented May 7, 2016

vheon commented May 7, 2016

puremourning commented May 7, 2016

coveralls commented May 7, 2016

micbou commented May 8, 2016

Valloric commented May 9, 2016

homu commented May 9, 2016

homu commented May 9, 2016

homu commented May 9, 2016

micbou commented May 9, 2016

homu commented May 9, 2016

homu commented May 9, 2016

homu commented May 9, 2016

micbou commented May 9, 2016

homu commented May 9, 2016

homu commented May 9, 2016

micbou commented May 16, 2016

micbou commented May 19, 2016

[READY] Do not rely on CMake to find Python libraries #481

[READY] Do not rely on CMake to find Python libraries #481

Conversation

micbou commented May 7, 2016 • edited Loading

Problem

Solution

micbou commented May 7, 2016

coveralls commented May 7, 2016

Valloric commented May 7, 2016

puremourning commented May 7, 2016

micbou commented May 7, 2016

coveralls commented May 7, 2016

coveralls commented May 7, 2016

vheon commented May 7, 2016

puremourning commented May 7, 2016

coveralls commented May 7, 2016

micbou commented May 8, 2016

Valloric commented May 9, 2016

homu commented May 9, 2016

homu commented May 9, 2016

homu commented May 9, 2016

micbou commented May 9, 2016

homu commented May 9, 2016

homu commented May 9, 2016

homu commented May 9, 2016

micbou commented May 9, 2016

homu commented May 9, 2016

homu commented May 9, 2016

micbou commented May 16, 2016

micbou commented May 19, 2016

micbou commented May 7, 2016 •

edited

Loading