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

Jump to bottom

nixos/nixos-build-vms: use pkgs.qemu for virtualisation #101473

Merged
merged 1 commit into from Oct 24, 2020
Merged

nixos/nixos-build-vms: use pkgs.qemu for virtualisation #101473

merged 1 commit into from Oct 24, 2020

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Oct 23, 2020

Motivation for this change

When I test a change e.g. in the module system manually, I usually use
nixos-build-vms(8) which also gives me a QEMU window where I can play
around in the freshly built VM.

It seems as this has changed recently when the default package for
non-interactive VM tests using the same framework was switched to
pkgs.qemu_test to reduce the closure size. While this is a reasonable
decision for our CI tests, I think that you really want a QEMU window of
the VM by default when using nixos-build-vms(8).

[1] bc2188b

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Ma27
nixos/nixos-build-vms: use pkgs.qemu for virtualisation
aab534b 
When I test a change e.g. in the module system manually, I usually use
`nixos-build-vms(8)` which also gives me a QEMU window where I can play
around in the freshly built VM.

It seems as this has changed recently when the default package for
non-interactive VM tests using the same framework was switched to
`pkgs.qemu_test` to reduce the closure size. While this is a reasonable
decision for our CI tests, I think that you really want a QEMU window of
the VM by default when using `nixos-build-vms(8)`.

[1] bc2188b
@Ma27 Ma27 requested review from andir and rnhmjoj October 23, 2020 15:42
andir
andir approved these changes Oct 23, 2020
View reviewed changes
Copy link
Member

@andir andir left a comment

Choose a reason for hiding this comment

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

Looks reasonable. Sorry for breaking your workflow :)

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Oct 23, 2020

I'm not too familiar with this tool. Is nixos-build-vms purpose to build testing machines or any network of NixOS VMs (in production)? If it's the latter, then perhaps it shouldn't be using nixos/lib/testing-python.nix.

In any case, I think it would be better if testing-python.nix would take an interactive option, to make more obvious what's going on.

@Ma27
Copy link
Member Author

Ma27 commented Oct 23, 2020

It's intended to be a testing tool as it only starts a QEMU user session just like the NixOS testing framework.

In any case, I think it would be better if testing-python.nix would take an interactive option, to make more obvious what's going on.

Not sure if having another option doing (usually) the exact same thing is an actual benefit. Improving the option's description may be better.

@andir
Copy link
Member

andir commented Oct 23, 2020

It's intended to be a testing tool as it only starts a QEMU user session just like the NixOS testing framework.

In any case, I think it would be better if testing-python.nix would take an interactive option, to make more obvious what's going on.

Not sure if having another option doing (usually) the exact same thing is an actual benefit. Improving the option's description may be better.

Actually, the change I did in #49403 adds the driverInteractive attribute that will use the full-blown QEMU for local test execution (including sdl, audio, gtk, …).

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Oct 23, 2020

Ah, cool. So, shouldn't this be enough?

--- a/nixos/modules/installer/tools/nixos-build-vms/build-vms.nix
+++ b/nixos/modules/installer/tools/nixos-build-vms/build-vms.nix
@@ -15,4 +15,4 @@ with import ../../../../lib/testing-python.nix {
   pkgs = import ../../../../.. { inherit system config; };
 };
 
-(makeTest { inherit nodes; testScript = ""; }).driver
+(makeTest { inherit nodes; testScript = ""; }).driverInteractive

I'm not sure how I to test this...

@Ma27
Copy link
Member Author

Ma27 commented Oct 23, 2020

@rnhmjoj with a command like this:

$ cat foo.nix
{ vm = {}; }
$ nixos-build-vms -I nixpkgs=`pwd` foo.nix

@andir not sure if I'm missing something, but using driverInteractive doesn't open a QEMU window.

@andir
Copy link
Member

andir commented Oct 23, 2020

@rnhmjoj with a command like this:

$ cat foo.nix
{ vm = {}; }
$ nixos-build-vms -I nixpkgs=`pwd` foo.nix

@andir not sure if I'm missing something, but using driverInteractive doesn't open a QEMU window.

Oh yeah I see the issue now. I somehow did expect that the driver script was the only place that required the variable qemu parameter. I'll have to pass it down the module system eval as well. I'll get to work on fixing that right away.

@Ma27
Copy link
Member Author

Ma27 commented Oct 23, 2020

@andir I can take care of it as well here :) Not sure if I can get to it tonight though.

@andir
Copy link
Member

andir commented Oct 23, 2020 •
edited

@andir I can take care of it as well here :) Not sure if I can get to it tonight though.

I think you'll need the change proposed in this PR anyway. The entire test runner script has to be a bit refactored. Currently we are specifying the qemu package on waaaaay too many places. I'd like the module system to be the only place where it is being defined and then we just stick with that.

@andir
Copy link
Member

andir commented Oct 23, 2020

I think I've a workable solution: https://github.com/andir/nixpkgs/tree/nixos-build-vms-qemu

I didn't want to push it to this branch (just yet) as this change here can go in as is. I'd like to get some feedback on my additional proposed changes if things then work as expected with your use-case (and with -A driverInteractive)

@Ma27
Copy link
Member Author

Ma27 commented Oct 24, 2020

@andir fair enough. Does that mean I should merge the PR for now? You can revert the change and switch to driverInteractive as soon as your refactoring is completed, right?

@andir
Copy link
Member

andir commented Oct 24, 2020

Yes, just merge this. This is good from my point of view.

@Ma27 Ma27 merged commit 48612c7 into NixOS:master Oct 24, 2020
@Ma27 Ma27 deleted the nixos-build-vms-qemu branch October 24, 2020 12:43
@Ma27
Copy link
Member Author

Ma27 commented Oct 24, 2020

Done. Thanks a lot for your review and your work on this! :)

@andir andir mentioned this pull request Oct 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andir andir andir approved these changes

@rnhmjoj rnhmjoj Awaiting requested review from rnhmjoj

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Ma27 @rnhmjoj @andir