nh icon indicating copy to clipboard operation
nh copied to clipboard

Add build-vm subcommand to `nh os build` to build a VM start script.

Open cdo256 opened this issue 1 year ago • 6 comments

This commit adds the boolean argument --vm/-v to build a virtual machine activation script instead of a full system. Note that this only applies to the os build subcommand, since it doesn't entirely make sense to switch to a VM, as I don't believe it's currently possible to switch between guest versions on the host or the guest directly without restarting the guest.

The patch https://github.com/viperML/nh/pull/74 was created in May 2024, which proposed to create a build-vm subcommand, but was never fully implemented. This is fully working, though I've only tested it on flakes on NixOS. There's two outstanding todo's:

  • Get it working on Darwin (I don't have hardware to test it currently, though).
  • Fix the logic with the commands to ideally only show the --vm option for the nh os build subcommand.

Please let me know if you'd be willing to merge a completed patch, and I can sort out those todo's. My feeling is after writing this, that it would make more sense to have a full subcommand like pr 74 suggested. That way it's neater to add VM specific options (such as adding a with --with-bootloader) to act like #config.system.build.vmWithBootloader.

build-image is also possible, but looks to be a little more work since it's handled a bit differently for flakes vs plain Nix. See https://github.com/NixOS/nixpkgs/blob/481c7ead0aafb3d6dfa863ae87e3fdd6badc0b28/pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.sh#L832

Just let me know if you'd merge this patch if I finish it off, and whether you'd prefer a build-vm sub-command or --vm option on the build command. I'm happy to do either, but nh os build-vm would be my vote.

cdo256 avatar Jan 09 '25 15:01 cdo256

Yeah, as you probably saw, the types don't allow to have custom flags for each subcommand. You add make more spaguetti types like so, but I'm not sure I'm a fan:

pub enum OsSubcommand {
    Switch(OsRebuildArgs),
    Build(OsBuildArgs),
    // ...
}

struct OsBuildArgs {
    #[command(flatten)]
    common: OsRebuildArgs,

    #[arg(long)
    vm: bool
}

I'm happy to do either, but nh os build-vm would be my vote.

Indeed

viperML avatar Jan 10 '25 18:01 viperML

  • Get it working on Darwin (I don't have hardware to test it currently, though).

I have the hardware to test if needed, but there is also a github action that you can use to test.

Eveeifyeve avatar Jan 10 '25 23:01 Eveeifyeve

Yeah, as you probably saw, the types don't allow to have custom flags for each subcommand. You add make more spaguetti types like so, but I'm not sure I'm a fan:

...

I'm happy to do either, but nh os build-vm would be my vote.

Indeed

Thanks confirming, I'll switch the subcommand over to build-vm get a new patch pushed up tonight.

cdo256 avatar Jan 12 '25 13:01 cdo256

I might add some nix ci testing to test all the functions.

Eveeifyeve avatar Jan 13 '25 07:01 Eveeifyeve

I've changed it to be the build-vm subcommand. I've tested on my different system configurations and it works the same as nixos-rebuild build-vm and nixos-rebuild build-vm-with-bootloader.

On researching NixOS images on Darwin, I found Nix documentaiton that states that the process of building a NixOS VM is a bit different in Darwin. I'm not familiar at all with MacOS or Darwin, so I would have to leave that part to someone else. I've backed out my changes so it's NixOS Linux VMs being built on Linux only.

There are four commits off master in my build-vm branch

The first commit "Add build-vm subcommand to nh os." is the main one with build-vm implemented, and --with-bootloader option.

The second commit disables nvd if the system hostname is different from the target hostname. Assuming that you only want to diff the versions if you are building for your local machine, rather than for another machine, or a VM. This might not be the desired behaviour, so anyone with push access can feel free to skip this change.

cdo256 avatar Jan 13 '25 17:01 cdo256

I've been busy these days, so I'll take a deeper look and merge ASAP

viperML avatar Feb 03 '25 10:02 viperML

@cdo256 are you still interested in finishing this PR? I've left some comments, after which this could be merged. There are some merge conflicts that arose, but I don't think they are that unmanagable. If you don't plan to finish it, and if you don't mind me taking over, I can try to complete this to help with our #254 goals

NotAShelf avatar May 04 '25 23:05 NotAShelf

@NotAShelf Hi, sorry for the delay. Thanks for the feedback. I'm happy to finish this, I'll see what I can do tonight.

cdo256 avatar May 08 '25 22:05 cdo256

Okay I've think I've covered all of your points. Let me know if there's anything else you think should be changed. I've squashed all of the commits into a single commit, since I'm assuming the intention is for both of them to be merged. (adding build-vm as well as checking if the target and system hostnames match before running nvd.) I can split them back up if prefered.

Edit: The build has failed with some warnings, but they're in json.rs so it doesn't look like my code.

cdo256 avatar May 09 '25 00:05 cdo256

The build "fail" appears to be from formatting (which I should fix, I think) and it can be resolved easily. If you could run sh fix.sh or invoke cargo fmt manually, and then push your changes again, it should go away. I'll provide a review shortly.

NotAShelf avatar May 09 '25 00:05 NotAShelf

Okay that should have fixed everything, including the most recent merge conflicts. LMK if there's anything else.

cdo256 avatar May 10 '25 14:05 cdo256

I couldn't find anything to nit, good work. Went ahead and merged it anyway, if there is anything missing we'll probably catch it in testing (i.e. in production)

NotAShelf avatar May 10 '25 14:05 NotAShelf