nixos: move default module location logic to eval-config.nix

[ncfavier]

This patch refactors nixosSystem in a few ways.

Semantically, it changes two things:

1. the lib argument that is passed to nixos/lib/eval-config.nix now defaults to final: this makes extensions of the lib (including nixosSystem itself) available in the modules by default.
2. system.nixos.versionSuffix and system.nixos.revision are now also set in the VM configs, not only in the top-level config.

I have tested both of these things.

Syntactically, it removes a bit of duplication.

The only thing that is now being done by this PR is to move the addModuleDeclarationFile bit into eval-config.nix as it doesn't only make sense for flake configs.

cc @edolstra @cole-h

[roberth]

Prevously, authors of the flake have decided to expend the extra effort not to add nixosSystem to the normal nixpkgs lib, but only to the flake lib. For this reason, it would seem that it was their intent not to add this function to lib anywhere else.

I would like to know why this decision was made. Without knowing why, I can't decide whether this change is good, or whether it should be simplified by just adding it to lib/default.nix, which may be more consistent and useful.

[ncfavier]

Quoting @jtojnar on Matrix:

also lib is supposed to be self-contained but nixosSystem depends on nixos/lib

[roberth]

The way I think of it, is that indeed nixpkgs/lib is supposed to be self-contained. It should be possible to use it by itself, such as with the nixpkgs/lib/flake.nix flake.

This is different from the (currently unofficial; NixOS/nix#4744) lib flake attribute, which, viewed top-down, represents the functions that are available inside any flake. For nixpkgs, this means exposing a NixOS function, as well as the functions from the nixpkgs/lib component.

That's my "ivory tower" interpretation. However, lib is already tightly coupled to the nixpkgs/pkgs component, providing some of the logic for nixpkgs/pkgs/stdenv for instance. The dependency is in the "correct" direction, but the coupling is definitely there.

More thoughts

Categorization of nixpkgs dependencies:

Undisputed:

pkgs -> lib
nixos -> lib

Apparently necessary:

nixos -> pkgs

flake -> lib
flake -> pkgs
flake -> nixos

Practical:

pkgs -> nixos (pkgs.nixosTests, pkgs.nixosTest, pkgs.nixos, pkgs.dockerTools.nixosImage (potentially, poc: #148456)

Dubious:

lib -> nixos

Disallowed:

lib -> * (apparently)
* -> flake

Even more thoughts

If we have lib.nixosSystem, why wouldn't we have lib.nixDarwinSystem?
Actually I'd really like for nix-darwin to be integrated into nixpkgs. We could have platform-independent modules. lib.nixosDarwinSystem anyone?

I'm not 100% opposed to a change like this, but I think the current solution is not optimal, because it reinforces the existence of two differenet lib-like objects that are usually interchangeable, rather than removing the artificial distinction between the two libs. A solution where nixosSystem becomes part of nixpkgs/lib would be a step in the right direction, but not a solution for the lib/flake.nix flake, which is inherently unsolvable.

So architecturally we're between a rock (nixpkgs-flake.lib) and a hard place (lib/flake.nix). Perhaps we should admit defeat and make a pragmatic choice by adding nixosSystem to pkgs.lib?

[ncfavier]

I'm fine with that. Should I create a lib/nixos.nix or something?

[edolstra]

Should I create a lib/nixos.nix or something?

I don't think we should put NixOS-specific stuff in nixpkgs/lib if it can be avoided. Tightly coupling nixpkgs/lib with NixOS would be a step in the wrong direction.

[ncfavier]

I don't think we should put NixOS-specific stuff in nixpkgs/lib if it can be avoided. Tightly coupling nixpkgs/lib with NixOS would be a step in the wrong direction.

Fine by me too, I'll give you the final say in this matter.

---

I would like to extend the scope of this PR a bit and move most of the logic of nixosSystem out of flake.nix.

nixosSystem is currently a wrapper around eval-config.nix made up of three layers:

1. add location information to the given modules using unsafeGetAttrPos. I think this can be moved into eval-config.nix, or even lib.evalModules, without problems; see this current discussion.
2. add a module that sets system.nixos.revisionSuffix and system.nixos.revision based on the flake's metadata. This obviously stays in the flake.
3. add vm and vmWithBootLoader configurations. I think this can be moved to eval-config.nix, which has the upside of also deduplicating the code in nixos/default.nix.

Let me know how this sounds, I'll push code shortly.

[roberth]

add location information to the given modules using unsafeGetAttrPos. I think this can be moved into eval-config.nix, or even lib.evalModules, without problems

evalModules reinvokes itself indirectly and this would cause the wrapping to duplicate. The wrapping should only be done once, so the natural way to do it is to shift the responsibility to the caller and giving them a helper function.
The alternative, to put a facade around evalModules doesn't seem like a good choice to me, because it seems that the only the caller knows which modules might have come from where.

It seems best to write a helper function lib.modules.setDefaultModuleLocation (or something) that can be used by eval-config.nix and such.

add vm and vmWithBootLoader configurations. I think this can be moved to eval-config.nix, which has the upside of also deduplicating the code in nixos/default.nix.

See #151082.

[ncfavier]

evalModules reinvokes itself indirectly and this would cause the wrapping to duplicate. The wrapping should only be done once, so the natural way to do it is to shift the responsibility to the caller and giving them a helper function. The alternative, to put a facade around evalModules doesn't seem like a good choice to me, because it seems that the only the caller knows which modules might have come from where.

It seems best to write a helper function lib.modules.setDefaultModuleLocation (or something) that can be used by eval-config.nix and such.

Sounds good.

add vm and vmWithBootLoader configurations. I think this can be moved to eval-config.nix, which has the upside of also deduplicating the code in nixos/default.nix.

See #151082.

Ah that's even nicer. I'll wait until that is merged and rebase.

[ncfavier]

With #151082 merged, this is now ready for review.

[ncfavier]

The manual fails to evaluate but this doesn't seem related to this PR (also fails on master)

[roberth]

Sadly I can't decide the future of lib on my own. The rest of the PR looks good and I think it'd be a good idea to merge the refactoring separately.

[roberth]

This line isn't uncontroversial.

I'm having trouble weighing the utility of "more stuff available" against the subjective "good architecture". That's because I can't assume that we continue to have two different libs prominently available in one repo.
I like "good architecture" too, but I think the die has been cast the moment nixosSystem was added to lib.
With that consideration, the solution is to add it to nixpkgs/lib, along with the experimental nixos attr above, and turn the whole thing into a nicely consistent user experience where lib is just lib.
So unless we have a real reason to keep the split, I think this change is vaguely in the right direction, but not complete or does not go to the root of the problem.

Hence, my question is, do we envisage a practical reason to keep nixpkgs/lib decoupled? This would be some feasible future action where the decoupling is actually necessary.
The only action I can think of is extracting lib into a separate repo. Although I question the utility of this, as pkgs, lib and nixos benefit a lot from being monorepos, I can see a solution for this, which is to extract the non-coupled parts - most of lib. Only a small layer that adds nixos and perhaps pkgs attrs would have to be separated out before extracting lib into a separate repo. We can even do this preemptively by making pkgs and nixos import a wrapper in a separate file, similar to what the flake does, but not just for the flake. Equivalently, we can move the non-coupled attrs into a separate overlay in lib, so that those parts can be accessed independently.

[ncfavier]

Regardless of how we decide to organise the lib, I don't see a reason not to pass the "most accurate lib" to eval-config.nix (in this case final). If nixosSystem was defined in nixpkgs/lib as you suggest, surely it would have something like { lib = lib; } // args, right? Otherwise we just force the user to add the lib argument themselves as soon as they want to use their own lib extensions in a module. I don't see how this is good architecture.

I think the decoupling debate is important and deserves its own issue/discussion/RFC if that's what it takes, but should be orthogonal to this PR.

[ncfavier]

Phrased differently, eval-config.nix has a safe default value for lib, but nixosSystem knows better what the default lib should be since it's in the process of defining it.

[roberth]

but nixosSystem knows better what lib should be

Whether that's actually better is a judgement that would depend on the outcome of the discussion.

Alternative explanations exists that make it not better: conflating flake.lib and nixpkgs/lib if they're considered separate concepts. They could be.

These requirements make my mind go in circles. It's awful and I need to stop.

I hope others can agree on what's actually good for users.

[ncfavier]

@cole-h would you happen to have an opinion about this?

[ncfavier]

I just noticed

        nixos = import ./nixos/lib { lib = final; };

and now I'm confused

[roberth]

Uh, yeah, I guess I remembered what you want when I wrote it and didn't think about it again since then.

Zooming out from the lib = final bit, that nixos attr is an experimental interface for testing and RFC 22-related refactoring. See #148456

[ncfavier]

Ok, this PR is just the refactoring now. I'll move the lib debate somewhere else

[edolstra]

system.nixos.versionSuffix and system.nixos.revision are now also set in the VM configs, not only in the top-level config.

What does this mean exactly? That any commit to Nixpkgs will trigger a rebuild of all VM tests? That would be very bad.

[roberth]

system.nixos.versionSuffix and system.nixos.revision are now also set in the VM configs, not only in the top-level config.

What does this mean exactly? That any commit to Nixpkgs will trigger a rebuild of all VM tests? That would be very bad.

It would, so they don't. Also they already would have because that code has been around for a while.

These defs are only used in nixos-rebuild build-vm. The VM tests don't use this code and they already contain one or two mkForces for variables that aren't nearly as problematic. Don't worry :)

[ncfavier]

Updated the original post. As @roberth says, that's not part of this PR anymore.

[ncfavier]

Opened #157056 about the lib issue

[roberth]

@test installer.simple

[ncfavier]

I think you mean
@ofborg test installer.simple