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

make cmake use env python #196

Closed
wants to merge 3 commits into from
Closed

Conversation

amchoukir
Copy link

CustomPythonCmakeArgs forces CMake to pickup the environment python libraries and include but the interpreter is still picked using PythonInterp which picks up the system default instead of the environment python. This cause issues with vitualenv.

Review on Reviewable

CustomPythonCmakeArgs forces CMake to pickup the environment python libraries and include but the interpreter is still picked using PythonInterp which picks up the system default instead of the environment python. This cause issues with vitualenv.
@Valloric
Copy link
Member

Hm, this has the potential to break everything. The whole multiple-versions-of-python-on-one-machine thing is the single most frequent reason for failed YCM installations.

So we need to be really sure this is doing the right thing. I'd appreciated it if various people could try out this change; the more OS's/distros we cover, the better.

@vheon @micbou @puremourning

@micbou
Copy link
Collaborator

micbou commented Aug 16, 2015

The CustomPythonCmakeArgs function is only called on OS X so Linux and Windows are not affected by this change.

@puremourning
Copy link
Member

happy to give this a spin as i have 2 macs - 1 which the install always works and 1 which i always have do battle with homebrew/python/etc.

However, @amchoukir could you elaborate on exactly what problem this is solving? Does it relate to an issue on the tracker? Are there some obvious test steps we should go through?

Ta,
Ben

@amchoukir
Copy link
Author

Thanks for having a look and giving it a try.

@puremourning, @Valloric

CustomPythonCmakeArgs uses python-config to figure out where python is installed in the current environment. Unfortunately virtualenv does not copy python-config when setting up a new environment, I will take this issue separately to the virtualenv github repo. Copying python-config to my virtualenv takes care of the PythonLibs.

But there is still an issue with PythonInterp as can be seen from the install log below:

-- Found PythonLibs: /usr/local/Cellar/python/2.7.10/Frameworks/Python.framework/Versions/2.7/lib/libpython2.7.dylib (found suitable version "2.7.10", minimum required is "2.6")
-- Found PythonInterp: /usr/bin/python2.6 (found suitable version "2.6.9", minimum required is "2.6")

PythonInterp picks up the system default instead of the interpreter in my virtualenv. The fix I proposed take care of the issue in a similar way as PythonLibs.

My setup is as follows:

OS X 10.10.3 (Maverick)
Python 2.7.10 installed via homebrew
Macvim 7.4 installed via homebrew

The test steps I went through are:

  • Make sure the same version of the lib and interpreter are picked during the install
  • Check that you don't get vim crashing upon startup.

@vheon
Copy link
Contributor

vheon commented Aug 16, 2015

To my understanding virtualenv is a tool for python available not only on OSX, but the fix you're proposing is implemented in a function that is called only on OSX, so is the fix not needed on other platforms?


return [
'-DPYTHON_LIBRARY={0}'.format( python_library ),
'-DPYTHON_INCLUDE_DIR={0}'.format( python_include )
'-DPYTHON_INCLUDE_DIR={0}'.format( python_include ),
'-DPYTHON_EXECUTABLE={0}'.format(python_executable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace needed around python_executable. :)

@amchoukir
Copy link
Author

@vheon yes it might definitely be needed. I have access to linux servers on which I could try it out, if someone could have a look at windows.

@vheon
Copy link
Contributor

vheon commented Aug 19, 2015

@vheon yes it might definitely be needed.

@amchoukir Then this PR should be modified to run on linux as well right?

@puremourning
Copy link
Member

I think we should hold out on this change while I get the Travis OSX builds working. I've had to do some work to streamline the build which I think obsolete this change.

On 19 Aug 2015, at 18:46, Andrea Cedraro [email protected] wrote:

@vheon yes it might definitely be needed.

Then this PR should be modified to run on linux as well right?


Reply to this email directly or view it on GitHub.

@Valloric
Copy link
Member

@puremourning Sure, np.

@amchoukir
Copy link
Author

Here are the results of my tests on Linux

Scenario 1: System default sanity check

-- Found PythonLibs: /usr/lib/x86_64-linux-gnu/libpython2.7.so (found suitable version "2.7.6", minimum required is "2.6")
-- Found PythonInterp: /home/vagrant/.virtualenv/ycmd/bin/python2 (found suitable version "2.7.6", minimum required is "2.6")

Scenario 2: Vanilla virtualenv with 2.7.10

-- Found PythonLibs: /usr/lib/x86_64-linux-gnu/libpython2.7.so (found suitable version "2.7.6", minimum required is "2.6")
-- Found PythonInterp: /home/vagrant/.virtualenv/ycmd2.10/bin/python2 (found suitable version "2.7.10", minimum required is "2.6")

Wrong library picked up. It picked up the system default instead of the 2.7.10.

Scenario 3: virtualenv with 2.7.10 and python-config copied over

-- Found PythonLibs: /usr/lib/x86_64-linux-gnu/libpython2.7.so (found suitable version "2.7.6", minimum required is "2.6")
-- Found PythonInterp: /home/vagrant/.virtualenv/ycmd2.10/bin/python2 (found suitable version "2.7.10", minimum required is "2.6")

Wrong library picked up. It picked up the system default instead of the 2.7.10. So here python-config does not fix the issue.

Here are the specifics of python-config

(ycmd2.10)vagrant@vvv:/ycmd$ python-config --prefix
/usr/local/python/2.10
(ycmd2.10)vagrant@vvv:
/ycmd$ which python-config
/home/vagrant/.virtualenv/ycmd2.10/bin/python-config

Most likely this is due to the difference in paths between OS X and Linux

Next Steps

I will dig into build.py to check what happens.

@puremourning
Copy link
Member

@amchoukir Unfortunately virtualenv does not copy python-config when setting up a new environment, I will take this issue separately to the virtualenv github repo. Copying python-config to my virtualenv takes care of the PythonLibs.

Yes, this is annoying. I found the same problem when making the ycmd build/test run on Travis OS X. Did you raise this with virtualenv folks? If so could we get a reference?

Thanks,
Ben

@amchoukir
Copy link
Author

@puremourning yes Paul opened the following issue:
pypa/virtualenv#783

@amchoukir
Copy link
Author

Hello Everyone, I am back from vacation and I will have a look at why the build does not work on virtualenv for Linux.

Meanwhile the fix to get the python-config copied in virtualenv has been merged:

pypa/virtualenv#798

homu added a commit that referenced this pull request May 9, 2016
[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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/481)
<!-- Reviewable:end -->
homu added a commit that referenced this pull request May 9, 2016
[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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/481)
<!-- Reviewable:end -->
@homu homu closed this in #481 May 9, 2016
@homu
Copy link
Contributor

homu commented May 9, 2016

☔ The latest upstream changes (presumably #481) made this pull request unmergeable. Please resolve the merge conflicts.

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.

6 participants