nixos/top-level: attrs -> attrsOf for system.build

[danbst]

Fixes #62856

But to make it work I've chosen to extract system.build.fileSystems into separate option. Let me know if making with types; lazyAttrsOf unspecified is more favorable.

EDIT: this is now changed to attrsOf unspecified, due to manual present in system.build.

[danbst]

cc @infinisil @volth

[danbst]

thanks ofBorg, indeed I had disabled manual, so I didn't notice this issue. But I don't know where to put manual, so I give up to unspecified type.

[infinisil]

Does attrsOf unspecified not work? Because lazyAttrsOf has the problem that conditional definitions aren't fully supported, meaning it should only be used where mkIf false definitions aren't meaningful (e.g. for _module.args in #70138), or if those definitions are handled separately (as I'm doing in #75584)

[danbst]

@infinisil interesting, indeed attrsOf works. But then first patch is justified. If I remove first patch (about filesystems split) and change type to attrsOf unspecified, I get infinite recursion.

[danbst]

@GrahamcOfBorg eval

[danbst]

@infinisil so attrsOf unspecified doesn't work. It turns out, whenever config.system.build.manual.manualHTMLIndex is called, module system doesn't know that system.build.manual.manualHTMLIndex is really system.build subattr option.

Maybe it's possible to fix modules.nix somehow, but for now lazyAttrsOf unspecified is a real workaround.

much thanks @GrahamcOfBorg for catching this.

[infinisil]

Wait what was the issue exactly? How can I reproduce?

[danbst]

@infinisil just change to attrsOf unspecified and run

nix-instantiate nixos/release-combined.nix -A tested

[infinisil]

I tracked down the infinite recursion with attrsOf. Just this config.nix reproduces it with nix-instantiate nixos --arg configuration ./config.nix -A system

{
  boot.loader.grub.device = "nodev";
  fileSystems."/".device = "test";

  services.nixosManual.showManual = true;
}

The recursion is coming from:

1. system.build.toplevel being evaluated as the entrypoint
2. This result of which depends on all warnings, which are therefore evaluated too
3. One of which is this one:

   nixpkgs/nixos/modules/system/boot/systemd.nix

   Lines 808 to 810 in b92d2ce

   	warnings = concatLists (mapAttrsToList (name: service:
   	optional (service.serviceConfig.Type or "" == "oneshot" && service.serviceConfig.Restart or "no" != "no")
   	"Service ‘${name}.service’ with ‘Type=oneshot’ must have ‘Restart=no’") cfg.services);

   which is causing all systemd.services.*.serviceConfig.Type's to be evaluated
4. The type of serviceConfig is the strict attrsOf:

   nixpkgs/nixos/modules/system/boot/systemd-unit-options.nix

   Lines 245 to 251 in 69efb5c

   	serviceConfig = mkOption {
   	default = {};
   	example =
   	{ StartLimitInterval = 10;
   	RestartSec = 5;
   	};
   	type = types.addCheck (types.attrsOf unitOption) checkService;

   , meaning evaluating Type also evaluates all other attributes next to it
5. One of which is this one:

   nixpkgs/nixos/modules/services/misc/nixos-manual.nix

   Lines 60 to 61 in 02371d9

   	serviceConfig = {
   	ExecStart = "${cfg.browser} ${config.system.build.manual.manualHTMLIndex}";

6. But this depends on system.build.manual.manualHTMLIndex, and because system.build uses the strict attrsOf too, this in turn causes all of system.build.* to be evaluated
7. One of which is our original system.build.toplevel, and there's the infinite recursion

So, there's multiple ways of solving this, each breaking this loop at a point:

• (break at 1.) Moving toplevel out of system.build.* into system.toplevel. This however would be problematic because third-party code depends on this being there (even though the option is marked internal)
• (break at 3.) Implementing assertions and warnings in the module system directly, such that that serviceConfig assertion can be done within the submodule. I've thought about this for some time, concluding that it's pretty hard to get right, if possible at all
• (break at 4.) Changing serviceConfig to use lazyAttrsOf instead. This seems to be a very good solution, as it's very reasonable to assume that systemd is the only place where it gets processed, and because of this one could set the unitOption's emptyValue to something that gets later ignored by the systemd file builders (similar to what I'm doing in Configuration file formats for JSON, INI, YAML and TOML #75584)
• (break at 5.) Not making ExecStart depend on system.build by e.g. installing that index in /run/current-system and using that path instead there. While this would work, it doesn't sound like the right solution here
• (break at 6.) By making system.build be a lazyAttrsOf to break the recursion, which is the solution in this PR. I'm not a fan of this because I don't think there's a reasonable way to rescue mkIf definitions, which I'd think are probably indeed used for it (e.g. to check whether system.build.isoImage is available).

So I think the break at 4. is the most preferable here, so I suggest implementing that over the current break at 6.

[stale]

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.