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: turn vmWithBootLoader into option (nixos-rebuild build-vm) #151082

Merged
merged 9 commits into from
Jan 14, 2022

Conversation

roberth
Copy link
Member

@roberth roberth commented Dec 17, 2021

Motivation for this change
  • Clean up by not repeating evalModules calls or vm definitions.
  • Allow users to affect the nixos-rebuild build-vm configuration directly, by setting configs in the new options.
  • Make the vm and vmWithBootloader attrs less special. These are now in regular config values.
  • Fix the type of system.build.

This is similar to #144094

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.

... which is like a specialisation, but for nixos-rebuild build-vm
@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 Dec 17, 2021
@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 Dec 17, 2021
@ncfavier
Copy link
Member

I think nixos-rebuild would have to be updated to handle build-vm* with flakes correctly.

nixos/default.nix Outdated Show resolved Hide resolved
nixos/lib/eval-config.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
We were exposing everything pointwise anyway.
If any new attrs are added, there's a good chance we'll want to
expose them anyway.
This will help anyone who imports the qemu module themselves, to
avoid a collision.
@roberth roberth requested a review from dasJ as a code owner December 17, 2021 13:40
@github-actions github-actions bot added 8.has: changelog 8.has: documentation This PR adds or changes documentation labels Dec 17, 2021
Legacy types.attrs has really bad merging behavior and does not
support priorities.

f build
@roberth roberth force-pushed the nixos-cleanup-vmWithBootLoader branch from bd6ea4b to e391690 Compare December 17, 2021 13:43
flake.nix Outdated Show resolved Hide resolved
nixos/default.nix Outdated Show resolved Hide resolved
@roberth roberth force-pushed the nixos-cleanup-vmWithBootLoader branch from e391690 to a271025 Compare December 17, 2021 13:49
Copy link
Member

@ncfavier ncfavier left a comment

Choose a reason for hiding this comment

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

Looks great now!

@roberth roberth added 0.kind: bug Something is broken 0.kind: enhancement Add something new labels Dec 17, 2021
@roberth roberth added 8.has: clean-up 8.has: module (new) This PR adds a module in `nixos/` labels Dec 17, 2021
@ncfavier
Copy link
Member

Maybe we can add a boot.isVM option, in the spirit of boot.isContainer?

@roberth
Copy link
Member Author

roberth commented Dec 17, 2021

Maybe we can add a boot.isVM option, in the spirit of boot.isContainer?

Not a fan of such options, particularly when they're intended to remove things after the fact. The current solution is already more in the spirit of #148456 so I would consider depending on it to be somewhat of a regression. That said, I do think a use case may arise, but I'm not eager to add this option until we know for sure that we need it.

@roberth
Copy link
Member Author

roberth commented Dec 17, 2021

Looks great now!

Thanks for the fast paced collaboration / review!

@ncfavier
Copy link
Member

Not a fan of such options, particularly when they're intended to remove things after the fact. The current solution is already more in the spirit of #148456 so I would consider depending on it to be somewhat of a regression. That said, I do think a use case may arise, but I'm not eager to add this option until we know for sure that we need it.

OK!

Looks great now!

Thanks for the fast paced collaboration / review!

NP!

@dasJ
Copy link
Member

dasJ commented Jan 5, 2022

Is this ready to get merged?

@ncfavier
Copy link
Member

ncfavier commented Jan 6, 2022

I think so.

@ncfavier
Copy link
Member

I've tested that nixos-rebuild build-vm and build-vm-with-bootloader work fine both with flakes and non-flakes. This is OK to merge.

@roberth roberth merged commit 2bf5958 into NixOS:master Jan 14, 2022
@ncfavier
Copy link
Member

ncfavier commented Jan 14, 2022

This conflicts with #149532 in some way (tested before/after) and causes the manual to fail to evaluate.

Cacheable portion of option doc build failed.
Usually this means that an option attribute that ends up in documentation (eg default or description) depends on the restricted module arguments config or pkgs.

@roberth
Copy link
Member Author

roberth commented Jan 14, 2022

Created #155022 but can't merge in the coming ~2h :( g2g

@Atemu
Copy link
Member

Atemu commented Jan 21, 2022

I greatly appreciate the change and am in favor of its spirit but the fallout needs to be reduced IMO.

This broke my VM detection and made my regular configs have VM settings. This could have devastating effects and implications.

builtins.hasAttr "vm" config.system.build

I'm not sure what other mechanisms there were but I'd prefer they stayed in-tact (with a deprecation notice) and only broke once the new mechanism has reached a stable release. Is that possible?

@roberth
Copy link
Member Author

roberth commented Jan 21, 2022

Perhaps the attributes could be renamed, so that they don't trigger such logic.

I'm not sure what other mechanisms there were

None that I know of and none documented here in the manual at least https://nixos.org/manual/nixos/stable/#sec-changing-config

@ncfavier
Copy link
Member

I am in favour of renaming the attributes, perhaps to something like virtualisation.build.vm* ?

@Atemu
Copy link
Member

Atemu commented Jan 21, 2022

This is also very non-trivial/near impossible to work around as if then else doesn't play well with the module system and an ugly mkMerge of mkIfs still fails because virtualisation.vmVariant (guarded behind a false mkIf statement!) doesn't exist in older Nixpkgs and the module system still complains.

I finally found a workaround (_module.check = false; and virtualisation.vmVariant = lib.mkIf ...) but that took quite a while to debug.

Atemu added a commit to Atemu/nixos-config that referenced this pull request Jan 21, 2022
NixOS/nixpkgs#151082

Should support stable, new unstable and older unstable.
@ElvishJerricco
Copy link
Contributor

ElvishJerricco commented Feb 6, 2022

Has this broken the virtualisation options in qemu-vm.nix? I can't set e.g. virtualisation.cores = 4; and use nix-build ./nixpkgs/nixos --arg configuration ./configuration.nix -A vm. I get error: The option `virtualisation.cores' does not exist.

EDIT: Oh I see, I had to add imports = [./nixos/modules/virtualisation/qemu-vm.nix];

@jackinloadup
Copy link

I'm running into the same issue as @ElvishJerricco but either I don't understand the proposed solution or it only works within nixpkgs. A related issue #59219 proposes a similar solution but is impure with <nixpkgs>. Is this import required by design? It seems like this file should already be imported at

vmVariant = extendModules {
modules = [ ./qemu-vm.nix ];
};
as part of nixos-rebuild build-vm.

I could be misunderstanding as I'm still a nix beginner. My goal is to test my nixosConfigurations locally while tweaking them so I don't need to be near the target machine to see changes to the boot process.

@roberth
Copy link
Member Author

roberth commented May 29, 2022

@jackinloadup I think ElvishJerricco's problem was unrelated to this PR.

nixos-rebuild build-vm should just work. If your configuration isn't quite compatible with the default vm variant of your configuration, you may have to add some overrides in the virtualisation.vmVariant option. It takes any NixOS option; for example: virtualisation.vmVariant.services.nginx.enable = lib.mkForce false;

If you know of a realistic option that needs to be overridden for nixos-rebuild build-vm, it'd be great to add it to the manual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug Something is broken 0.kind: enhancement Add something new 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: clean-up 8.has: documentation This PR adds or changes documentation 8.has: module (new) This PR adds a module in `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.

7 participants