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.

[bbenne10]

I'm sorry - I'd put this off to reply after work one way and then completely forgot to do so.

I think the thing I want is grouped dependencies for each backend, which passthru.optional-dependencies does not offer. I think exposing the dependencies there is a bit confusing, since you can do the exact same thing with just python3.withPackages (ps: [ ps.simplekv ps.dulwich ]) and this seems closer to what a python developer would expect. In fact, it is even more verbose in the passthru.optional-dependencies approach (python3.withPackages (ps: [ ps.simplekv ps.simplekv.optional-dependencies.dulwich ]), which I suppose does clarify why you want it (i.e. for which package it exists in your inputs) but does nothing to say which backend or part of that python library it is for (and it gets worse as you add more dependencies for a backend, as there's no clarity that they're unified).

These critiques are soft ones, but it seems redundant. If I get around to fixing this soon, I'll do what feels most natural as I get to it.

[fabaff]

In an ideal world upstream would handle their optional requirements. As `simplekv is not really maintained or under active development one would best make a PR with the changes in the upstream repo and we pull that patch.

I added dulwich because that was what my consumer of simplekv needed as far as I remember.

[bbenne10]

What does it mean for the upstream to handle their optional requirements? We can't pull in a python project's optional dependency groups reasonably, can we? Without that ability, we have to do the mapping manually.

If we do it at all, we should do it manually and fully. If we don't, we shouldn't do it partially. Your PR does it partially (and does not address a generalize-able usecase, imo).

[bbenne10]

I was incorrect: We can declare grouped dependencies by setting optional-dependencies to an attrSet (a pattern I had not encountered before, but very useful). I have a opened a PR for this which also adds me as a co-maintainer of this package.