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

Improve virtualenv support & egg-link resolution #636

Merged
merged 8 commits into from
Oct 26, 2015

Conversation

immerrr
Copy link
Contributor

@immerrr immerrr commented Oct 20, 2015

This PR implements the offline part of the functionality implemented in #562 so that it's easier to discuss and commit regardless of the decision about fetching path from live interpreter. It also extends venv-related tests a bit.

It should fix #443 and #384, also might help with #560 and #535.

@davidhalter
Copy link
Owner

Very awesome. Thanks a lot!! There are some tests failing (haven't looked into it).

@immerrr
Copy link
Contributor Author

immerrr commented Oct 21, 2015

@davidhalter it seems that the test failure is an issue in pytest, it cannot find a fixture and falls apart trying to report it (reported as pytest-dev/pytest#1148). I worked it around by adding testpaths = test to pytest.ini, but it might be yet another issue with doctest fixture discovery.

@immerrr
Copy link
Contributor Author

immerrr commented Oct 21, 2015

huh, travis caught an actual failure, let me look into it

@immerrr immerrr force-pushed the add-sys-path-customization branch from 9a1f649 to dcd95db Compare October 21, 2015 09:04
@immerrr
Copy link
Contributor Author

immerrr commented Oct 21, 2015

There's also a hack in emacs jedi epc server that refers to #36 and mentions caching issues. I have added a test to ensure that sys_path= kwarg plays well with caching, just in case.

@immerrr immerrr force-pushed the add-sys-path-customization branch from e77e9d0 to cb9ff53 Compare October 21, 2015 15:05
@immerrr
Copy link
Contributor Author

immerrr commented Oct 21, 2015

All green after rebasing on top of #637.

@davidhalter
Copy link
Owner

I like this pull request a lot. It's really fine work!!! :)

I'm not merging yet, for one minor reason. I think I have talked about #562 before:
I struggle a bit about just including it, because it allows (as far as I remember) execution of arbitrary Python code, which is not allowed in Jedi, yet. Is there no other option of doing it maybe in a different way (with Jedi introspection)? Which (well-known) packages does it benefit? Examples?

@immerrr
Copy link
Contributor Author

immerrr commented Oct 25, 2015

I have done a quick grep through pth files on my system and all of the custom code in pth files is either easy_install bootstrap code:

import sys; sys.__plen = len(sys.path)
./jeditestmodule-0.0.0-py2.7.egg
/home/immerrr/sources/jedi
/home/immerrr/sources/pytest
import sys; new = sys.path[sys.__plen:]; del sys.path[sys.__plen:]; p = getattr(sys, '__egginsert', 0); sys.path[p:p] = new; sys.__egginsert = p + len(new)

or namespace package bootstrap code:

import sys,types,os; p = os.path.join(sys._getframe(1).f_locals['sitedir'], *('sphinxcontrib',)); ie = os.path.exists(os.path.join(p,'__init__.py')); m = not ie and sys.modules.setdefault('sphinxcontrib',types.ModuleType('sphinxcontrib')); mp = (m or []) and m.__dict__.setdefault('__path__',[]); (p not in mp) and mp.append(p)

I think those can be skipped without any harm to sys.path detection. What worries me is that eval SMTH in site.addpackage function cannot be disabled, so either mocking or copy-pasting it is required.

@davidhalter
Copy link
Owner

Since we're just adding to the sys.path, we can just add al the paths to it, ignore the import lines and that's it? Or would it be more complicated?

- add sys_path= kwarg to Script & Evaluator constructors

- store sys_path for each evaluator instance

- replace get_sys_path with get_venv_path

- get_venv_path: use addsitedir to load .pth extension files

- get_venv_path: look for egg-link files in all directories in path
@immerrr immerrr force-pushed the add-sys-path-customization branch from e9a51f3 to cc139e8 Compare October 26, 2015 10:03
@immerrr
Copy link
Contributor Author

immerrr commented Oct 26, 2015

standard library employs case-sensitivity hacks, so it seems better to just pull in some well-debugged code under a permissive license than to reimplement it from scratch.

@immerrr
Copy link
Contributor Author

immerrr commented Oct 26, 2015

(which i have done)

@davidhalter
Copy link
Owner

Very, very nice! I like this PR a lot. Thanks so much!

It's a lot already, I'm really glad we didn't mix it with the VIRTUAL_ENV stuff anymore. Can you briefly describe in #562, what the leftovers are? What still would need to be done?

davidhalter added a commit that referenced this pull request Oct 26, 2015
Improve virtualenv support & egg-link resolution
@davidhalter davidhalter merged commit c50fc7a into davidhalter:dev Oct 26, 2015
@immerrr
Copy link
Contributor Author

immerrr commented Oct 26, 2015

There are several issues that are affected by this PR directly, #443 and #384 should be fixed now, #560 and #535 are a bit vague, but might be fixed too.

@immerrr immerrr deleted the add-sys-path-customization branch October 26, 2015 21:21
@davidhalter
Copy link
Owner

Wow that's a lot of fixes. :) Thanks a lot (for also searching all the issues).

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