services.nix-daemon: refactor into mkRemoteBuilderDesc

[teto]

I like to specify my builders on the go (for various reasons, there are many factors that decide where I build), in my shell.
what I do is I export in my shell
RUNNER1="xxxx"
RUNNER2="yyyyy"
then I can run any nix command and append a --option buiilders "$RUNNER1" (any combination of the $RUNNER).

which are generated from "mkRemoteBuilderDesc" that I've copy/pasted in my flake.
I would like to expose mkRemoteBuilderDesc somehow but not sure how to
do so. In some file in lib/ ?

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.11 Release Notes (or backporting 22.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[roberth]

If you only use the function, you lose the name and type checking of the module system. Effectively you're trying an implementation of RFC 78.

To implement it without losing functionality and without duplicating code, you could

1. Put the builder submodule in a separate file "remote-builder.nix"
2. Move the mkRemoteBuilderDesc logic into remote-builder.nix as an internal option rendered = mkOption { internal = true; type = types.str; ..... }. (remove the machine param in favor of the config module argument)
3. Add a function somewhere renderRemoteBuilder = machine: (lib.evalModules { modules = [ ./path/to/remote-builder.nix machine ]; }).config.rendered. Could be nixos/lib/default.nix or nix.passthru, or somewhere else.
4. Write a unit test and add it to nixos/tests/all-tests.nix
5. Document it in the NixOS manual

(1) and (2) can already be implemented without much decision making and enable you to reuse the function without copy-paste. (3) can be put in nixos/lib/default.nix as an experimental feature. We have feature flags there. (4) and (5) are trivial.

Fwiw, renderRemoteBuilder isn't NixOS-specific, so it should go into a nix directory instead. Related discussion: https://github.com/nixpkgs-architecture/issues/issues/5#issuecomment-1228656788

[roberth]

Something like these suggestions should restore the option docs and add a bit of context to errors.

[teto]

@roberth I am almost there, I just can't apply the ".rendered" on a valid machine description, e.g., nix-build lib/tests/nix-daemon.nix --show-trace fails with

       error: attribute 'sshUser' missing

       at /home/teto/nixpkgs3/nixos/modules/services/misc/remote-builder.nix:11:28:

           10|     (concatStringsSep " " ([
           11|         "${optionalString (machine.sshUser != null) "${machine.sshUser}@"}${machine.hostName}"
             |                            ^
           12|         (if machine.system != null then machine.system else if machine.systems != [ ] then concatStringsSep "," machine.systems else "-")

[roberth]

Suggested change

	type = lib.types.submodule (import ../../nixos/modules/services/misc/remote-builder.nix (args // { isNixAtLeastPre24 = true; }));
	type = lib.types.submodule {
	imports = [ ../../nixos/modules/services/misc/remote-builder.nix ];
	nixVersion = config.nix.package.version;
	};

This way you don't have to call the module, and the module system can pass its own config parameter to the submodule, so that the submodule works.

I think it'd be nicer to a value into a nixVersion option than isNixAtLeastPre24 because the latter probably won't be sufficient in the future.