Conversation
dasJ
left a comment
There was a problem hiding this comment.
Some ideas.
I still don't really see the point of the new files (I do see the point in a minimal module list however!) when I could just do this instead:
{
eval = import <nixpkgs/lib/eval-config.nix> {
system = "";
modules = [ whatever ];
};
}
nixos/lib/default.nix
Outdated
There was a problem hiding this comment.
Are you sure this is the right place for evaluating NixOS? I'd have expected this to be in nixos/default.nix (but I know that place is already used). Maybe someone else has a good idea but this location makes it seem like importing that file gives me library functions around NixOS and not NixOS
There was a problem hiding this comment.
It is not great, I admit, and as you've noted that's mostly due to nixos/default.nix being taken. I've considered turning the existing nixos/default.nix into a __functor, but I've seen the command line tools behave weird around __functor and I don't want to touch such an essential file for an experimental feature.
I've also considered nixos/eval-config.nix. While familiarity is good, it is too easily confused with nixos/lib/eval-config.nix, which behaves rather differently, both wrt module list and a ton of legacy behavior we don't need.
There was a problem hiding this comment.
nixos/minimal-eval.nix does sound less confusing. or maybe nixos/lib/minimal-eval.nix? anything that doesn't break the pattern of import ./lib pulling in all lib functions, really.
There was a problem hiding this comment.
anything that doesn't break the pattern of
import ./libpulling in all lib functions
Actually, that's exactly what this is doing, when looking at the flake.nix change. Its lib.nixos is simply this file.
So then it should be lib/default.nix and the fact that the functions are about minimal evaluation is currently a coincidence.
There was a problem hiding this comment.
from the angle of what the flake exports you're totally right. but there's a lot of other stuff in nixos/lib (like that now-standard eval-config.nix) that isn't available from import ./nixos/lib. so i guess it's fine either way, and we retract the suggestion.
There was a problem hiding this comment.
Right, I now better understand your suggestion. Depending on whether those are intended for "public" consumption, the functions in those other files can later be exposed through nixos/lib/default.nix or moved to a nixos/common directory where exposure is not desirable.
There was a problem hiding this comment.
we have nixos/lib/eval-config.nix, so I would propose nixos/lib/eval-config-minimal.nix. Also nixos/lib/eval-config.nix should be using nixos/lib/eval-config-minimal.nix, so prove it is minimal and therefore the status quo cna be built on top of it.
There was a problem hiding this comment.
I've moved the implementation as @Ericson2314 suggested, kept nixos/lib/default.nix for the purpose of being lib.nixos in the flake and anyone who likes the lib style.
I'm undecided whether it only proves a point or whether it should be a requirement. If we find a reason to disconnect the two, proving a point shouldn't stop us from doing that. I can imagine that we find a useful change along the way, that we don't want to apply to conventional NixOS, yet I do hope the two can remain as similar as possible.
There was a problem hiding this comment.
Yeah if we have need to direct them, sure. I am not yet familiar with the more ambitious plans discussed before.
nixos/lib/default.nix
Outdated
There was a problem hiding this comment.
Maybe also add to the description that these are not meant for general use and should only be used when _module.args can't be used.
There was a problem hiding this comment.
Now it's
specialArgs: An attribute set of module arguments. Unlike
`config._module.args`, these are available for use in
`imports`.
`config._module.args` should be preferred when possible.
There was a problem hiding this comment.
config._module.argsshould be preferred when possible.
Doing more in from inside the modules is good I suppose and I think maybe it could be typed this way as well.
What would @infinisil, who I believe came up with this recommendation, tell the reader?
1f5e2ba to
aa1d542
Compare
The new function is specifically for working with a minimal module list, which you don't get out of |
|
While this pr is low risk, it is important code, so I won't merge it for another couple of days, so you can give more feedback. |
pennae
left a comment
There was a problem hiding this comment.
the test does not run because nixpkgs.nix includes meta attributes without loading the meta.nix module. looks pretty good otherwise :)
1c3adbc to
ca40dac
Compare
ca40dac to
3168017
Compare
|
Will merge tomorrow, unless anyone has more feedback. |
|
I don't like to self-merge, but this is blocking other work. It's marked as experimental, so we can change it after merging anyway. |
Looking forward to seeing it 😄 |
Motivation for this change
See #148456
In short: improve modularity, enabling new use cases, improving performance. This is just a first step; doing this in reviewable increments.
This adds a new entrypoint, similar to the PoC, but incorporating some feedback, making the entrypoint itself more minimal.
It also marks the new function as experimental, because using a minimal module list may be a little rough at first and I know it to be incomplete. However, in the spirit of minimalism, this is ok and things that can't be made optional can be added later if necessary.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes