Python3Packages.simplekv: Improperly adds pytestCheckHook to dependencies

[bbenne10]

Nixpkgs version

• Stable (24.11)

Describe the bug

simplekv uses dependencies to add buildInputs to itself. These end up in propagatedBuildInputs, which causes everything downstream of this package to end up running pytestCheckHook, even if doCheck is false. You can avoid this by setting dontUsePytestCheck, but that is poorly documented and will be necessary in all consuming packages, which is onerous for consumers.

Steps to reproduce

use python3.buildPythonPackage to bundle any python library (or python3.buildPythonApplication for any python application) and add simplekv to propagatedBuildInputs and set doCheck to false (I cannot offer a reproduction repository at this time, I am sorry). You will notice that pytestCheckPhase is run in the dependent package.

Expected behaviour

I would expect test behavior to be isolated to the package requesting them

Screenshots

No response

Relevant log output

Additional context

No response

System metadata

• system: "x86_64-linux"
• host os: Linux 5.14.0-503.22.1.el9_5.x86_64, Red Hat Enterprise Linux, 9.5 (Plow), nobuild
• multi-user?: yes
• sandbox: yes
• version: nix-env (Lix, like Nix) 2.91.1 System type: x86_64-linux Additional system types: i686-linux, x86_64-v1-linux, x86_64-v2-linux, x86_64-v3-linux Features: gc, signed-caches System configuration file: /etc/nix/nix.conf User configuration files: /home/bbennett37/.config/nix/nix.conf:/etc/xdg/nix/nix.conf Store directory: /nix/store State directory: /nix/var/nix Data directory: /nix/store/lknx1arna0x37dg61ixsjbdlwcy9dvld-lix-2.91.1/share
• nixpkgs: /nix/store/56m82shl5xdjl0s54licn6davvpj12as-source

Notify maintainers

@fabaff

Note for maintainers: Please tag this issue in your pull request description. (i.e. Resolves #ISSUE.)

I assert that this issue is relevant for Nixpkgs

• I assert that this is a bug and not a support request.
• I assert that this is not a duplicate of an existing issue.
• I assert that I have read the NixOS Code of Conduct and agree to abide by it.

Is this issue important to you?

Add a 👍 reaction to issues you find important.

[bbenne10]

I may be able to offer a small refactor of the simplekv package, moving things to using propagatedBuildInputs and checkInputs - but that might be delayed until Monday for personal reasons.

[eclairevoyant]

seems that #303895 should be mostly reverted

[bbenne10]

I think it is a bit more subtle than that. The usage of dependencies is fine for everything EXCEPT pytestCheckHook. I dont really care for dependencies as a attr name here, as it is different from other derivations and confuses some subtle distinctions between dependency kind as shown off in this derivation, but it does work to simplify the packaging of new stuff for new contributors, so I'm not 100% sure what to take away from the whole thing. I think we should simply move pytestCheckHook to nativeCheckInputs and leave the rest of the referenced commit as is.

[eclairevoyant]

They were originally all part of nativeCheckInputs, implying that the package itself didn't need them, i.e. none of them should be in dependencies. For example, mock is also a testing framework, and should only be used in the checkPhases.

[bbenne10]

I didn't believe that could've been right, but checking simplekv's github: it has no direct dependencies declared in setup.py. This is surprising to me, but not wrong, so yeah - we should probably just revert that change (and re-nixfmt the file).

[eclairevoyant]

Seems at least dulwich is required at runtime: https://github.com/mbr/simplekv/blob/48657ce36f1a85c6716a2cca4adbcb67919ef1b6/simplekv/git.py#L5-L6

And according to https://github.com/mbr/simplekv/blob/master/docs/changes.rst six is no longer required (but I do see one test using it: https://github.com/mbr/simplekv/blob/48657ce36f1a85c6716a2cca4adbcb67919ef1b6/tests/test_hmac.py#L9)

So I guess, dulwich stays in dependencies, the rest back to *checkInputs.

[bbenne10]

Well, this isn't the full picture.

simplekv has a number of backends. Each is largely independent of the rest, but shares a common interface. The reason simplekv doesn't advertise exact dependencies is that all of the dependencies are optional and all of them depend on the consumer's usage. For instance, we only use the JsonStore (which has no dependencies) and the RedisStore (which depends on the redis library). I don't particularly want gae or dulwich installed when I am only using those two stores. The totally correct answer (as I understand it) here is to use optional-dependencies to emulate the dependency groups from the python world. To extend this into simplekv, we'd need to define dependency groups for each backend.

In other words: the thing simplekv is doing is python-y. I don't love it, but I understand why they're doing it. To emulate it, we'd need something more complicated, but I'm not sure it is immediately worth the effort to do this 100% correctly. I think everything should just go into nativeCheckInputs for now.

[eclairevoyant]

Okay, if dulwich is optional, then sure let's put it in passthru.optional-dependencies as well as *checkInputs.