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

adapt to win32 Lib path. #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

adapt to win32 Lib path. #124

wants to merge 1 commit into from

Conversation

t2psyto
Copy link

@t2psyto t2psyto commented Jan 22, 2014

Lib path of windows is different to others.
virtualenv.path_locations() is useful for this.

  • win32 => "$VIRTUALENV_HOME/Lib/site-package"
  • others => "$VIRTUALENV_HOME/lib/pythonXX.X/site-package"

Lib path of windows is different to others.
virtualenv.path_locations() is useful for this.

 - win32 => "$VIRTUALENV_HOME/Lib/site-package"
 - others => "$VIRTUALENV_HOME/lib/pythonXX.X/site-package"
@tkf
Copy link
Owner

tkf commented Feb 23, 2014

Hi, I like the idea of virtualenv.path_locations but we cannot use this here because the Python instance used to start jedi epc server may not have virtualenv module. For example, the Python instance can be system-wide one in which virtualenv is not installed and user may use jediepcserver.py --virtual-env SOME_VENV to add SOME_VENV. Python side of the test with tox fails due to the same reason if you set VIRTUAL_ENV.

It looks like making win32 version of path can be done in one line. So why not do it without virtualenv?

@t2psyto
Copy link
Author

t2psyto commented Feb 24, 2014

Actually, in environment which has not installed virtualenv, VIRTUAL_ENV is not set and add_virtualenv_path() returns before execute "import virturlenv".
Would it be possible to change tox test for this behavior?

@tkf
Copy link
Owner

tkf commented Feb 25, 2014

Consider this

$ echo $VIRTUAL_ENV
/home/tkf/.virtualenvs/SOME-ENVIRONMENT
$ which python
/home/tkf/.virtualenvs/SOME-ENVIRONMENT/bin/python
$ /usr/bin/python -m virtualenv
/usr/bin/python: No module named virtualenv

/usr/bin/python cannot import virtualenv but we should tell Jedi to look into SOME-ENVIRONMENT, at least when --virtual-env /home/tkf/.virtualenvs/SOME-ENVIRONMENT is given. If /usr/bin/python is used to invoke jediepcserver.py in your patch, it is impossible to add virtualenv path. We are currently supporting this behavior and I don't want break it for now, if code we need to add is not huge.

@t2psyto
Copy link
Author

t2psyto commented Feb 25, 2014

sorry, I cannot image "/usr/bin/python -m virtualenv" => "No module named virtualenv".

I think, before you create "/home/tkf/.virtualenvs/SOME-ENVIRONMENT",
you need to do command "virtualenv /home/tkf/.virtualenvs/SOME-ENVIRONMENT".

And to use virtualenv command, it is need to do "easy_install virtualenv" on "/usr/bin/python" environment.

Following that way, "/usr/bin/python -m virtualenv" is callable.

Another way to create ".virtualenvs/SOME-ENVIRONMENT" is by not using virtualenv command, (= manualy).

@tkf
Copy link
Owner

tkf commented Feb 25, 2014

Then read my comment by replacing /usr/bin/python with something like ~/.pyenv/shims/python3.3.

@tkf
Copy link
Owner

tkf commented Feb 25, 2014

another thing to consider: Is it a public Python API of virtualenv?

@t2psyto
Copy link
Author

t2psyto commented Feb 26, 2014

virtualenv.pathlocations() is public.
For example, it is used in pip's test code.
https://github.com/pypa/pip/blob/1.5.4/tests/lib/venv.py#L29

code of virtualenv.pathlocations()::
https://github.com/pypa/virtualenv/blob/1.11.4/virtualenv.py#L999

I think, if there is virtualenv environment, we can assume virtualenv is installed on its parental python enviroment...

virtualenv.path_locations's Lib-path treatment is better.

If it is important to imprement without virtualenv library,
I suggest, instead of import, copy/paste from a part of virtualenv.path_locations()'s code.

@tkf
Copy link
Owner

tkf commented Feb 26, 2014

Considering Pythonista's obsession about documentation, I'd say it's not public API if you can't find a documentation. Do you find one?

I am not afraid of using internal API for testing. For production... that's a bit scary (though sometimes it's OK since "practicality beats purity").

we can assume virtualenv is installed on its parental python enviroment

I've been explaining the case you are using non-"parental" python executable to run jediepcserver.py. Consider:

  1. You install virtualenv with /usr/bin/python2.7
  2. You build Python 3.3 and install it to ~/.local/bin/python3.3
  3. You are in virtual environment created by the virtualenv which is run by python2.7
  4. You run python3.3 -c 'import virtualenv'

This causes an import error, right?

@t2psyto
Copy link
Author

t2psyto commented Feb 26, 2014

I've got understand.
I cancel this Pull Request. and will try to resolve without virturalenv.path_locations().

@t2psyto
Copy link
Author

t2psyto commented Feb 26, 2014

Is it ok to copy/paste from virtualenv.path_locations()'s code ?

@tkf
Copy link
Owner

tkf commented Feb 26, 2014

Ah, I guess we just have to add virtualenv to requirements.txt. Then we can always import virtualenv like you do and it's cleaner. The only concern is if it is public or not. If not... we should wrap it by try except ImportError to not fail when it is changed.

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.

2 participants