Python: fix virtualenv with Python 2. #88756
Conversation
...ent/python-modules/virtualenv/0001-Check-base_prefix-and-base_exec_prefix-for-Python-2.patch
Show resolved
Hide resolved
...ent/python-modules/virtualenv/0001-Check-base_prefix-and-base_exec_prefix-for-Python-2.patch
Show resolved
Hide resolved
Too many tests set it.
There was a problem hiding this comment.
Ay, i totally messed up this review thing. Did not use this feature so often on github and therefore forgot to submit.
But now that you gave a link to a pull request on cpython where pyvenv.cfg also was present, i thought it might be relevant.
Also you are very quick on merging @FRidh.
I am not that fast, especially on a subject i am really not accustomed to.
| # For Python 3 we check whether our `base_prefix` is different from our current `prefix`. | ||
| # For Python 2 we check whether the non-standard `real_prefix` is set. | ||
| # https://stackoverflow.com/questions/1871549/determine-if-python-is-running-inside-virtualenv | ||
| in_venv = (sys.version_info.major == 3 and sys.prefix != sys.base_prefix) or (sys.version_info.major == 2 and hasattr(sys, "real_prefix")) |
There was a problem hiding this comment.
I wonder if it is more aproriate to use pyvenv.cfg from module-site but i must admit i only have partial understanding of the matter.
If a file named “pyvenv.cfg” exists one directory above sys.executable, sys.prefix and sys.exec_prefix are set to that directory and it is also checked for site-packages (sys.base_prefix and sys.base_exec_prefix will always be the “real” prefixes of the Python installation). If “pyvenv.cfg” (a bootstrap configuration file) contains the key “include-system-site-packages” set to anything other than “true” (case-insensitive), the system-level prefixes will not be searched for site-packages; otherwise they will.
There was a problem hiding this comment.
At least, it seems virtualenv does something with the pyvenv.cfg, if present (search in virtualenv github), but that needs more investigation to verify.
There was a problem hiding this comment.
I mean, maybe there is a chance venv and virtualenv just doing the right thing if we put that pyvenv.cfg into the wrapped python folder. I don't know. What do you think? Is this something worth to investigate?
There was a problem hiding this comment.
pyvenv.cfg is still Python 3-only, which is a downside. An alternative I've also considered is setting instead PYTHONUSERBASE. But that won't be sufficient for the non-withPackages case such as executables in applications.
Anyway, there is no clear solution to this problem because the problem we need to solve is not a typical problem for which the tooling has been designed. It's a matter of balancing things to come to a "sufficiently working" setup.
At least now by improving test coverage we can test whether any new method will really be better or not. And this change is an improvement.
There was a problem hiding this comment.
At least now by improving test coverage we can test whether any new method will really be better or not. And this change is an improvement.
Agree to that.
Right now i started to get a clue on the python nixos tests.
I noticed we don't have integration tests for the cases we are in any virtual env, is this true or did i miss them?
I will try to extend that so that we can test 'import sqlite3' (for std lib) and 'import sitepkg' with any of the virtual env pythons.
There was a problem hiding this comment.
Yes, in pkgs/development/interpreters/python/tests/test_python.py. There it tests the prefixes for Nix envs, virtualenvs and venvs. That could be extended.
Note these are not NixOS tests, as in, they do not run in a VM.
There was a problem hiding this comment.
Yes, ok, then i will make a PR for that. Thanks, for the clarification also.
There was a problem hiding this comment.
@FRidh i have done it. But am waiting right now until my local staging is build to check if everything applies. Btw. as the change somehow depends on the merged commits to staging should this PR be opened against staging or master?
There was a problem hiding this comment.
Ay, ok. that i never did before and was scared because of so many changes when github compared against master by default.
Motivation for this change
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)cc @calbrecht