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

grub bootloader: add forceInstall option #19379

Merged
merged 1 commit into from
Nov 21, 2016
Merged

grub bootloader: add forceInstall option #19379

merged 1 commit into from
Nov 21, 2016

Conversation

nixy
Copy link
Contributor

@nixy nixy commented Oct 9, 2016

Motivation for this change

Currently NixOS can't be installed on a partitionless disk, because grub-install will fail. GRUB recommends against installing on a paritionless disk because it can be unstable, but it will allow the installation if the '--force' option is used.

Even though the partitionless installation isn't recommended it makes sense in some situations. In particular Linode uses partitionless disks for most of its tools. When using GRUB on Linode's platform the bootloader is run on the host machine instead of the VM and the only thing that is used is the grub.cfg file. This is the main motivation for the change as I am attempting to make NixOS work better on Linode's platform

In general I think more options for NixOS is better, even if they may be potentially dangerous. As long as the user is fully informed how dangerous these options are.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

Using the --force option on GRUB isn't recommended, but there are very
specific instances where it makes sense. One example is installing on a
partitionless disk.

@mention-bot
Copy link

@nixy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @wkennington, @edolstra and @obadz to be potential reviewers.

@lucabrunox
Copy link
Contributor

Looks good to me, but I fear any change with perl and grub :P cc @edolstra

@Mic92
Copy link
Member

Mic92 commented Oct 23, 2016

It tried running the vm tests, which installs grub, but they failed on my machine. But I am sure it is not due grub.

@lucabrunox
Copy link
Contributor

lucabrunox commented Oct 23, 2016

@Mic92 I believe that. The problem with grub is that it cannot be rolled back. So we must be 100% sure it works.

@Mic92
Copy link
Member

Mic92 commented Oct 23, 2016

of course.

@nixy
Copy link
Contributor Author

nixy commented Oct 25, 2016

These changes shouldn't affect anything grub at all unless the new NixOS option is used.

That said, I agree these should be tested carefully. It is something that is important to get right and I am not terribly familiar with perl. I have successfully used these changes to install NixOS multiple times on a Linode with no issue.

But, I believe this could create broken installs by its nature. If grub is installed forcibly even when problems are detected then it would be possible to get into a situation where you have a broken bootloader.

I think the key here is to make sure that the changes in the PR only affect users who intentionally enable the forceInstall option.

Using the --force option on GRUB isn't recommended, but there are very
specific instances where it makes sense. One example is installing on a
partitionless disk.
@nixy
Copy link
Contributor Author

nixy commented Nov 16, 2016

I would like to do whatever needs to be done to get this pulled in, since other things I would like to do to contribute are blocking on this.

I can see that the travis checks are "failing" but there don't appear to be any actual issues? It looks like one of the checks tried to compile all of nixpkgs, or something like that, and the log got too long and auto terminated.

Can someone clarify if there is anything wrong here or if there is anything I need to do to get this integrated?

@Mic92 Mic92 merged commit cb8af0c into NixOS:master Nov 21, 2016
@Mic92
Copy link
Member

Mic92 commented Nov 21, 2016

Thanks for your contribution! Tested on my machine and with the VM tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants