Fix Python venv creation #326094
Fix Python venv creation #326094
Conversation
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
| makeWrapper "$path/bin/$prg" "$out/bin/$prg" --set NIX_PYTHONPREFIX "$out" --set NIX_PYTHONEXECUTABLE ${pythonExecutable} --set NIX_PYTHONPATH ${pythonPath} ${lib.optionalString (!permitUserSite) ''--set PYTHONNOUSERSITE "true"''} ${lib.concatStringsSep " " makeWrapperArgs} | ||
| if [ -f ".$prg-wrapped" ]; then | ||
| echo "#!${pythonExecutable}" > "$out/bin/$prg" | ||
| sed -e '1d' -e '3d' ".$prg-wrapped" >> "$out/bin/$prg" |
There was a problem hiding this comment.
The main reason this original PR got reverted, was the assumption that you can undo every wrapper by deleting those lines which sometimes deleted legit python code like for pretalx.
There was a problem hiding this comment.
And it still deletes part of pretalx-manage:
#!/nix/store/pc8n829fmq2494s5vadw7cppiwfn68a7-python3-3.12.4-env/bin/python3.12
import sys;import site;import functools;sys.argv[0] = '/nix/store/07rcj9vcgvmsq7aqgi39csw7ypsyv990-pretalx-2024.1.0/bin/pretalx-manage';functools.reduce(lambda k, p: site.addsitedir(p, k), [';
import sys
if __name__ == "__main__":
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "pretalx.settings")
from django.core.management import execute_from_command_line
execute_from_command_line(sys.argv)
vs https:/pretalx/pretalx/blob/v2024.1.0/src/manage.py
missing the os import.
There was a problem hiding this comment.
@RuRo suggests a somewhat hardened sed expression in https://discourse.nixos.org/t/how-to-build-python-virtualenv-with-packages-provided-by-python3-withpackages/24766/20
sed \
-e '/^#!\/nix\/store\//d' \
-e '/^import sys;import site;import functools;sys\.argv\[0\] = /d' \
".$prg-wrapped" >> "$out/bin/$prg"
| The module recursively adds paths that are on `NIX_PYTHONPATH` to `sys.path`. In | ||
| order to process possible `.pth` files `site.addsitedir` is used. |
There was a problem hiding this comment.
Does dropping sitecustomize.py mean that packages whose correct behavior relies on .pth files (like setuptools as discovered in #326321 (comment)) would be broken? Or have I overthought this?
There was a problem hiding this comment.
My understanding is that the logic in sitecustomize.py was essentially manually emulating what normally happens with the correct base_prefix set, including the .pth logic.
Assuming this PR works as intended, then the base_prefix would be set to the output path of the -env python derivation and so the ${sys.base_prefix}/lib/python3.X/site-packages directory (that includes all the required package files) would automatically be considered a site directory (and thus all the .pth files inside it would be executed when they are supposed to).
| @@ -566,8 +566,7 @@ in with passthru; stdenv.mkDerivation (finalAttrs: { | |||
| # Strip tests | |||
| rm -R $out/lib/python*/test $out/lib/python*/**/test{,s} | |||
| '' + optionalString includeSiteCustomize '' | |||
| # Include a sitecustomize.py file | |||
| cp ${../sitecustomize.py} $out/${sitePackages}/sitecustomize.py | |||
There was a problem hiding this comment.
There's an empty string left here, I think that's not needed?
| is_nixenv = "False"; | ||
| is_virtualenv = "False"; | ||
| }; | ||
| } // lib.optionalAttrs (python.pythonAtLeast "3.8" && (!python.isPyPy)) { |
There was a problem hiding this comment.
Is it intentional that you repeat line 41 here? Could be one block
Also a comment why it needs to be at least python 3.8 would be nice.
The current comments around pypy executables is already quite confusing when it's in a block that doesn't support pypy anyway.
| is_nixenv = "True"; | ||
| is_virtualenv = "True"; | ||
| }; | ||
| } // lib.optionalAttrs (!python.isPyPy) { |
There was a problem hiding this comment.
Is it intentional that you repeat line 41 here? Could be one block
| @@ -35,14 +35,22 @@ let | |||
| fi | |||
| mkdir -p "$out/bin" | |||
| rm -f $out/bin/.*-wrapped | |||
There was a problem hiding this comment.
Are we sure that every wrapped executable in all of paths are python scripts? Otherwise this seems risky to me, but i might be missing something?
A short comment explaining why this is needed and safe would be appreciated. Is it just for the check below?
|
You might want to consider this one-line alternative: #261801 |
FYI, as far as I can tell, that PR doesn't actually fix the problem, but just adds a nix-specific way to work around the problem. With that PR, nested So properly fixing venvs is still probably desirable. |
|
Yes, indeed that's what it does. And of course a proper fix is better. But I think #261801 is still valuable because:
This one PR is still draft after 1 month. I'm afraid it could become stale, or reach a dead end just like #302385 did. And if it doesn't... perfect! Just remove my workaround once it works. Still no reason to ignore #261801 IMHO 😊 |
well, I am not sure. Right now we are always overwriting the variable and nesting could surely trigger some edge cases. |
|
Sorry, missclicked on mobile |
|
I also tried to take a stab at this. It looks like it is a hell of a job to develop, as experimenting with the simplest Python derivation results in mass rebuilds. My idea to circumvent this while testing different approaches is to copy the related wrapping code, and wire up a new python derivation to stop cascading builds and also have some output that I can experiment with. IDK if it is a viable approach. |
|
@vlaci my understanding is that the hardened But I am totally in agreement with you, that fixing this issue is a pain due to the need to rebuild python itself and everything python-related from source. Also, the fact that the previous PR was reverted due to regressions indicates that python packages might not have sufficient integration tests, so it's borderline impossible to get this right without breaking things. |
The way we build python environments is subtly broken. A python
environment should be semantically identical to a vanilla Python
installation in, say, /usr/local. The current implementation, however,
differs in two important ways. The first is that it's impossible to use
python packages from the environment in python virtual environments. The
second is that the nix-generated environment appears to be a venv, but
it's not.
This commit changes the way python environments are built:
* When generating wrappers for python executables, we inherit argv[0]
from the wrapper. This causes python to initialize its configuration
in the environment with all the correct paths.
* We remove the sitecustomize.py file from the base python package.
This file was used tweak the python configuration after it was
incorrectly initialized. That's no longer necessary.
The end result is that python environments no longer appear to be venvs,
and behave more like a vanilla python installation. In addition it's
possible to create a venv using an environment and use packages from
both the environment and the venv.
(cherry picked from commit 234bb31)
When building a python environment's bin directory, we now detect wrapped python scripts from installed packages, and generate unwrapped copies with the environment's python executable as the interpreter. (cherry picked from commit 9611885)
mcdonc
force-pushed
the
python-env-venv
branch
from
b9b1a15
to
74fd490
Compare
This is a reapplication of commits by @cwp from #297628, which aimed to fix Python virtualhost creation on top of a newer master. The original PR was closed after it caused regressions. The related Discourse discussion is at https://discourse.nixos.org/t/how-to-build-python-virtualenv-with-packages-provided-by-python3-withpackages/24766
https://hydra.nixos.org/jobset/nixpkgs/python-env-venv#tabs-evaluations