WIP: Python: wrap and patch using `requiredPythonModules` instead of `propagatedBuildInputs` by FRidh · Pull Request #102613 · NixOS/nixpkgs

FRidh · 2020-11-03T11:05:25Z

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 the
propagatedBuildInputs. propagatedBuildInputs are dependencies that
need 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, which wrapPythonPrograms also considers.pythonPath,
however, is just an attribute that is not recursed into.

This commit implements requiredPythonModules, which was already set in
the passthru of Python packages for
python.buildEnv/python.withPackages. Now, it is also passed to the
builder, and wrapPythonPrograms will consider this environment
variable instead of propagatedBuildInputs. This change will thus
prevent unwanted bloat in wrappers. It also effectively replaces
pythonPath. Note we could have used pythonPath for this, but I want
to avoid that name because what we do is not exactly what PYTHONPATH
does.

Builders other than buildPythonPackage will typically need to pass
something like

requiredPythonModules = with python3.pkgs; computeRequiredPythonModules [ sphinx ];

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

Tasks

Tested diffoscope. Closure size remained the same.
change log stating backwards incompatible change
update docs
replace propagatedBuildInputs treewide
replace pythonPath treewide

This is a backwards incompatible change. cc @jonringer as well as @adisbladis @DavHau @costrouc because of external tools.

Fixes #24128

FRidh · 2020-11-03T11:09:53Z

doc/languages-frameworks/python.section.md

  `--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`).


need fixing

FRidh · 2020-11-03T11:10:01Z

doc/languages-frameworks/python.section.md

  are added to `nativeBuildInputs` when `doCheck = true`. Items listed in
  `tests_require` go here.
-* `propagatedBuildInputs ? []`: Aside from propagating dependencies,
+* `requiredPythonModules ? []`: Aside from propagating dependencies,


need fixing

pkgs/top-level/python-packages.nix

SuperSandro2000 · 2020-11-04T00:53:46Z

computeRequiredPythonModules this will be very hard to type correct on first try. I think it would be easier if the name would be shorter.

Also I personally have a little trouble reading and understanding very short lines which made reading the text a little bit hard.

DavHau · 2020-11-04T05:14:16Z

Thanks for notifying me.
It would be nice if backwards incompatible changes like this would be discoverable, so that tools like mach-nix can operate on different versions of nixpkgs. Right now one could check for the existence of python3.pkgs.computeRequiredPythonModules to see if the new API needs to be used or not.
Though, it would be nicer if the version of nixpkgs' python-API would be versioned and that version was discoverable.

jonringer · 2020-11-05T17:15:35Z

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 propagatedBuildInputs in a buildPython{Package,Application} has been deprecated, please use requirePythonModules instead". There may be some cases where someone wanted to propagate an actual build dependency and they will need to "endure" the warning for a period, but i would say in the 99% case, it's going to be just python packages.

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.

FRidh · 2021-04-04T16:09:45Z

I was worried about evaluation performance regressions, but I could not find any degradation with NIX_SHOW_STATS=1. Quite the contrary, with every package I checked every stat dropped in value.

FRidh · 2021-04-04T16:17:08Z

About requiredPythonModules. As it is, it is only relevant for storing the runtime dependencies, such as for wrapping.

During the build, PYTHONPATH is configured using Python's setup hook which considers the hostOffset. The pro of this is that it is a simple solution requiring only buildInputs to be set. The con is that it does not distinguish between build and check inputs.

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 PYTHONPATH as well as PATH. In my opinion we would trespassing into stdenv space then. I would prefer to do this with multiple derivations instead (#118452 (comment)).

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.

FRidh · 2021-04-05T07:08:34Z

requiredPythonModules = with python3.pkgs; computeRequiredPythonModules [ sphinx ];

This could be made unnecessary if Python's setup hook would check the file in nix-support and recurse in those folders.

nixos-discourse · 2021-05-04T15:15:17Z

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

stale · 2021-11-09T19:20:34Z

I marked this as stale due to inactivity. → More info

LunNova · 2022-02-07T04:41:18Z

What's still left to do here?

jonringer · 2022-02-07T04:44:35Z

it's already implemented for the most part. The major thing is to make it so that people stop using propagatedBuildInputs by default.

LunNova · 2022-02-07T16:09:20Z

I think this will make overrideAttrs not work for requiredPythonModules

6f845f7#diff-c263b18f23de7c1f00ed846783c3b977718aae8cdfdd89fc82adcf365d1d8bebR108-R163

If we overrideAttrs for requiredPythonModules, the old copy in requiredPythonModules_ will still be used.

If we wait for #119942 to go in we could refer to finalAttrs.requiredPythonModules instead.

yajo · 2023-06-29T10:46:52Z

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?

yajo · 2024-04-12T05:34:34Z

#271597 implemented this, which was stale already

FRidh requested review from jonringer, mmahut and teto as code owners November 3, 2020 11:05

FRidh added the 2.status: work-in-progress label Nov 3, 2020

FRidh changed the base branch from master to staging November 3, 2020 11:07

FRidh commented Nov 3, 2020

View reviewed changes

FRidh force-pushed the requiredPythonModules branch from ddcd386 to 9d43a9c Compare November 3, 2020 12:01

FRidh added 1.severity: mass-rebuild labels Nov 3, 2020

FRidh force-pushed the requiredPythonModules branch from ffe4603 to 6b5b552 Compare November 3, 2020 13:36

teto removed their request for review November 3, 2020 13:49

FRidh force-pushed the requiredPythonModules branch from 6b5b552 to 68c327d Compare November 3, 2020 13:56

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 3, 2020

FRidh force-pushed the requiredPythonModules branch from 68c327d to 619d93b Compare November 3, 2020 15:01

FRidh requested review from andir and zowoq as code owners November 3, 2020 16:21

ofborg bot added the 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. label Nov 3, 2020

FRidh mentioned this pull request Nov 5, 2020

Python-unstable "run" #102858

Closed

This was referenced Jan 13, 2021

Octave modules #108562

Merged

[RFC 0075] Declarative wrappers NixOS/rfcs#75

Draft

gebner mentioned this pull request Apr 3, 2021

Run time closure of duplicity includes mesa drivers #118410

Open

FRidh mentioned this pull request Apr 22, 2021

mkPythonDerivation: validate propagatedBuildInputs and buildInputs use a matching python #120220

Merged

10 tasks

FRidh mentioned this pull request Aug 5, 2021

NixOS 21.05: buildPythonApplication import conflict when two applications are built using different interpreter versions and same named dependencies #132761

Closed

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 9, 2021

jtojnar mentioned this pull request Dec 18, 2021

cambalache: init at 0.8.0 #138940

Merged

16 tasks

jtojnar mentioned this pull request Dec 30, 2021

soundconverter: init at 4.0.3 #152733

Merged

13 tasks

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Feb 7, 2022

FRidh mentioned this pull request Apr 27, 2022

Python: build wheels in separate derivation #170577

Open

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 12, 2022

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 29, 2023

Artturin mentioned this pull request Aug 31, 2023

python310Packages.pygobject: don't propagate cairo dev dependencies #252613

Merged

12 tasks

tobiasBora mentioned this pull request Nov 22, 2023

wrapPythonPrograms does not work on scripts #269198

Open

SomeoneSerge mentioned this pull request Dec 2, 2023

cudaPackages.setupCudaHook: propagate deps and the hook #271078

Merged

13 tasks

adisbladis mentioned this pull request Dec 2, 2023

python3.pkgs.buildPythonPackage: Separate runtime & build time dependencies #271597

Merged

13 tasks

SomeoneSerge mentioned this pull request Dec 25, 2023

opencv4.cxxdev: deps not propagated when cudaSupport=false #276691

Open

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024

yajo closed this Apr 12, 2024

Uh oh!

Conversation

FRidh commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

Things done

Tasks

Uh oh!

FRidh Nov 3, 2020

Choose a reason for hiding this comment

Uh oh!

FRidh Nov 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SuperSandro2000 commented Nov 4, 2020

Uh oh!

DavHau commented Nov 4, 2020

Uh oh!

jonringer commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FRidh commented Apr 4, 2021

Uh oh!

FRidh commented Apr 4, 2021

Uh oh!

FRidh commented Apr 5, 2021

Uh oh!

nixos-discourse commented May 4, 2021

Uh oh!

stale bot commented Nov 9, 2021

Uh oh!

LunNova commented Feb 7, 2022

Uh oh!

jonringer commented Feb 7, 2022

Uh oh!

LunNova commented Feb 7, 2022

Uh oh!

yajo commented Jun 29, 2023

Uh oh!

yajo commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

FRidh commented Nov 3, 2020 •

edited

Loading

jonringer commented Nov 5, 2020 •

edited

Loading

yajo commented Apr 12, 2024 •

edited

Loading