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

pre-commit: prevent propagating inputs and polluting user's PYTHONPATH #235123

Merged

Conversation

MatrixManAtYrService
Copy link
Contributor

Description of changes

This prevents pre-commit's dependences from clobbering the user's dependencies via the PYTHONPATH variable in a nix-shell.

Fixes: #223275

Things done

Tested by changing which input is commented in this flake.nix:

{
  inputs.nixpkgs.url = "github:NixOS/nixpkgs/nixos-unstable";
  #inputs.nixpkgs.url = "path:/home/matt/src/nixpkgs"; # has these changes

  outputs = { self, nixpkgs }:

  let
    pkgs = nixpkgs.legacyPackages.x86_64-linux;
  in {
      packages = {
        x86_64-linux.default = pkgs.hello;
      };

      devShells.x86_64-linux.default = pkgs.mkShell {
        packages = [ 
          pkgs.python311
          pkgs.pre-commit
        ];
      };
    };
}

Before:

$ nix develop
$ echo $PYTHONPATH
/nix/store/02r58pwlr896fyas37kmv7dz7qy7vl9c-pre-commit-3.3.2/lib/python3.10/site-packages:/nix/store/cvvyizc66s7bpnp3v2b98r5kad6875vi-python3.10-cfgv-3.3.1/lib/python3.10/site-packages:/nix/store/pbrzc0798laj1vndgjs0i3byjm3cgzcf-python3.10-six-1.16.0/lib/python3.10/site-packages:/nix/store/95cxzy2hpizr23343b8bskl4yacf4b3l-python3-3.10.11/lib/python3.10/site-packages:/nix/store/hfki5gjbn3zln5a3s7r7ca4pv5xypvfv-python3.10-identify-2.5.24/lib/python3.10/site-packages:/nix/store/893hxxw019ddliij1vz8wdiz31kaldcn-python3.10-nodeenv-1.8.0/lib/python3.10/site-packages:/nix/store/a9nrh0jkfxcf4v1arm0v8cg491wpi6y9-python3.10-setuptools-67.4.0/lib/python3.10/site-packages:/nix/store/ni6vlicv2zmrqabrab12ibbb7mrs5nfr-python3.10-pyyaml-6.0/lib/python3.10/site-packages:/nix/store/s6bkxc4di4cdj6bdcpi0823b0p3zi3ap-python3.10-toml-0.10.2/lib/python3.10/site-packages:/nix/store/sph78jm83dajxl11gxrnvzqqh4kh1ka2-python3.10-virtualenv-20.19.0/lib/python3.10/site-packages:/nix/store/fv7kdisq5nsb0dkkr018g4l6z0kp0zg2-python3.10-distlib-0.3.6/lib/python3.10/site-packages:/nix/store/gxgsb27s8fp92ivy1rb1p6rc7ypqkihg-python3.10-filelock-3.9.0/lib/python3.10/site-packages:/nix/store/8qxwzcgyyrxyvj23l278zr7ylmbgf4r0-python3.10-platformdirs-3.0.0/lib/python3.10/site-packages

After:

$ nix develop
$ echo $PYTHONPATH
/nix/store/2nzwxmvb9n18iahgszzc3lfafg3rjzmz-python3-3.11.3/lib/python3.11/site-packages
  • Built on platform(s)
    • x86_64-linux
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Fixes a bug where pre-commit would pollute the PYTHONPATH with its
dependencies, potentially interfering with the user's dependencies.
@roberth
Copy link
Member

roberth commented Jun 1, 2023

This seems like a problem with the python hooks. In mkShell, packages is nativeBuildInputs, so that part should be fine afaict.
Adding strictDeps = true doesn't help.

@FRidh what do you think?

@FRidh
Copy link
Member

FRidh commented Jun 1, 2023

This issue has come up so unbelievably many times.

nix-shell is primarily for interactively working on a derivation. Hence setup hooks will be executed like they would in a regular nix-build. That means also Python's setup hook, which sets PYTHONPATH gets executed. And that causes problems when mixing multiple Python versions.

I don't want to go into this part entirely here, because it's complex, and possible solutions have been described, but it goes much wider than just Python.

This work-around, removing the propagation in case of applications, is acceptable and we do it more often. This is also why we have a separate buildPythonApplication (so in the future we could actually do this always) but that hasn't been implemented. It also isn't useful when people start using packages from the Python packages set (buildPythonPackage).

(IMO mkShell should have never been added to Nixpkgs. It advertises something it does not do. I typically wrap my applications in a buildEnv or symlink tree so hooks don't get executed.)

@FRidh FRidh merged commit 8ebcb96 into NixOS:master Jun 3, 2023
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 13, 2023
@NilsIrl NilsIrl mentioned this pull request Mar 30, 2024
13 tasks
truh added a commit to truh/nixpkgs that referenced this pull request Jul 10, 2024
Add setuptools to virtualenv dependency of pre-commit.
Adding pre-commit as a propagatedBuildInput of pre-commit
directly doesn't work since propagation is disabled for
pre-commit NixOS#235123

Python3.12 no longer ships with distutils. Setuptools
package continues to provide distutils.
https://docs.python.org/3/whatsnew/3.12.html
Many pre-commit jobs require distutils to function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pre-commit clobbers python311 PYTHONPATH entries with references to python310 packages
4 participants