nixos/fishnet: init by OlivierMarty · Pull Request #209181 · NixOS/nixpkgs

OlivierMarty · 2023-01-05T15:18:37Z

Description of changes

Adds a module to run fishnet, a distributed Stockfish analysis for lichess.org.

Things done

dmytrokyrychuk · 2023-10-28T12:33:26Z

maintainers/maintainer-list.nix

This must be done in a separate commit, as per CONTRIBUTING.md

When adding yourself as maintainer, in the same pull request, make a separate commit with the message maintainers: add <handle>. Add the commit before those making changes to the package or module

Thank you! I fixed the PR.

OlivierMarty · 2025-09-16T21:12:38Z

I rebased this PR. It's ready again to be merged, if someone can approve.

Thanks.

maevii

Wow this is an old PR, I'm surprised it's gotten so little attention. Few minor things, otherwise it looks good. I wasn't able to actually test the module since I don't have a key (and don't want to apply for one), but the documented options & config format match upstream's.

New modules should also be added to the release notes.

maevii · 2025-10-07T10:57:19Z

nixos/modules/services/misc/fishnet.nix

+    mkKeyValue = k: v: if v == null then "" else lib.generators.mkKeyValueDefault { } "=" k v;
+  };
+in
+with lib;


with lib; shouldn't be used, #208242

Done.

Thanks for the review!

maevii · 2025-10-07T11:01:52Z

nixos/modules/services/misc/fishnet.nix

+        let
+          stateDirectory = "fishnet";
+        in


I feel like this should be customizable, including the /var/lib prefix

It's necessary in /var/lib if we want to leverage systemd managing the directory: https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html#RuntimeDirectory=

I know some modules handle paths in or out of /var/lib/ differently, but this seems overkill here.
I can make the base name customizable though.

What do you think?

You're right, grepping through the repo most of the modules seem to also hardcode their state dirs. Some do have options for changing it, but it's probably not necessary here.

I'd still probably hardcode it directly instead of using the let though, since it's pretty much guaranteed not to change and most other modules also just set it directly.

I kept the variable to not repeat myself.

maevii · 2025-10-07T11:14:03Z

nixos/modules/services/misc/fishnet.nix

+      type = types.submodule {
+        freeformType = format.type;
+
+        options.Fishnet = {


Running fishnet configure seems to output all-lowercase options, if the config is case insensitive I'd probably document everything either in all lowercase (to match upstream) or in camelCase (to match most other NixOS options).

Done with all in lowercase.

maevii

Few more things I missed the first time around, sorry. Other than that everything LGTM.

maevii · 2025-11-13T16:54:19Z

nixos/modules/services/misc/fishnet.nix

+    };
+
+    assertions = lib.singleton {
+      assertion = (cfg.settings.fishnet.key == null) != (cfg.keyfile == null);


What about the case where neither are set? Assertions/warnings should also be defined at the top of config rather than the bottom.

Suggested change

assertion = (cfg.settings.fishnet.key == null) != (cfg.keyfile == null);

assertion = cfg.settings.fishnet.key != null || cfg.keyfile != null;

I moved the assertion.

I believe it was correct though, I test that exactly one of key or keyfile is not null. My version is an exclusive or (!= for booleans is XOR) while yours is a regular or which would allow both options to be set.

You're absolutely right, sorry! Somehow I forgot for a second that assertions work the opposite from warnings (true != true -> false, which is correct here but wouldn't be in a warning), then remembered that when I was making the suggestion, but also forgot both options can't be set at the same time.

Sorry again, thanks for calling me out. Think I need more sleep...

maevii · 2025-11-13T17:07:56Z

nixos/modules/services/misc/fishnet.nix

+
+  format = pkgs.formats.ini {
+    # Skip null values.
+    mkKeyValue = k: v: if v == null then "" else lib.generators.mkKeyValueDefault { } "=" k v;


Suggested change

mkKeyValue = k: v: if v == null then "" else lib.generators.mkKeyValueDefault { } "=" k v;

mkKeyValue = k: v: lib.optionalString (v != null) (lib.generators.mkKeyValueDefault { } "=" k v);

Very much a nitpick, but since you use optionalString in ExecStart I think this would be better for consistency

maevii

Alright, everything looks good to me now.

Requesting @SuperSandro2000 for review/merge, since this PR is almost 3 years old and would probably slip under the radar otherwise.

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 5, 2023

OlivierMarty force-pushed the fishnet branch 2 times, most recently from 98f7072 to 91ac193 Compare January 5, 2023 15:26

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 5, 2023

OlivierMarty force-pushed the fishnet branch from 91ac193 to 9e092b1 Compare January 5, 2023 15:46

h7x4 added the 8.has: module (new) This PR adds a module in `nixos/` label Oct 23, 2023

dmytrokyrychuk suggested changes Oct 28, 2023

View reviewed changes

OlivierMarty force-pushed the fishnet branch from 9e092b1 to f8f6bf6 Compare February 12, 2024 20:54

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 12, 2024

OlivierMarty force-pushed the fishnet branch 2 times, most recently from 1036578 to b4d86cf Compare February 12, 2024 22:56

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 12, 2024

dmytrokyrychuk approved these changes Feb 13, 2024

View reviewed changes

wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Mar 9, 2024

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 27, 2024

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 1, 2025

nixpkgs-ci bot added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 25, 2025

OlivierMarty force-pushed the fishnet branch from b4d86cf to 64202c2 Compare September 16, 2025 19:35

OlivierMarty force-pushed the fishnet branch 2 times, most recently from 1404a76 to 7acfc15 Compare September 16, 2025 20:47

maevii reviewed Oct 7, 2025

View reviewed changes

maintainers: add oliviermarty

149f036

OlivierMarty force-pushed the fishnet branch from 7acfc15 to 975d693 Compare November 12, 2025 19:34

OlivierMarty force-pushed the fishnet branch from 975d693 to e55f2ae Compare November 12, 2025 19:39

nixpkgs-ci bot added 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation labels Nov 12, 2025

OlivierMarty force-pushed the fishnet branch from e55f2ae to 9fbbea4 Compare November 12, 2025 21:33

maevii suggested changes Nov 13, 2025

View reviewed changes

nixos/fishnet: init

280d143

OlivierMarty force-pushed the fishnet branch from 9fbbea4 to 280d143 Compare November 13, 2025 21:47

maevii approved these changes Nov 14, 2025

View reviewed changes

maevii requested a review from SuperSandro2000 November 14, 2025 02:41

nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Nov 14, 2025

nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 2, 2025

	assertion = (cfg.settings.fishnet.key == null) != (cfg.keyfile == null);
	assertion = cfg.settings.fishnet.key != null \|\| cfg.keyfile != null;

	mkKeyValue = k: v: if v == null then "" else lib.generators.mkKeyValueDefault { } "=" k v;
	mkKeyValue = k: v: lib.optionalString (v != null) (lib.generators.mkKeyValueDefault { } "=" k v);

Uh oh!

Conversation

OlivierMarty commented Jan 5, 2023

Description of changes

Things done

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlivierMarty commented Sep 16, 2025

Uh oh!

maevii left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maevii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maevii left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maevii left a comment •

edited

Loading