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

Remove -DPYTHON_HOME and -DPYTHON3_HOME by default #2787

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@methane
Copy link

methane commented Apr 6, 2018

Use Python prefix when python is built, instead of vim is built.
This allows upgrading Python without rebuilding Vim, when Python
is dynamically linked.

Remove -DPYTHON_HOME and -DPYTHON3_HOME by default
Use Python prefix when python is built, instead of vim is built.
This allows upgrading Python without rebuilding Vim, when Python
is dynamically linked.

@methane methane referenced this pull request Apr 6, 2018

Open

No module name encodings #562

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 6, 2018

Codecov Report

Merging #2787 into master will increase coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2787     +/-   ##
=========================================
+ Coverage   75.09%    75.2%   +0.1%     
=========================================
  Files          91       92      +1     
  Lines      131613   134385   +2772     
=========================================
+ Hits        98832   101060   +2228     
- Misses      32781    33325    +544
Impacted Files Coverage Δ
src/if_python3.c 74.93% <0%> (-1.38%) ⬇️
src/if_python.c 76.71% <0%> (-1.31%) ⬇️
src/spellfile.c 73.94% <0%> (-1.05%) ⬇️
src/if_perl.xs 85.86% <0%> (-0.36%) ⬇️
src/netbeans.c 27.03% <0%> (-0.28%) ⬇️
src/libvterm/src/screen.c 54.91% <0%> (-0.19%) ⬇️
src/if_lua.c 54.17% <0%> (-0.16%) ⬇️
src/eval.c 81.25% <0%> (-0.15%) ⬇️
src/main.c 56.35% <0%> (-0.14%) ⬇️
src/regexp.c 82.51% <0%> (-0.08%) ⬇️
... and 67 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5359b2...92e1f8b. Read the comment docs.

@brammool

This comment has been minimized.

Copy link
Contributor

brammool commented Apr 6, 2018

@methane

This comment has been minimized.

Copy link

methane commented Apr 10, 2018

Unfortunately, the docs for Py_SetPythonHome() do not mention what happens if the function is not called. Are you implying that it will use some internal default from when Python was built?

Yes. See PYTHONHOME document.
https://docs.python.org/3.6/using/cmdline.html#envvar-PYTHONHOME

If I recall correctly, the reason it was done this way is that it's possible that a system has more than one version of Python installed and Vim needs to know wher the libraries are.

This was added by Roland's report and patch.

He said python3 can't load it's libraries from /usr/local without Py_SetPythonHome. But actually, Python can.
I assume his environment was broken; maybe, libpython3.x.so is loaded from where he didn't expect.
But I'm not sure because his mail doesn't have enough information about how Python is installed and linked.

But after that the 'pythonhome' option was added, so perhaps that is no longer a good reason.

Agree.

@methane

This comment has been minimized.

Copy link

methane commented Apr 10, 2018

This issue is very important for binary distribution of Vim like MacVim.
When Python's path is different between builder and user, Python can't load standard libraries even when Vim can load python DLL and the python DLL knows right python prefix.
Many MacVim users are suffered from this issue.

@brammool brammool closed this in 0424958 Apr 10, 2018

@methane

This comment has been minimized.

Copy link

methane commented Apr 10, 2018

thanks!

@methane methane deleted the methane:remove-pythonhome branch Apr 10, 2018

@justrajdeep

This comment has been minimized.

Copy link

justrajdeep commented Apr 10, 2018

I have multiple versions of python installed in my system. The $PYTHON_HOME points to something.

which python is set to a different version of python.

i wanted vim to use a 3rd version of python and i was setting that in the config using --with-python-config-dir=

after this change

py import sys is broken with the following error

E887: Sorry, this command is disabled, the Python's site module could not be loaded.

Thanks
Rajdeep

@micbou

This comment has been minimized.

Copy link

micbou commented Apr 10, 2018

@justrajdeep You should set the pythonhome (or pythonthreehome) option to the right Python home in your vimrc.

@methane

This comment has been minimized.

Copy link

methane commented Apr 10, 2018

Theorically speaking, pythonhome should point to exactly same build of python. It means there can be problem caused by private API mismatch.

If vim is linked to different python, fix linking is better than using only library of different python.

@justradeep how do you build vim? +python/dyn or +python? If later, is python statically linked or dynamically linked?

@justrajdeep

This comment has been minimized.

Copy link

justrajdeep commented Apr 10, 2018

 --enable-pythoninterp=yes \
 --enable-python3interp=no \

in my config

@justrajdeep

This comment has been minimized.

Copy link

justrajdeep commented Apr 10, 2018

@methane

This comment has been minimized.

Copy link

methane commented Apr 10, 2018

What ldd vim shows?

@brammool

This comment has been minimized.

Copy link
Contributor

brammool commented Apr 10, 2018

@justrajdeep

This comment has been minimized.

Copy link

justrajdeep commented Apr 10, 2018

@methane

This comment has been minimized.

Copy link

methane commented Apr 10, 2018

@justrajdeep Thanks. It seems your vim statically linked Python.
Maybe, prefix is different when Python is executed by python command and libpython.a.
Could you try this?

$ python2 -c 'import sys; print sys.prefix, sys.exec_prefix'
/usr /usr

$ cat test.c
#include <Python.h>

int
main(int argc, char *argv[])
{
	printf("prefix = %s\n", Py_GetPrefix());
	printf("exec_prefix = %s\n", Py_GetExecPrefix());
	printf("path = %s\n", Py_GetPath());
	printf("pyhome = %s\n", Py_GetPythonHome());

	Py_Initialize();
	PyRun_SimpleString("from time import time,ctime\n"
			"print('Today is', ctime(time()))\n");
	return 0;
}

$ cc test.c -o test `python2-config --cflags --ldflags`
$ ./test
prefix = /usr
exec_prefix = /usr
path = /usr/lib/python2.7:/usr/lib/python2.7/plat-x86_64-linux-gnu:/usr/lib/python2.7/lib-tk:/usr/lib/python2.7/lib-old:/usr/lib/python2.7/lib-dynload
pyhome = (null)
('Today is', 'Wed Apr 11 01:41:59 2018')

Pasting python2 -m sysconfig output to gist is also helpful.

@methane

This comment has been minimized.

Copy link

methane commented Apr 10, 2018

I suspected this might happen. How about keeping the PYTHON_HOME flag when with-python-config-dir was used? Let me add that to configure so you can try it out.

I don't like it. When vim is distributed as binary, Python may be in different location.
Especially, homebrew changes directly for every micro version at least.

For special case like that, people can set PYTHON_HOME by CFLAGS="-DPYTHON_HOME=$(python -c 'import sys; print(sys.prefix)).

@methane

This comment has been minimized.

Copy link

methane commented Apr 10, 2018

@brammool I confirmed -DPYTHON_HOME is not necessary.

$ git checkout f59c6e8cee092433d325ba21a107654a8d84f776
$ ./configure --enable-pythoninterp=yes --with-python-config-dir=$HOME/pyenv/versions/2.7.13/lib/python2.7/config --prefix=$HOME/local/vim && make -j4 && make install
...
:py import sys; print sys.prefix, sys.exec_prefix
/home/inada-n/pyenv/versions/2.7.13 /home/inada-n/pyenv/versions/2.7.13

Please don't embed PYTHON_HOME by default. It makes impossible to distribute vim binary safely.
Python library knows it's prefix. Even if not, user can use set pythonhome.

@justrajdeep

This comment has been minimized.

Copy link

justrajdeep commented Apr 10, 2018

@methane

$ python2 -c 'import sys; print sys.prefix, sys.exec_prefix'
/usr /usr

save.txt

$ cc test.c -o test `python2-config --cflags --ldflags`
$ ./test

test_out.txt

python2 -m sysconfig

sysconfig.txt

@methane

This comment has been minimized.

Copy link

methane commented Apr 10, 2018

Sorry, I meant "python you want to use with vim", not /usr/bin/python2.

@methane

This comment has been minimized.

Copy link

methane commented Apr 10, 2018

@justrajdeep When seeing test_out and sysconfig, your Python is fine.
Now I suspect you're linking different Python.

Your gvim_version.txt says:

Linking: gcc -L/tool/pandora64/.package/glib-2.32.1/lib -L/usr/lib64   -L/usr/lib64   -L. 
(snip)
-L/tool/pandora64/.package/python-2.7.3/lib -Wl,-rpath,/usr/lib64 -lutil -L/usr/local/lib -Wl,--as-needed -o vim   -pthread
(snip)
-L/tool/pandora64/.package/python-2.7.3/lib/python2.7/config -lpython2.7
(snip)

Does this command really link /tool/pandora64/.package/python-2.7.3/lib/libpython2.7.a?
What's happen if you replace -lpython2.7 with /tool/pandora64/.package/python-2.7.3/lib/lib/python2.7.a?

@justrajdeep

This comment has been minimized.

Copy link

justrajdeep commented Apr 10, 2018

@methane
i dont do manual linking anywhere

setenv LDFLAGS "-L/tool/pandora64/.package/python-2.7.3/lib -Wl,-rpath,/usr/lib64 -lutil"
...
--with-python-config-dir=/tool/pandora64/.package/python-2.7.3/lib/python2.7/config \
--x-libraries=/usr/lib64 \
--x-includes=/usr/include \
--enable-pythoninterp=yes \
--enable-python3interp=no \
...

then only command run is:

make clean && make distclean && ./my_config && make && make install

the following

/tool/pandora64/.package/python-2.7.3/bin/python2 -m sysconfig 

has no outpout

@brammool

This comment has been minimized.

Copy link
Contributor

brammool commented Apr 10, 2018

@methane

This comment has been minimized.

Copy link

methane commented Apr 10, 2018

@justrajdeep

i dont do manual linking anywhere

make does linking. I suspect that it do wrong thing because of some linker option.
Could you describe what /tool/pandora64 is and what my_config is?

If it's difficult to manual linking, could you try this?

# Show precise Python version.  
$ /tool/pandora64/.package/python-2.7.3/bin/python -c 'import sys; print(sys.version)'
2.7.13 (default, Apr 10 2017, 15:59:43)
[GCC 6.2.0 20161005]

# Show same in Vim.  Replace vim path with your vim working with python.
$ ~/local/vim/bin/vim
:py import sys; print(sys.version)
2.7.13 (default, Apr 10 2017, 15:59:43)
[GCC 6.2.0 20161005]

If link is succeeded, exact same string is output.

@methane

This comment has been minimized.

Copy link

methane commented Apr 10, 2018

@brammool

The other report says it is necessary.

In common case (apt install python, yum install python, pyenv, manual make&install), it's not necessary. In other common case (homebrew, or tools like stow), it cause SIGABORT.

I don't know pandra64, but I think it's rare case.

Note that Vim should work out of the box, that's why we have configure. Setting 'pythonhome' should only be needed in rare cases.

Yes. And installing Vim and Python from binary distribution is the major case.
Many homebrew users are suffered from -DPYTHON3_HOME because homebrew installs python to different place for every revision.
And they can't set specify configure/compile option, because the binary is prebuilt.

I'm OK to adding opt-in option which use -DPYTHON_HOME and -DPYTHON3_HOME,
but please don't use it.

For another major cases (apt install python, yum install python, pyenv install python), -DPYTHON_HOME and -DPYTHON3_HOME are not required.

@justrajdeep

This comment has been minimized.

Copy link

justrajdeep commented Apr 11, 2018

@methane

Could you describe what /tool/pandora64 is and what my_config is?

/tool/pandora64 is admin maintain installation location of 64-bit versions of s/w

my_config is just a text file to put all the options to configure vim.

Show precise Python version.
$ /tool/pandora64/.package/python-2.7.3/bin/python -c 'import sys; print(sys.version)'

2.7.3 (default, May 9 2013, 09:31:41)
[GCC 4.4.6]

Show same in Vim. Replace vim path with your vim working with python.

:py import sys; print(sys.version)
2.7.3 (default, May 9 2013, 09:31:41)
[GCC 4.4.6]

@methane

This comment has been minimized.

Copy link

methane commented Apr 11, 2018

@justrajdeep OK, I found inconsistencies in your environment.

I don't know why, but you have (maybe broken) Python 2.7.3 and 2.7.5.
When you run python or python2, Python 2.7.5 is used.
But Vim linked with Python 2.7.3.

Since Vim embeds sys.prefix of python command (not library), your Vim running with Python 2.7.5's standard library.
Even if it works with -DPYTHON_HOME or set pythonhome, it's not right approach.
While Python keeps ABI while micro versions, standard libraries use private API which are not part of ABI. Python standard libraries are tightly coupled with Python executable and library.

So in your case, you need to fix your configure script to use Python 2.7.5's config, not 2.7.3.

@justrajdeep

This comment has been minimized.

Copy link

justrajdeep commented Apr 11, 2018

@methane

I don't know why, but you have (maybe broken) Python 2.7.3 and 2.7.5.
When you run python or python2, Python 2.7.5 is used.
But Vim linked with Python 2.7.3.

changing it works, I have verified it 164268d and 6995c0a.

Thanks

@methane

This comment has been minimized.

Copy link

methane commented Apr 11, 2018

@brammool Now I understand why @justrajdeep is confused.

Here is demonstration using v8.0.1688 (which uses -DPYTHON_HOME again):

# I have two Python 2.7; /usr/bin/python2 and ~/pyenv/versions/2.7.13/bin/python
# I don't set PATH for pyenv's Python 2.
$ which python2
/usr/bin/python2
$ python2 -V
Python 2.7.14
$ ~/pyenv/versions/2.7.13/bin/python2 -V
Python 2.7.13

# Let's configure Vim with pyenv's config dir.
$ ./configure --enable-pythoninterp=yes --with-python-config-dir=$HOME/pyenv/versions/2.7.13/lib/python2.7/config

# config.mk mixes two Pythons!
$ grep PYTHON_ src/auto/config.mk
PYTHON_SRC      = if_python.c
PYTHON_OBJ      = objects/if_python.o
PYTHON_CFLAGS   = -I/usr/include/python2.7 -DPYTHON_HOME='"/usr"' -pthread -fPIE
PYTHON_LIBS     = -L/home/inada-n/pyenv/versions/2.7.13/lib/python2.7/config -lpython2.7 -lpthread -ldl -lutil -lm -Xlinker -export-dynamic
PYTHON_CONFDIR  = /home/inada-n/pyenv/versions/2.7.13/lib/python2.7/config
PYTHON_GETPATH_CFLAGS = -DPYTHONPATH='":/usr/lib/python2.7:/usr/lib/python2.7/plat-x86_64-linux-gnu:/usr/lib/python2.7/lib-tk:/usr/lib/python2.7/lib-old:/usr/lib/python2.7/lib-dynload:/home/inada-n/.local/lib/python2.7/site-packages:/usr/local/lib/python2.7/dist-packages:/usr/lib/python2.7/dist-packages"' -DPREFIX='"/usr"' -DEXEC_PREFIX='"/usr"'

When --with-python-config-dir option is used, is it strict requirement that config-dir is consistent with python command?
If no, I think we should calculate PYTHON_CFLAGS (and all PYTHON_ options) from config dir.

@methane

This comment has been minimized.

Copy link

methane commented Apr 11, 2018

@justrajdeep FYI, I think removing --with-config-dir option is better approach. configure can detect right config dir from your python command.

@justrajdeep

This comment has been minimized.

Copy link

justrajdeep commented Apr 11, 2018

@methane

I think removing --with-config-dir option is better approach. configure can detect right config dir from your python command

my idea was to have the reverse effect, --with-config-dir option to dictate which python to be used for vim. Shell might have a different version of python which might be broken still vim would be un-impacted.

@brammool

This comment has been minimized.

Copy link
Contributor

brammool commented Apr 11, 2018

Looking at this again, I wonder why we can specify the config directory with --with-python-config-dir,
but we cannot define the name of the python executable. It's fixed to search for "python2" and "python".
So, why don't we add an --with-python2 argument, and deprecate --with-python-config-dir ?

@methane

This comment has been minimized.

Copy link

methane commented Apr 11, 2018

So, why don't we add an --with-python2 argument, and deprecate --with-python-config-dir ?

I concur you. +1.

@methane

This comment has been minimized.

Copy link

methane commented Apr 12, 2018

There is --with-ruby-command option.
--with-python2-command and --with-python3-command will be consistent with it.

@ilovezfs ilovezfs referenced this pull request Apr 17, 2018

Closed

macvim: revision for python #26722

4 of 4 tasks complete

adizero pushed a commit to adizero/vim that referenced this pull request May 19, 2018

patch 8.0.1683: Python upgrade breaks Vim when defining PYTHON_HOME
Problem:    Python upgrade breaks Vim when defining PYTHON_HOME.
Solution:   Do not define PYTHON_HOME and PYTHON3_HOME in configure. (Naoki
            Inada, closes vim#2787)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment