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/virtualbox: Adds more options to virtualbox-image.nix #42699

Merged
merged 2 commits into from Jul 11, 2018
Merged

nixos/virtualbox: Adds more options to virtualbox-image.nix #42699

merged 2 commits into from Jul 11, 2018

Conversation

dalaing
Copy link
Contributor

@dalaing dalaing commented Jun 28, 2018

Previously you could only set the size of the disk.

This change adds the ability to change the amount of memory
that the image gets, along with the name / derivation name /
file name for the VM.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

Previously you could only set the size of the disk.

This change adds the ability to change the amount of memory
that the image gets, along with the name / derivation name /
file name for the VM.
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

I don't see the point of these *Name options, why would they be useful?

'';
};
vmDerivationName = mkOption {
type = types.string;
Copy link
Member

Choose a reason for hiding this comment

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

These should all be types.str instead

@dalaing
Copy link
Contributor Author

dalaing commented Jun 30, 2018

I'm trying to build some custom NixOS VMs for a workshop using this, and it's really handy to be able to change those names so that I can have Hydra produce an OVA with a particular appliance name and filename.

Otherwise I have to create the OVA, then import it, change the name, and then export it again, which seems like a waste of time relative to just creating it with the name I want in the first place.

@infinisil
Copy link
Member

Okay, that's vmName, but what about the other two?

@dalaing
Copy link
Contributor Author

dalaing commented Jun 30, 2018

I also wanted to be able to have a particular filename for the output OVA without having to monkey around with things in a second step, and at that point I also figured I should probably be naming these derivations differently in the nix store.

If I didn't have a need / couldn't see a need to change the derivation name or filename, then I would have left them out :)

If folks don't want that kind of flexibility in this module then that's fine - I can just maintain my own copy of this file. I just thought that if I wanted it that someone else might end up wanting it.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

I'm always a bit worried about cost vs. benefit of adding options. The cost is that a NixOS system takes longer to evaluate, and every single option contributes to that (I think it's about linear in the amount of options). The benefit is how many people actually end up using these options and finding them useful.

That said, I think these option are alright, I would be complaining more if it were over 1000 options.

@dalaing
Copy link
Contributor Author

dalaing commented Jul 3, 2018

Thanks for that feedback - it'll really help guide me in the future when I'm trying to decide between making a PR and maintaining something that maybe only a few people need in my local repos. It's great that moving between those is not a big drama in Nix :)

@xeji xeji merged commit 4d5371f into NixOS:master Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants