python: prepare to drop legacy wrapper and adjust how it's applied #2234
Conversation
…ons of nixpkgs This can also be toggled manually if needed.
🔍 Suggested ReviewersBased on git blame analysis of the changed lines, the following contributors have significant experience with the modified code:
Please consider reviewing this PR as you have authored significant portions of the code being modified. Your expertise would be valuable! 🙏 This comment was automatically generated by git-blame-auto-reviewer Last updated: 2025-10-23T16:49:17.396Z |
sandydoo
changed the title
Deploying devenv with
|
| Latest commit: |
74f437e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f216645d.devenv.pages.dev |
| Branch Preview URL: | https://fix-1218.devenv.pages.dev |
| @@ -582,7 +657,7 @@ in | |||
| }; | |||
|
|
|||
| enterShell = '' | |||
| export PYTHONPATH="$DEVENV_PROFILE/${package.sitePackages}''${PYTHONPATH:+:$PYTHONPATH}" | |||
| export PYTHONPATH="$DEVENV_PROFILE/${cfg.package.sitePackages}''${PYTHONPATH:+:$PYTHONPATH}" | |||
There was a problem hiding this comment.
question: Why do we even need to set PYTHONPATH. We should never tinker with that if possible?
There was a problem hiding this comment.
As I see this resolves to DEVENV_PROFILE/lib/python.../site-packages. for what is that path and what does it contain?
There was a problem hiding this comment.
No clue. Maybe an old attempt at doing what the wrapper does?
There was a problem hiding this comment.
f151daf#diff-f5bb89f776eda1a758db67c6c75b19ff1665b2f10998d17d1d98479835fc9e2eR482
This is from a large tasks PR. Let's try removing it.
There was a problem hiding this comment.
This PYTHONPATH is critical in making site packages (withPackages) available to venvs created with venv.enable.
There was a problem hiding this comment.
ah weird: ok, so its an empty folder which gets filled by venv.enable I guess
| { | ||
| languages.python = { | ||
| enable = true; | ||
| package = pkgs.python3.withPackages (ps: [ |
sandydoo
changed the title
|
Added a test to verify We now also patch This PR still needs to be tested against the upstream nixpkgs PR. Once it's out of staging, we can verify this easily without rebuilding the universe. |
| ps.tkinter | ||
| ]); | ||
| patches.buildEnv.enable = true; | ||
| venv.enable = true; |
There was a problem hiding this comment.
suggestion: could we add a uv test as well? with uv.enable = true and uv.sync = true or so
pyproject.toml
[project]
name = "mymodule"
description = "test"
version = "0.0.1"
dependencies = [
"request",
"pytest",
]| # Check that sys.base_prefix points to the -env buildEnv, not the bare interpreter | ||
| assert "-env" in sys.base_prefix, \ | ||
| f"sys.base_prefix ({sys.base_prefix}) should point to python-env with packages, not bare interpreter" |
|
Shall we merge this? |
|
suggestion: we need another uv test as uv is the predominant tool |
|
but should not hold it back! |
This prepares us for the arrival of several upstream patches (including NixOS/nixpkgs#442540) that will make it possible to drop the python wrapper.
This also changes the way the old wrapper is applied.
withPackagesare now propagated through the wrapperWe no longer overrideOkay, this one was too good to be trueLD_LIBRARY_PATH, which should resolve a bunch of issues with python subprocesses.TODO: adjust the issue list
Fixes #1807.
Fixes #1792.
Fixes #1486. (possibly earlier)
Fixes #1335.
Fixes #1218.
Fixes #1111.
Fixes #773.