-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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/grub: add new implementation of install-grub.pl
#317026
base: master
Are you sure you want to change the base?
Conversation
6327db4
to
eff29a0
Compare
Found a couple of (serious) bugs, fortunately there's a test I can use to catch them... :D |
339c8db
to
f8e1766
Compare
Ready for review |
Should probably post this in the discourse thread of prs ready to review :P |
@ofborg test grub grub-ng |
f8e1766
to
c0234b2
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4819 |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
It's what's blocking me from going perl-less. I'll give it a try when I have time, though my setup is not likely to trigger many edge cases.
Generally it seems like we should use eyre
a lot more: most code just uses ?
without adding any context. That'll lead to errors users can't understand 🙁
IMO, it's fine for a prototype to do that, but the best time for error handling is when you write the code, and the second best is when there's a full review. Otherwise you end up having to go over every thing again and it's very easy to miss some places.
I haven't done a full review but already spent enough time one this now. Here's the comments I wrote for now.
Don't hesitate to tell me I'm wrong or whatever in the comments :)
fs::create_dir_all(&grub)?; | ||
fs::set_permissions(&grub, fs::Permissions::from_mode(0o700))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any parent dirs will have unexpected permissions here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If create_dir_all
creates any parent directories, they won't use the permission we set just below.
I'm not sure what's the best thing to do here.
ac96ddc
to
9c7825b
Compare
37e7aea
to
98ee2b7
Compare
98ee2b7
to
d6bd08b
Compare
(I didn't read the PR but the os prober test passes, great!) |
I tested this and the Example entry from this PR:
Example entry from old script:
I have an impermanence setup. This is my partition setup for reference:
|
Might have missed a variable reference there when porting? (Note to self: never touch Perl ever again) |
So, you could say I'm one of the maintainers of grub: I use it on both EFI and non-EFI systems and I wrote the NixOS tests. As a first impression: can we try to keep it simpler? |
Most of the complexity is simply handling the error cases that the Perl code just skips over — more rigorous input parsing, more rigorous subprocess spawning, etc. Rust is in general a much more explicit language than Perl (which is infamous for being so terse that it's impenetrable). As to where to add the extra bootloader entries, it's aptly under src/builder/entries.rs :p |
Description of changes
Following in the footsteps of Perlless Activation (#270727) and Perlless
switch-to-configuration
(#308801), it's now time for Perllessinstall-grub
, which can be enabled by simply settingboot.loader.grub.useInstallNg
totrue
.This is still a draft PR for now since I don't use GRUB myself, and I could do with some help from GRUB users who could actually test this :) The GRUB config generation logic seems to work pretty well, though.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.