-
Notifications
You must be signed in to change notification settings - Fork 11
installer: atomically copy directory to specified ESP #1
Conversation
({ options, lib, ... }: {
config = lib.mkIf (!(options.boot.loader ? manual)) {
boot.loader.systemd-boot.enable = true;
boot.loader.efi.canTouchEfiVariables = true;
};
}) |
installer/src/main.rs
Outdated
console_mode: pico | ||
.value_from_str("--console-mode") | ||
.unwrap_or_else(|_| String::from("keep")), | ||
console_mode: pico.value_from_str("--console-mode")?, | ||
configuration_limit: pico.value_from_str("--configuration-limit")?, |
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.
This should be optional.
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.
corresponding change to your flake example:
${lib.optionalString (config.boot.loader.systemd-boot.configurationLimit != null) ''--configuration-limit="${toString config.boot.loader.systemd-boot.configurationLimit}"''} \
|
Opa! I'm switched! |
Also, there should probably be some better error handling -- would be nice if there was some way to differentiate between fatal and not-so-fatal errors. |
This copies the files to a temporary directory, then renames them to their desired destination, and finally removes the temporary directory.
Also implements the configuration_limit and editor flags.
We don't need to error if the directory we're trying to clean doesn't exist (it will be created). We do need to error if `nix-env` returns empty output -- `nix- env` requires root to list the generations of a profile (for whatever reason...).
It's a bit annoying having to run this as root even for a dry run (because `nix- env` needs root to create a lockfile to list generations... for some reason). So, we just do the work ourselves.
This function isn't only run against the ESP, so we don't want to give that impression in the function signature.
4995743
to
3b4b29a
Compare
installer/src/systemd_boot/mod.rs
Outdated
pub(crate) fn bootloader_is_old( | ||
bootloader_version: Option<usize>, | ||
systemd_version: usize, | ||
) -> Result<bool> { | ||
if let Some(bootloader_version) = bootloader_version { | ||
let old = systemd_version > bootloader_version; | ||
Ok(old) | ||
} else { | ||
String::from("/nix/var/nix/profiles/system") | ||
warn!("could not find any previously installed systemd-boot"); | ||
Ok(false) | ||
} | ||
} | ||
|
||
pub(crate) fn get_bootloader_version(bootctl: &Path, esp: &Path) -> Result<Option<usize>> { | ||
trace!("checking bootloader version"); | ||
|
||
debug!("running `bootctl status`"); | ||
let output = Command::new(&bootctl) | ||
.args(&[&format!("--path={}", &esp.display()), "status"]) | ||
.output()? | ||
.stdout; | ||
|
||
let version = self::parse_bootloader_version(&output)?; | ||
|
||
Ok(version) | ||
} | ||
|
||
pub(crate) fn parse_bootloader_version(output: &[u8]) -> Result<Option<usize>> { | ||
let output = str::from_utf8(output)?; | ||
|
||
// pat in its own str so that `cargo fmt` doesn't choke... | ||
let pat = "^\\W+File:.*/EFI/(BOOT|systemd)/.*\\.efi \\(systemd-boot (?P<version>\\d+)\\)$"; | ||
|
||
// See enumerate_binaries() in systemd bootctl.c for code which generates this: | ||
// https://github.com/systemd/systemd/blob/788733428d019791ab9d780b4778a472794b3748/src/boot/bootctl.c#L221-L224 | ||
let re = RegexBuilder::new(pat) | ||
.multi_line(true) | ||
.case_insensitive(true) | ||
.build()?; | ||
let caps = re.captures(output); | ||
|
||
let version = if let Some(caps) = caps { | ||
caps.name("version") | ||
.and_then(|cap| cap.as_str().parse::<usize>().ok()) | ||
} else { | ||
None | ||
}; |
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.
Consider making more things. For example, we're not just comparing usize
s we're comparing installed SystemdBoot versions.
What if there was a struct of SystemdBootVersion
with a version
field, and you could construct it by calling SystemdBootVersion::detect_version()
which executes the command and calls SystemdBootVersion::from_status_output(string)
which calls SystemdBootVersion::new(version number)
? Then we've got a little library, and a struct we could hang some helper functions on, like Ord for comparing.
All this could go in to a nice, small file with some tests just for it, in an easy to consume nibble of code. Compare that to here: 50 lines of self-contained-ish code burried 200 lines deep in a nearly 500 line long file.
Similar feedback applies to the systemd version stuff.
fb3f953
to
0fcb3ba
Compare
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.
Looking MUCH better with these improvements. This review is just one one specific commit, though. Maybe toss in a couple tests for cmp and eq?
Ok(version) | ||
} | ||
|
||
pub fn detect_version(bootctl: &Path, esp: &Path) -> Result<Option<Self>> { |
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.
This return type is a bit weird. It might be worth looking out for ways to improve this, like an enum of DetectionResult { ExecError, ParseError, Ok } -- but not sure, and definitely not asking you to do it now.
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.
Looking pretty good!
.status()?; | ||
} | ||
} else { | ||
warn!("could not find any previously installed systemd-boot"); |
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.
Note this is not warning-grade error if this is a new installation, what is the difference between installing and upgrading? Maybe we should automatically install if that fails?
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.
I just felt like there should be the "least magic" happening as possible -- if a user wants to install systemd-boot, they should use the --install
flag. Otherwise, they want to upgrade their bootloader. If systemd-boot isn't present, then that's fine; we'll just copy the kernels and boot entries to the specified ESP and let them install it either manually (via bootctl
) or via this tool.
Does that make sense? This is more a warning that "we'll copy everything you want, but it won't boot yet, because there's no installed bootloader (systemd-boot)".
Also test most of those split-out functions.
This copies the files to a temporary directory, then renames them to their
desired destination, and finally removes the temporary directory.
Usable in conjunction with https://github.com/DeterminateSystems/nixpkgs-priv/tree/boot-json and the following module (ripped from my flake; may need modifications):
Closes #2.
Closes #4.