Skip to content

Python fix venv #358823

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.

Sign up for GitHub

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

Open
MagicRB wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Python fix venv #358823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0362bb6
Fix venv creation in Python environments
Mar 22, 2024
809dd78
Unwrap python scripts when building an environment
Mar 22, 2024
0541966
Distinguish between a Python venv or not by environment variable
MagicRB Nov 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions pkgs/development/interpreters/python/cpython/2.7/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,8 +313,6 @@ in with passthru; stdenv.mkDerivation ({
inherit passthru;

postFixup = ''
# Include a sitecustomize.py file. Note it causes an error when it's in postInstall with 2.7.
cp ${../../sitecustomize.py} $out/${sitePackages}/sitecustomize.py
'' + lib.optionalString strip2to3 ''
rm -R $out/bin/2to3 $out/lib/python*/lib2to3
'' + lib.optionalString stripConfig ''
Expand Down
3 changes: 0 additions & 3 deletions pkgs/development/interpreters/python/cpython/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,9 +516,6 @@ in with passthru; stdenv.mkDerivation (finalAttrs: {
'' + optionalString stripTests ''
# 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
'' + optionalString stripBytecode ''
# Determinism: deterministic bytecode
# First we delete all old bytecode.
Expand Down
3 changes: 0 additions & 3 deletions pkgs/development/interpreters/python/pypy/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,6 @@ in with passthru; stdenv.mkDerivation rec {
ln -s $out/${executable}-c/include $out/include/${libPrefix}
ln -s $out/${executable}-c/lib-python/${if isPy3k then "3" else pythonVersion} $out/lib/${libPrefix}

# Include a sitecustomize.py file
cp ${../sitecustomize.py} $out/${if isPy38OrNewer then sitePackages else "lib/${libPrefix}/${sitePackages}"}/sitecustomize.py

runHook postInstall
'';

Expand Down
3 changes: 0 additions & 3 deletions pkgs/development/interpreters/python/pypy/prebuilt.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,6 @@ in with passthru; stdenv.mkDerivation {
echo "Removing bytecode"
find . -name "__pycache__" -type d -depth -delete

# Include a sitecustomize.py file
cp ${../sitecustomize.py} $out/${sitePackages}/sitecustomize.py

runHook postInstall
'';

Expand Down
3 changes: 0 additions & 3 deletions pkgs/development/interpreters/python/pypy/prebuilt_2_7.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,6 @@ in with passthru; stdenv.mkDerivation {
echo "Removing bytecode"
find . -name "__pycache__" -type d -depth -delete

# Include a sitecustomize.py file
cp ${../sitecustomize.py} $out/${sitePackages}/sitecustomize.py

runHook postInstall
'';

Expand Down
39 changes: 0 additions & 39 deletions pkgs/development/interpreters/python/sitecustomize.py
View file
Open in desktop

This file was deleted.

95 changes: 73 additions & 22 deletions pkgs/development/interpreters/python/tests.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,22 @@ let
is_nixenv = "False";
is_virtualenv = "False";
};
} // lib.optionalAttrs (!python.isPyPy && !stdenv.hostPlatform.isDarwin) {
# Use virtualenv from a Nix env.
# Fails on darwin with
# virtualenv: error: argument dest: the destination . is not write-able at /nix/store
nixenv-virtualenv = rec {
env = runCommand "${python.name}-virtualenv" {} ''
${pythonVirtualEnv.interpreter} -m virtualenv venv
mv venv $out
} // lib.optionalAttrs (!python.isPyPy) {
# Use virtualenv with symlinks from a Nix env.
nixenv-virtualenv-links = rec {
env = runCommand "${python.name}-virtualenv-links" {} ''
${pythonVirtualEnv.interpreter} -m virtualenv --system-site-packages --symlinks --no-seed $out
'';
interpreter = "${env}/bin/${python.executable}";
is_venv = "False";
is_nixenv = "True";
is_virtualenv = "True";
};
} // lib.optionalAttrs (!python.isPyPy) {
# Use virtualenv with copies from a Nix env.
nixenv-virtualenv-copies = rec {
env = runCommand "${python.name}-virtualenv-copies" {} ''
${pythonVirtualEnv.interpreter} -m virtualenv --system-site-packages --copies --no-seed $out
'';
interpreter = "${env}/bin/${python.executable}";
is_venv = "False";
Expand All @@ -61,27 +69,48 @@ let
is_nixenv = "True";
is_virtualenv = "False";
};
} // lib.optionalAttrs (python.isPy3k && (!python.isPyPy)) {
# Venv built using plain Python
} // lib.optionalAttrs (python.pythonAtLeast "3.8" && (!python.isPyPy)) {
# Venv built using links to plain Python
# Python 2 does not support venv
# TODO: PyPy executable name is incorrect, it should be pypy-c or pypy-3c instead of pypy and pypy3.
plain-venv = rec {
env = runCommand "${python.name}-venv" {} ''
${python.interpreter} -m venv $out
plain-venv-links = rec {
env = runCommand "${python.name}-venv-links" {} ''
${python.interpreter} -m venv --system-site-packages --symlinks --without-pip $out
'';
interpreter = "${env}/bin/${python.executable}";
is_venv = "True";
is_nixenv = "False";
is_virtualenv = "False";
};

} // {
} // lib.optionalAttrs (python.pythonAtLeast "3.8" && (!python.isPyPy)) {
# Venv built using copies from plain Python
# Python 2 does not support venv
# TODO: PyPy executable name is incorrect, it should be pypy-c or pypy-3c instead of pypy and pypy3.
plain-venv-copies = rec {
env = runCommand "${python.name}-venv-copies" {} ''
${python.interpreter} -m venv --system-site-packages --copies --without-pip $out
'';
interpreter = "${env}/bin/${python.executable}";
is_venv = "True";
is_nixenv = "False";
is_virtualenv = "False";
};
} // lib.optionalAttrs (python.pythonAtLeast "3.8") {
# Venv built using Python Nix environment (python.buildEnv)
# TODO: Cannot create venv from a nix env
# Error: Command '['/nix/store/ddc8nqx73pda86ibvhzdmvdsqmwnbjf7-python3-3.7.6-venv/bin/python3.7', '-Im', 'ensurepip', '--upgrade', '--default-pip']' returned non-zero exit status 1.
nixenv-venv = rec {
env = runCommand "${python.name}-venv" {} ''
${pythonEnv.interpreter} -m venv $out
nixenv-venv-links = rec {
env = runCommand "${python.name}-venv-links" {} ''
${pythonEnv.interpreter} -m venv --system-site-packages --symlinks --without-pip $out
'';
interpreter = "${env}/bin/${pythonEnv.executable}";
is_venv = "True";
is_nixenv = "True";
is_virtualenv = "False";
};
} // lib.optionalAttrs (python.pythonAtLeast "3.8") {
# Venv built using Python Nix environment (python.buildEnv)
nixenv-venv-copies = rec {
env = runCommand "${python.name}-venv-copies" {} ''
${pythonEnv.interpreter} -m venv --system-site-packages --copies --without-pip $out
'';
interpreter = "${env}/bin/${pythonEnv.executable}";
is_venv = "True";
Expand All @@ -93,11 +122,33 @@ let
testfun = name: attrs: runCommand "${python.name}-tests-${name}" ({
inherit (python) pythonVersion;
} // attrs) ''
mkdir $out

# set up the test files
cp -r ${./tests/test_environments} tests
chmod -R +w tests
substituteAllInPlace tests/test_python.py
${attrs.interpreter} -m unittest discover --verbose tests #/test_python.py
mkdir $out

# run the tests by invoking the interpreter via full path
echo "absolute path: ${attrs.interpreter}"
${attrs.interpreter} -m unittest discover --verbose tests 2>&1 | tee "$out/full.txt"

# run the tests by invoking the interpreter via $PATH
export PATH="$(dirname ${attrs.interpreter}):$PATH"
echo "PATH: $(basename ${attrs.interpreter})"
"$(basename ${attrs.interpreter})" -m unittest discover --verbose tests 2>&1 | tee "$out/path.txt"

# make sure we get the right path when invoking through a result link
ln -s "${attrs.env}" result
relative="result/bin/$(basename ${attrs.interpreter})"
expected="$PWD/$relative"
actual="$(./$relative -c "import sys; print(sys.executable)" | tee "$out/result.txt")"
if [ "$actual" != "$expected" ]; then
echo "expected $expected, got $actual"
exit 1
fi

# if we got this far, the tests passed
touch $out/success
'';

Expand Down
2 changes: 1 addition & 1 deletion pkgs/development/interpreters/python/tests/test_environments/test_python.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def test_site_prefix(self):

@unittest.skipIf(IS_PYPY or sys.version_info.major==2, "Python 2 does not have base_prefix")
def test_base_prefix(self):
if IS_VENV or IS_NIXENV or IS_VIRTUALENV:
if IS_VENV or IS_VIRTUALENV:
self.assertNotEqual(sys.prefix, sys.base_prefix)
else:
self.assertEqual(sys.prefix, sys.base_prefix)
Expand Down
6 changes: 4 additions & 2 deletions pkgs/development/interpreters/python/wrap-python.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ makePythonHook {
import sys
import site
import functools
sys.argv[0] = '"'$(readlink -f "$f")'"'
functools.reduce(lambda k, p: site.addsitedir(p, k), ['"$([ -n "$program_PYTHONPATH" ] && (echo "'$program_PYTHONPATH'" | sed "s|:|','|g") || true)"'], site._init_pathinfo())
import os
_nix_direct = os.environ.pop("NIX_PYTHON_IN_ENV", None) != "true"
sys.argv[0] = '"'$(readlink -f "$f")'"' if _nix_direct else sys.argv[0]
functools.reduce(lambda k, p: site.addsitedir(p, k), ['"$([ -n "$program_PYTHONPATH" ] && (echo "'$program_PYTHONPATH'" | sed "s|:|','|g") || true)"'], site._init_pathinfo()) if _nix_direct else None

@ruro ruro Jan 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I am still kind of conflicted about all this wrapping/unwrapping nonsense.

It seems to me that the sys.argv and site.addsitedir shenanigans here are basically doing the same thing as the removed sitecustomize.py. So shouldn't we also remove this "preamble" and instead just set substitutions.executable to an instance of python that already has all the required dependencies?

@ruro ruro Jan 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought about it some more, and it seems to me that the primary reason why we can't do this is that it would create a circular reference.

Ideally, instead of doing site.addsitedir at runtime, the shebang in /nix/store/aaaa-python3.12-foo-1.0.0/bin/foo should just be replaced with #!/nix/store/bbbb-python3-3.12.8-env/bin/python. Where bbbb is an instance of python that would just "pick up" the correct environment (from /nix/store/bbbb-python3-3.12.8-env/lib/python3.12/site-packages).

However, /nix/store/bbbb-python3-3.12.8-env/lib/python3.12/site-packages would also need to contain symlinks to /nix/store/aaaa-python3.12-foo-1.0.0/lib/python3.12/site-packages/libfoo (because bin/foo also needs libfoo). But this is not possible since this would create a circular dependency aaaa (bin/foo) -> bbbb (bin/python) -> bbbb (lib/*/site-packages) -> aaaa (lib/*/site-packages/libfoo).

I think that this could be solved without creating any circular references if #170577 (or something like it) were to be implemented. Alternatively, the whole environment could be included inside the aaaa derivation (something like /nix/store/aaaa-python3.12-foo-1.0.0/nix-support/env/...).

Honestly, all of this is kind of yucky 😒 (less yucky than the current situation, of course).

@MagicRB MagicRB Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You lost me :) i don't know nearly enough about python to effectively and correctly implement your suggestions. I went to fix the immediate problem if venvs being slightly broken, which I managed. I can implement the magic comment approach but as for fixing up the PR you linked, its probably beyond me

@ruro ruro Jan 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, @cwp and some other people were also considering removing the wrapping/unwrapping logic, but it was never actually implemented. My comments above were mainly my way of documenting my thought process for why removing this extra wrapping is more complicated than it might seem at first glance.

So yeah, this is mostly me talking to myself, but also kind of hoping that somebody with a better understanding of the current python3Packages machinery might swing by and sanity check my thoughts.

Removing the preamble-wrap thing should probably be beyond the scope of this PR.

@MagicRB MagicRB Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah no worries feel free to think out-loud, it helps and your input is very welcome. Might be good merge this, if no one has objections so that we have a partial if cursed solution already in. Dunno if anyone who knows enough about the python infra will show up soon.

'';

in ''
Expand Down
10 changes: 9 additions & 1 deletion pkgs/development/interpreters/python/wrapper.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,15 @@ let
if [ -f "$prg" ]; then
rm -f "$out/bin/$prg"
if [ -x "$prg" ]; then
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}

@ruro ruro Jan 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the only place where pythonPath was used in this file, so it should probably be removed from the above let-in.

if [ -f ".$prg-wrapped" ] && ( cat ".$prg-wrapped" | head -n 1 | grep -q "python" ) ; then
echo "#!${pythonExecutable}" >> "$out/bin/$prg"
echo "import os" >> "$out/bin/$prg"
echo 'os.environ["NIX_PYTHON_IN_ENV"] = "true"' >> "$out/bin/$prg"

@ruro ruro Jan 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we assume that we really DO need to wrap/unwrap python scripts, I still don't like that we have to write to the environment here simply to disable a piece of code a few lines later in the same file.

We are using the magic NIX_PYTHON_IN_ENV variable here, but the only other place this variable is used in nixpkgs is in the "preamble" in wrap-python.nix. Why do we need to touch the environment (or search the correct line to delete like in the previous iteration of this code)?

Since we control both the wrapping and the unwrapping logic, couldn't we massively simplify all this. Keep the preamble as it was before (always set sys.argv[0] and call site.addsitedir), but add a "magic comment" to the end (say, # NIX_PYTHON_ENV_PREAMBLE). Then, in the unwrapping code, just delete the line with the magic comment (sed '/NIX_PYTHON_ENV_PREAMBLE/d').

@ruro ruro Jan 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it might be better to replace the old shebang with the new one, instead of just prepending the new one. Prepending the new shebang means that the resulting derivation contains references to both the old and the new python executable, which might be confusing/suboptimal in terms of why-depends/requisites/closures.

@MagicRB MagicRB Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right the magic comment thing would work and is quite easy to do. I'm personally hesitant to do it that way because deleting anything carries the risk of breaking things. Could be that at some point something else in nixpkgs wraps a wrapped python executable and then this code tries to undo the preamble which proceeds to delete the other wrap because it ended up on the same line. I know it's very unlikely but it just doesn't seem like a clean approach, neither is my solution, it's awful in a different way.

As for the still remaining reference, yeah you're right I didn't think of that. With my solution it's not possible to get rid of it.

Thanks for the daily drive, I don't use any python things on my laptop sadly.

@ruro ruro Jan 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be that at some point something else in nixpkgs wraps a wrapped python executable and then this code tries to undo the preamble which proceeds to delete the other wrap because it ended up on the same line.

I don't think that this is possible, because that would imply that this hypothetical wrapping code inserts the code onto the same line with already existing code (which is broken anyway) instead of inserting a new line (like this wrapping code does).

If anything, I think that my proposed solution might be slightly more robust, because it would actually verify (by checking for the presence of the magic comment) that the script that we are trying to unwrap was actually wrapped by wrap-python.nix.

Also, I noticed, that the current code doesn't clean up the .$prg-wrapped scripts after replacing $prg with the proper "unwrapped" version.

As for the still remaining reference, yeah you're right I didn't think of that. With my solution it's not possible to get rid of it.

I think, that once we've verified that the NIX_PYTHON_ENV_PREAMBLE comment is present in the file, removing the old shebang line should be 100% safe, since the shebang line must be the first line in the script for it to function as a shebang in the first place.

Please, let me know if you are planning on adding any of the changes that I've suggested in the previous review. No pressure, if you are busy or unwilling to add these changes (I understand your reasoning for implementing the current approach, even if I still think that we ought to try to remove as much of this wrapping/unwrapping cruft as possible). I'd just like to know if it's worth waiting for you to add these changes or if I should make my own PR.

@ruro ruro Jan 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, now that I started thinking about the .$prg-wrapped scripts, I am suddenly unsure if the current "unwrapping" implementation is 100% correct.

The "wrapping" performed in wrap-python.nix consists of two parts:

  1. $prg - is a bash script produced by wrapProgram that add stuff to $PATH, sets PYTHONNOUSERSITE and then calls .$prg-wrapped (2)
  2. .$prg-wrapped - is a patched version of the original python script that is being wrapped, but with a magic preamble that sets argv[0] back to $prg (1) and sets up the site dir appropriately

The current "unwrapping" logic replaces $prg with .$prg-wrapped, but with the argv[0] and site dir logic disabled and with a different shebang (pointing to the python interpreter with the correct environment setup).

However, this might be subtly wrong, since it only accounts for (2) in the original wrapping logic. At the very least, PYTHONNOUSERSITE is no longer being set. Additionally, PATH isn't being set correctly (although it seems that this problem is present in the current master anyway). I think that ideally, the currently constructed bin folder should be added to the PATH.

cat ".$prg-wrapped" >> "$out/bin/$prg"
chmod +x "$out/bin/$prg"
else
makeWrapper "$path/bin/$prg" "$out/bin/$prg" --inherit-argv0 --resolve-argv0 ${lib.optionalString (!permitUserSite) ''--set PYTHONNOUSERSITE "true"''} ${lib.concatStringsSep " " makeWrapperArgs}
fi
fi
fi
done
Expand Down