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.
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
WIP: Python: wrap and patch using requiredPythonModules
instead of propagatedBuildInputs
#102613
base: staging
Are you sure you want to change the base?
Conversation
@@ -899,7 +899,7 @@ following are specific to `buildPythonPackage`: | |||
install`. To pass options to `python setup.py install`, use | |||
`--install-option`. E.g., `pipInstallFlags=["--install-option='--cpp_implementation'"]`. | |||
* `pythonPath ? []`: List of packages to be added into `$PYTHONPATH`. Packages | |||
in `pythonPath` are not propagated (contrary to `propagatedBuildInputs`). | |||
in `pythonPath` are not propagated (contrary to `requiredPythonModules`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need fixing
@@ -920,7 +920,7 @@ because their behaviour is different: | |||
* `checkInputs ? []`: Dependencies needed for running the `checkPhase`. These | |||
are added to `nativeBuildInputs` when `doCheck = true`. Items listed in | |||
`tests_require` go here. | |||
* `propagatedBuildInputs ? []`: Aside from propagating dependencies, | |||
* `requiredPythonModules ? []`: Aside from propagating dependencies, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need fixing
ddcd386
to
9d43a9c
Compare
ffe4603
to
6b5b552
Compare
6b5b552
to
68c327d
Compare
68c327d
to
619d93b
Compare
Also I personally have a little trouble reading and understanding very short lines which made reading the text a little bit hard. |
Thanks for notifying me. |
One consequence of this is that it will make backporting certain changes more difficult for people who are not aware of these changes. It might be nice to do have some deprecation where, "Using I have some locally packaged python packages that I use for work, and it would be a terrible user experience to try and debug why my previously working python packages aren't working. |
I was worried about evaluation performance regressions, but I could not find any degradation with |
About During the build, The alternative is to specify specifically which Python inputs should be added when (as suggested by @jonringer). That would mean having hooks running at different phases to setup |
Now that Python packages should use `requiredPythonModules` instead of `propagatedBuildInputs`, we could actually check it during evaluation time. This commit adds a warning in case Python modules are propagated. Now, in principle it is fine to propagate modules, but there is not really any good reason for it. Note application propagation is not affected by this because in that case `drv.pythonModule` is `false` or missing.
This could be made unnecessary if Python's setup hook would check the file in |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/nix-shell-buildinputs-ordering-issue/12885/4 |
I marked this as stale due to inactivity. → More info |
What's still left to do here? |
it's already implemented for the most part. The major thing is to make it so that people stop using propagatedBuildInputs by default. |
I think this will make 6f845f7#diff-c263b18f23de7c1f00ed846783c3b977718aae8cdfdd89fc82adcf365d1d8bebR108-R163 If we If we wait for #119942 to go in we could refer to |
Hello! I think this is a great improvement. Very often I see hundreds of python packages rebuilt without need because we don't have such patch. It's sad that it's become so stale. Now it has more than 3000 lines of conflict. I suggest a simpler path: just add a valid deprecation path that keeps old derivations working as usual, but supports this new way of doing it. This way, python packages can evolve gradually to adopt this new system. Also the PR won't have 3000 lines and it will be easier to review and merge. WDYT? |
Motivation for this change
Python programs look up their dependencies dynamically. To support this,
Python executables are wrapped and patched.
To find the dependencies to include when wrapping and patching, the
wrapPythonPrograms
function would traverse thepropagatedBuildInputs
.propagatedBuildInputs
are dependencies thatneed to be propagated during build time. While that is needed, we do
not want to have all propagated build inputs always as run-time
dependencies. Therefore, many derivations instead choose to set
pythonPath
, whichwrapPythonPrograms
also considers.pythonPath
,however, is just an attribute that is not recursed into.
This commit implements
requiredPythonModules
, which was already set inthe
passthru
of Python packages forpython.buildEnv
/python.withPackages
. Now, it is also passed to thebuilder, and
wrapPythonPrograms
will consider this environmentvariable instead of
propagatedBuildInputs
. This change will thusprevent unwanted bloat in wrappers. It also effectively replaces
pythonPath
. Note we could have usedpythonPath
for this, but I wantto avoid that name because what we do is not exactly what
PYTHONPATH
does.
Builders other than
buildPythonPackage
will typically need to passsomething like
for run-time Python dependencies.
This change is a prerequisite to unifying how we deal with wrappers in
Nixpkgs. E.g., now we can look into a common method for dealing with
wrappers for Qt, Python and GApps without getting bloat from Python
packages.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)Tasks
propagatedBuildInputs
treewidepythonPath
treewideThis is a backwards incompatible change. cc @jonringer as well as @adisbladis @DavHau @costrouc because of external tools.
Fixes #24128