python313Packages.simplekv: fix inputs #388393
Conversation
|
|
| @@ -26,8 +26,9 @@ buildPythonPackage rec { | |||
|
|
|||
| build-system = [ setuptools ]; | |||
|
|
|||
| dependencies = [ | |||
| dulwich | |||
| dependencies = [ dulwich ]; | |||
There was a problem hiding this comment.
I hope I'm not nitpicking; simplekv's tox.ini lists more dependencies, that when missing causes their tests to be skipped.
Is this an appropriate time to add these dependencies (as optional-dependencies?)?
If so, I would recommend adding pytest-xdist to the nativeCheckInputs, as the tests already take quite long (5 - 6 minutes per test suite per system) to run.
There was a problem hiding this comment.
dulwich is no more important here than, say, redis is. Preferring one over the other because your client requires it is frankly being lazy.
I have spelled my opinions about this out here, but will repeat a bit:
simplekv is doing the "python thing" and saying that everything is optional because no one subset is required for any particular user's requirements. I don't want dulwich as I don't require it, but @fabaff shouldn't need redis if they don't require it either.
I think that if we could do grouped dependencies in optional-dependencies, everything should be in optional-dependencies. Since we cannot, I think it makes sense to leave that completely empty, remove pytestCheckHook from dependencies (as is done here) and rely on the consumer to know how this library works (this is a common pattern in python code and I think it makes sense to map it this way).
However, I recognize the incorrectness of this and would also be fine moving everything into optional-dependencies. It's more work for little gain, but it is the right thing to do in the Nix space.
We should not be adding dulwich like this though. It is wrong from both nix's point of view and from python's.
wegank
added
the
12.approvals: 1
| @@ -26,8 +26,9 @@ buildPythonPackage rec { | |||
|
|
|||
| build-system = [ setuptools ]; | |||
|
|
|||
| dependencies = [ | |||
| dulwich | |||
| dependencies = [ dulwich ]; | |||
There was a problem hiding this comment.
dulwich is no more important here than, say, redis is. Preferring one over the other because your client requires it is frankly being lazy.
I have spelled my opinions about this out here, but will repeat a bit:
simplekv is doing the "python thing" and saying that everything is optional because no one subset is required for any particular user's requirements. I don't want dulwich as I don't require it, but @fabaff shouldn't need redis if they don't require it either.
I think that if we could do grouped dependencies in optional-dependencies, everything should be in optional-dependencies. Since we cannot, I think it makes sense to leave that completely empty, remove pytestCheckHook from dependencies (as is done here) and rely on the consumer to know how this library works (this is a common pattern in python code and I think it makes sense to map it this way).
However, I recognize the incorrectness of this and would also be fine moving everything into optional-dependencies. It's more work for little gain, but it is the right thing to do in the Nix space.
We should not be adding dulwich like this though. It is wrong from both nix's point of view and from python's.
|
I misunderstood |
Related #382050
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.