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.

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

Fix Python venv creation #326094

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Fix Python venv creation #326094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2472acf
Fix venv creation in Python environments
cwp Mar 22, 2024
74fd490
Unwrap python scripts when building an environment
cwp Mar 22, 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 @@ -318,8 +318,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: 1 addition & 2 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 @@ -508,8 +508,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

Copy link
Member

Choose a reason for hiding this comment

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

There's an empty string left here, I think that's not needed?

'' + 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 @@ -126,9 +126,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) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it intentional that you repeat line 41 here? Could be one block

# 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)) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for python 3.8 is probably because python 2 doesn't support venv. Quoting from virtualenvs website:

Since Python 3.3, a subset of it has been integrated into the standard library under the venv module.

# 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
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 @@ -35,14 +35,22 @@ let
fi
mkdir -p "$out/bin"

rm -f $out/bin/.*-wrapped
Copy link
Member

Choose a reason for hiding this comment

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

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?


for path in ${lib.concatStringsSep " " paths}; do
if [ -d "$path/bin" ]; then
cd "$path/bin"
for prg in *; do
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}
if [ -f ".$prg-wrapped" ]; then
echo "#!${pythonExecutable}" > "$out/bin/$prg"
sed -e '1d' -e '3d' ".$prg-wrapped" >> "$out/bin/$prg"
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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://github.com/pretalx/pretalx/blob/v2024.1.0/src/manage.py

missing the os import.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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"

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