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

nixos: Add system.build.{toplevel,installBootLoader}, improve error message #156503

Merged
merged 8 commits into from
Jan 25, 2022

Conversation

roberth
Copy link
Member

@roberth roberth commented Jan 24, 2022

Motivation for this change
  • Undocumented options are bad, even more so when they're important ones like toplevel
  • Bad error message

This is the successor of #155883

Things done:

  • add temporary helper function
  • add types.unique { message } type
  • make system.build.toplevel non-internal
  • use types.unique to improve error message:

Original error:

building the system configuration...
error: The option `system.build.installBootLoader' is defined multiple times.
       Definition values:
       - In `nixos/modules/system/boot/loader/grub/grub.nix': <derivation /nix/store/v1yj7wffg4q1mqk8rahsq5pgvq1jagn8-install-grub.sh.drv>
       - In `nixos/modules/system/boot/loader/systemd-boot/systemd-boot.nix': <derivation /nix/store/kzmd23ma820f3kxx6sylnkc2yfa348cm-systemd-boot.drv>

New error:

nix-repl> (nixos { fileSystems."/".device = "x"; boot.loader.grub.devices = ["/dev/x"]; boot.loader.systemd-boot.enable = true; boot.loader.grub.enable = true; }).config.system.build.installBootLoader
error: The option `system.build.installBootLoader' is defined multiple times.
       Only one bootloader can be enabled at a time. This requirement has not
       been checked until NixOS 22.05. Earlier versions defaulted to the last
       definition. Change your configuration to enable only one bootloader.

       Definition values:
       - In `nixos/modules/system/boot/loader/grub/grub.nix': <derivation /nix/store/v1yj7wffg4q1mqk8rahsq5pgvq1jagn8-install-grub.sh.drv>
       - In `nixos/modules/system/boot/loader/systemd-boot/systemd-boot.nix': <derivation /nix/store/kzmd23ma820f3kxx6sylnkc2yfa348cm-systemd-boot.drv>
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.

Modules that do not depend on e.g. toplevel should not have to include it just to set
things in `system.build`. As a general rule, this keeps tests simple, usage flexible
and evaluation fast. While one module is insignificant, consistency and good practices
are.
This allows the values below it to be specified as options, while
remaining compatible with existing code.
@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 24, 2022
@roberth roberth force-pushed the nixos-add-system.build-options branch from 3f96a14 to 4c37667 Compare January 24, 2022 12:49
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 24, 2022
nixos/modules/system/activation/top-level.nix Show resolved Hide resolved
nixos/modules/system/activation/top-level.nix Show resolved Hide resolved
lib/options.nix Show resolved Hide resolved
lib/options.nix Outdated Show resolved Hide resolved
lib/types.nix Show resolved Hide resolved
Couldn't extend types.uniq and it had a silly name anyway.
Now we can have better error messages.
@ncfavier
Copy link
Member

ncfavier commented Jan 24, 2022

@ofborg test grub systemd-boot.basic

This improves the error message when the configuration contains
more than one boot loader.
I don't really approve of this solution, but documenting its
purpose was the least I could do for now.
@roberth roberth force-pushed the nixos-add-system.build-options branch from f023b86 to 4800f30 Compare January 24, 2022 15:17
@roberth roberth force-pushed the nixos-add-system.build-options branch from ff16e93 to 48dbe26 Compare January 24, 2022 15:32
@roberth roberth merged commit 8919495 into NixOS:master Jan 25, 2022
@roberth roberth mentioned this pull request Jan 25, 2022
13 tasks
@lopsided98
Copy link
Contributor

Previously it was only possible to override an attr in system.build using mkAfter:

system.build = mkAfter {
  attr = ...;
};

This now fails with lib/types.nix:569:38 called with unexpected argument 'priority'. The correct way is now to use mkForce like any other option.

I don't think this breakage needs to be fixed, since the new config is definitely better than the old mkAfter hack. I'm just posting this to help anyone else who runs into this cryptic error message.

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: documentation 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.

3 participants