Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add minimal NixOS entrypoint #153211

Merged
merged 6 commits into from
Jan 20, 2022
Merged

Add minimal NixOS entrypoint #153211

merged 6 commits into from
Jan 20, 2022

Conversation

roberth
Copy link
Member

@roberth roberth commented Jan 2, 2022

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
  • 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:
  • 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.05 Release Notes (or backporting 21.11 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.

@github-actions 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 2, 2022
@roberth roberth requested review from zimbatm and dasJ January 2, 2022 13:50
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 2, 2022
Copy link
Member

@dasJ dasJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ];
  };
}

@@ -0,0 +1,46 @@
let
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about nixos/minimal-eval.nix?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anything that doesn't break the pattern of import ./lib pulling 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah if we have need to direct them, sure. I am not yet familiar with the more ambitious plans discussed before.


modules: A list of modules that constitute the configuration.

specialArgs: An attribute set of module arguments. Unlike
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config._module.args should 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?

@roberth roberth force-pushed the minimal-nixos branch 3 times, most recently from 1f5e2ba to aa1d542 Compare January 4, 2022 12:13
@roberth roberth requested review from infinisil and pennae January 4, 2022 12:13
@roberth roberth requested a review from nbp January 4, 2022 20:06
@roberth
Copy link
Member Author

roberth commented Jan 4, 2022

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 ];
  };
}

The new function is specifically for working with a minimal module list, which you don't get out of eval-config.nix. Avoiding the eval-config.nix file altogether lets us bypass some legacy and keep things actually simple. For instance, it has logic to tell the specialisations module what the module list is supposed to be, and it reads from the process environment. Some of these things are either unnecessary, conceptually unsound in the new context or just a bad idea.

@ofborg ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 7, 2022
@roberth roberth requested a review from Ericson2314 January 9, 2022 11:43
@roberth
Copy link
Member Author

roberth commented Jan 10, 2022

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.

Copy link
Contributor

@pennae pennae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test does not run because nixpkgs.nix includes meta attributes without loading the meta.nix module. looks pretty good otherwise :)

@roberth
Copy link
Member Author

roberth commented Jan 19, 2022

Will merge tomorrow, unless anyone has more feedback.

@roberth
Copy link
Member Author

roberth commented Jan 20, 2022

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.

@roberth roberth merged commit 98ae5a9 into NixOS:master Jan 20, 2022
@aanderse
Copy link
Member

but this is blocking other work

Looking forward to seeing it 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants