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.

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

Draft
wants to merge 37 commits into
base: staging
Choose a base branch
from

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Nov 3, 2020

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

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 FRidh changed the base branch from master to staging November 3, 2020 11:07
@@ -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`).
Copy link
Member Author

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,
Copy link
Member Author

Choose a reason for hiding this comment

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

need fixing

pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
@SuperSandro2000
Copy link
Member

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
Copy link
Member

DavHau commented Nov 4, 2020

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.

@FRidh FRidh mentioned this pull request Nov 5, 2020
@jonringer
Copy link
Contributor

jonringer commented Nov 5, 2020

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
Copy link
Member Author

FRidh commented Apr 4, 2021

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
Copy link
Member Author

FRidh commented Apr 4, 2021

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

samueldr referenced this pull request Apr 4, 2021
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
Copy link
Member Author

FRidh commented Apr 5, 2021

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
Copy link

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
Copy link

stale bot commented Nov 9, 2021

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

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 9, 2021
@jtojnar jtojnar mentioned this pull request Dec 18, 2021
16 tasks
@jtojnar jtojnar mentioned this pull request Dec 30, 2021
13 tasks
@LunNova
Copy link
Member

LunNova commented Feb 7, 2022

What's still left to do here?

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

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

@LunNova
Copy link
Member

LunNova commented Feb 7, 2022

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.

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

yajo commented Jun 29, 2023

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?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 29, 2023
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants