Add support for buildPythonPackage

[rmcgibbo]

Hi @jtojnar:

I've started trying to add support for checking the arguments to buildPythonPackage as an addition to the current checking of the arguments to mkDerivation. So far, it appears to mostly work, although I can't yet seem to get it to report multiple failures at once -- the checks appear to short-circuit once a single failing check is identified.

Still WIP, but I wanted to post this now for any feedback.

[SuperSandro2000]

This will never be a complete list and is a deeper issue in nix. I don't want to start something with levenshtein because a good reviewer should catch that.

[jtojnar]

Adding a levehnstein checker is on my to do list since typos are common and tooling should catch them. Example from today https://discourse.nixos.org/t/error-building-python-package-no-matching-distribution-found-could-not-find-a-version-that-satisfies-the-requirement-six/11143

And adding synonyms is also a good idea IMHO (maybe we could even try to include it into levehnstein formula).

[SuperSandro2000]

I have a few ideas if you are interested:

• check for duplications in propagatedInputs with checkInputs.
• check for impure python imports which pull in another python version

[rmcgibbo]

I think I'm actually hitting this bug/issue, and I'm not sure how to solve it yet: NixOS/nixpkgs#44426

[jtojnar]

Try NixOS/nixpkgs#107044 (comment)?

[rmcgibbo]

Thanks for the hint. Let me bang my head against that for a bit.

[rmcgibbo]

This PR should be working now. I've force pushed and separated the commits into individual units.

I'm new to this codebase, and fairly new to nix in general, so any feedback would be appreciated. I'm sure I'm messing some things up.

Here's an example running with c64f442 and the new checks:

# hacked nixpkgs/pkgs/development/python-modules/aiostream/default.nix, with lots of bugs injected
{ lib
, buildPythonPackage
, numpy
}:

buildPythonPackage rec {
  pname = "aiostream";
    
  pythonImportCheck = [ "aiostream" ];  # typo
  propagatedBuildInputs = [ numpy ];
  checkInputs = [ numpy ];  # duplicate
  checkPhase = ''
    pytest
  '';  # unnecessary

  meta = with lib; {
    license = licenses.gpl3;  # deprecated
  };
}

Producing the output

$ ./tools/nixpkgs-hammer -f ~/projects/nixpkgs python38Packages.aiostream
When evaluating attribute ‘python38Packages.aiostream’:
warning: duplicate-check-inputs
Dependencies listed in propagatedBuildInputs shouldn't be repeated in checkInputs.

Near /home/mcgibbon/projects/nixpkgs/pkgs/development/python-modules/aiostream/default.nix:11:3:
   |
11 |   checkInputs = [ numpy ];  # duplicate
   |   ^
Near /home/mcgibbon/projects/nixpkgs/pkgs/development/python-modules/aiostream/default.nix:10:3:
   |
10 |   propagatedBuildInputs = [ numpy ];
   |   ^
See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/duplicate-check-inputs.md
warning: python-explicit-check-phase
Consider using pytestCheckHook in checkInputs rather than checkPhase="pytest".

Near /home/mcgibbon/projects/nixpkgs/pkgs/development/python-modules/aiostream/default.nix:12:3:
   |
12 |   checkPhase = ''
   |   ^
See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/python-explicit-check-phase.md
warning: python-imports-check-typo
A typo in pythonImportsCheck was detected.

Near /home/mcgibbon/projects/nixpkgs/pkgs/development/python-modules/aiostream/default.nix:9:3:
  |
9 |   pythonImportCheck = [ "aiostream" ];  # typo
  |   ^
See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/python-imports-check-typo.md
warning: unclear-gpl
`gpl3` is a deprecated license, check if project uses `gpl3Plus` or `gpl3Only` and change `meta.license` accordingly.

Near /home/mcgibbon/projects/nixpkgs/pkgs/development/python-modules/aiostream/default.nix:17:5:
   |
17 |     license = licenses.gpl3;
   |     ^
See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/unclear-gpl.md

[rmcgibbo]

@SuperSandro2000 wrote:

check for impure python imports which pull in another python version

I've implemented this in aa085cb, but the changes required are a little bigger to the code base, because it requires our checkDerivation functions knowing which interpreter version they're being called on. Making this consistent with the mkDerivation checks requires also changing those checks to get passed the relevant stdenv, I think.

So this commit will require some code review, I think. I'd be happy to move it to a separate PR, if you prefer.

[jtojnar]

Looks good at first glance, will give it a thorough review in the afternoon (CEST). Could you also add the test file to the tests directory, listing it in tests/default.nix and run-tests.py?

[SuperSandro2000]

So this commit will require some code review, I think. I'd be happy to move it to a separate PR, if you prefer.

Can't decide that because it is jtojnar projects.

[rmcgibbo]

@jtojnar: I've added tests.

[jtojnar]

It might be nice to name the offending packages. Though most of the time, they would probably be listed included using python*Packages in the file so maybe it is unnecessary.

[jtojnar]

Do you feel like writing a short blurb for each check to add to explanations/ directory? Otherwise, I can do that in the next week.

[rmcgibbo]

I believe that I've fixed all of the more cosmetic changes in the last review, but I have not (yet) addressed the more substantive ones.

[rmcgibbo]

Rebase finished.

[rmcgibbo]

It would be better to generate this list automatically.

[rmcgibbo]

Replacing this expression with pythonPackageSetNames = lib.attrNames prev.pythonInterpreters; -- which seems like it absolutely should work(!) instead gives an infinite recursion error.

[jtojnar]

Turns out I made a mistake when writing the composeOverlays function. I corrected it and then I found out I have written the exact same code that is contained in lib.composeExtensions (up to alpha equivalence).

The following should work:

diff --git a/lib/default.nix b/lib/default.nix
index 28abae1..97d2255 100644
--- a/lib/default.nix
+++ b/lib/default.nix
@@ -25,6 +25,9 @@ rec {
       };
     };
 
+  # Identity element for overlays.
+  idOverlay = final: prev: {};
+
   # Creates a function based on the original one, that, when called on
   # one of the requested packages, runs a check on the arguments passed to it
   # and then returns the original result enriched with the reports.
@@ -101,16 +104,14 @@ rec {
 
     in
     lib.genAttrs pythonPackageSetNames (pythonName:
-      let
-        prvPython = builtins.getAttr pythonName prev;
-      in prvPython.override {
-        packageOverrides = python-self: python-super: {
-          buildPythonPackage = wrapFunctionWithChecks prvPython.pkgs.buildPythonPackage namePositions check;
-        };
-      });
+      prev.${pythonName}.override (oldOverrides: {
+        packageOverrides = lib.composeExtensions (oldOverrides.packageOverrides or idOverlay) (final: prev: {
+          buildPythonPackage = wrapFunctionWithChecks prev.buildPythonPackage namePositions check;
+        });
+      }));
 
     checkFor = check: let
       o1 = (checkMkDerivationFor check);
       o2 = (checkBuildPythonPackageFor check);
-    in attrs: final: prev: (o1 attrs final prev) // (o2 attrs final prev);
+    in attrs: lib.composeExtensions (o1 attrs) (o2 attrs);
 }

[rmcgibbo]

@jtojnar: Nice!. I've pulled that diff into the first commit in this PR (now d3d30b2).

[rmcgibbo]

Actually -- I need to bisect my changes a little bit. Something I've done has caused every error message do be repeated twice. I suspect that diff might be the problem...

[rmcgibbo]

(The duplication is has been fixed by slapping in a call to lib.unique, based on @jtojnar's suggestion.)

[rmcgibbo]

I think this is ready to go, pending writing explanations for each error message. Is there anything else that's a blocker for you, @jtojnar?

[jtojnar]

Looks good to me otherwise. Great job.

Unfortunately, I do not think I will be able to write the explanations this week after all. We can just add link = false; to the new checks for now.

[rmcgibbo]

I added some explanations in fa77e76. Maybe you could take a look at them, @SuperSandro2000?

[SuperSandro2000]

Suggested change

	Not all python packages include tests. However it should be possible, at a minimum, to "smoke test" every python package my simply checking that the modules it declares can be `import`ed.
	Not all python packages include tests. However it should be possible, at a minimum, to "smoke test" every python package by simply checking that the modules it declares can be `import`ed.

This should probably added to all python packages regardless of tests.

[rmcgibbo]

Right now, the logic in the check only requires the package to have either a checkPhase or a pythonImportsCheck. Do you think it should be changed to require a pythonImportsCheck unconditionally?

[SuperSandro2000]

I am not completely sure but it does not hurt.

[jtojnar]

I have fixed some minor style issues, squashed the explanations into the introducing commits, added some links. I have also changed the wording of python-imports-check-typo.md back since I am not sure it makes sense to require pythonImportsCheck when regular tests are present – they should already check imports work. I have also changed pytestFlagsArray example since pytestFlagsArray = [ "-o cache_dir=$(mktemp -d)" ]; is wrong on three counts.

[jtojnar]

Thank you for this great job.