nixos/grub: add new implementation of install-grub.pl
#317026
Conversation
github-actions
bot
added
6.topic: nixos
pluiedev
force-pushed
the
init/install-grub-ng
branch
5 times, most recently
from
6327db4
to
eff29a0
Compare
ofborg
bot
added
8.has: package (new)
|
Found a couple of (serious) bugs, fortunately there's a test I can use to catch them... :D |
pluiedev
force-pushed
the
init/install-grub-ng
branch
2 times, most recently
from
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 |
pluiedev
force-pushed
the
init/install-grub-ng
branch
from
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.
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 :)
| rustPlatform.buildRustPackage { | ||
| pname = "install-grub"; | ||
| version = "0.1.0"; |
There was a problem hiding this comment.
I generally like to do this without duplicating the info:
| rustPlatform.buildRustPackage { | |
| pname = "install-grub"; | |
| version = "0.1.0"; | |
| let | |
| inherit (lib.importTOML ./Cargo.toml) package; | |
| in | |
| rustPlatform.buildRustPackage { | |
| pname = package.name; | |
| inherit (package) version; |
|
|
||
| use std::{ | ||
| collections::HashSet, | ||
| fmt::Write as _, |
There was a problem hiding this comment.
style nit: Is this really needed? I think just using the trait without naming it is enough to not trigger linter errors?
There was a problem hiding this comment.
This is required for write(ln)! unfortunately as otherwise it introduces a name conflict with std::io::Write
| } | ||
| } | ||
|
|
||
| let usernames = self.config.users.0.keys().copied().collect::<Vec<_>>(); |
There was a problem hiding this comment.
style nit: IMO Turbofish is best left alone in its natural habitat (this seems more common as a pattern to me):
| let usernames = self.config.users.0.keys().copied().collect::<Vec<_>>(); | |
| let usernames: Vec<_> = self.config.users.0.keys().copied().collect(); |
| } | ||
|
|
||
| if let Some(store) = &self.grub_store { | ||
| writeln!(&mut self.inner, r"{}", store.search)?; |
There was a problem hiding this comment.
style nit:
| writeln!(&mut self.inner, r"{}", store.search)?; | |
| writeln!(&mut self.inner, "{}", store.search)?; |
| function savedefault {{ | ||
| if [ -z "${{boot_once}}"]; then | ||
| saved_entry="${{chosen}}" | ||
| save_env saved_entry | ||
| fi | ||
| }} |
There was a problem hiding this comment.
style nit:
| function savedefault {{ | |
| if [ -z "${{boot_once}}"]; then | |
| saved_entry="${{chosen}}" | |
| save_env saved_entry | |
| fi | |
| }} | |
| function savedefault {{ | |
| if [ -z "${{boot_once}}"]; then | |
| saved_entry="${{chosen}}" | |
| save_env saved_entry | |
| fi | |
| }} |
There was a problem hiding this comment.
Having a builder.rs file and builder/ dir seems weird. Maybe move this to builder/mod.rs?
There was a problem hiding this comment.
I prefer new-style (mod.rs-less) module trees
See https://doc.rust-lang.org/edition-guide/rust-2018/path-changes.html
| fs::create_dir_all(&grub)?; | ||
| fs::set_permissions(&grub, fs::Permissions::from_mode(0o700))?; |
There was a problem hiding this comment.
Any parent dirs will have unexpected permissions here.
| grub_store: Option<Grub>, | ||
|
|
||
| default_config: &'conf Path, | ||
| pub copied: HashSet<PathBuf>, |
There was a problem hiding this comment.
Might've missed something, but I don't think this needs to be pub.
| default_config: &'conf Path, | ||
| pub copied: HashSet<PathBuf>, | ||
|
|
||
| dry_run: bool, |
There was a problem hiding this comment.
It seems error prone to have !dry_run every where. Could be nice to have a wrapper for each fs function we use that checks the flag automatically.
A TODO is fine by me if you like the idea:
| dry_run: bool, | |
| // TODO: create wrappers for fs functions we use that check `$DRY_RUN` automatically | |
| dry_run: bool, |
| .append_initrd_secrets(name, path, current)? | ||
| .unwrap_or_default(); | ||
|
|
||
| // FIXME: $confName |
There was a problem hiding this comment.
What does this mean? Maybe put more details in the comment.
There was a problem hiding this comment.
This is from the original script, and I have no idea what it means either
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.useInstallNgtotrue.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 = relaxedsandbox = truenix-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.