Changed the 0-9 to \d and added [^)]+ outside of the capture group. Ready for another review pass.

I'm still on the fence about capturing vs not capturing the extra suffix like in the arch case. Could be swayed either way.

I'm also not sure that this check is actually correct. Should it be "always upgrade and never downgrade" or "make it match the version the module expects?" If the later, an equivalence check would be better than the string order.

Yeah, that's true. We'd need to parse the version as either a float if we assume it'll only ever contain one ., or a semver parser otherwise.

wouldn't cause issues until release of systemd 1000

That assumes that it ever actually got to that check, which it wouldn't 😛

Is there any reason we shouldn't just capture the entire version string and ensure that it always completely matches the version from nixpkgs? It might even be prudent to add a hash to the end of our version to make sure that it's actually the nixos binary and not just one that shares the same version.

Just to add a bit of context here since I originally wrote this code: I did incorrectly assume the version string would always just be a series of digits, however, if it was originally installed by NixOS then it would be. From a closer look at the code (boot/efi/boot.c line 28), we see that it is set to whatever GIT_VERSION is set to:

static const char _used_ _section_(".sdmagic") magic[] = "#### LoaderInfo: systemd-boot " GIT_VERSION " ####";

GIT_VERSION is set via src/version/version.h.in from @VCS_TAG@, which can be set via meson using -Dversion-tag. (This is how arch sets their custom version tag).

Another thing is that bootctl update itself will only update the boot loader if it detects that its version is newer than the one present. (see src/boot/bootctl.c, and track how verb_install will indirectly call copy_file_with_version_check, which calls down to compare_version. Earlier this year it looks like they replaced the simple version comparator with a more complicated one (strversmp_improved).

Since bootctl update will already do the version comparison for us, and duplicating that functionality in our code would be error prone, perhaps we can just always run bootctl update? I don't recall my original reasoning for duplicating the version comparison in systemd-boot-builder.py, but if it's not needed then we should try to get rid of it.

Since bootctl update will already do the version comparison for us, and duplicating that functionality in our code would be error prone, perhaps we can just always run bootctl update? I don't recall my original reasoning for duplicating the version comparison in systemd-boot-builder.py, but if it's not needed then we should try to get rid of it

That sounds like the right approach to me. At worst, the bootctl version check will get it wrong and install an older version from nixpkgs, which is arguably more correct anyway.

Edit: Hmm, after playing with it, I might understand the original rationale. The version check in bootctl update only skips copying if the installed one is newer. It seems to always update even if it's the same version. It's also rather noisy, though that could probably be suppressed.

Looks like this commit would solve that issue, but I don't believe it's in the current systemd used in NixOS yet:
systemd/systemd@e5a8b4b
And we should probably also add --graceful to the options.

This is interesting, this change has bigger ramifications:

Ignore failure when the EFI System Partition cannot be found, when EFI variables cannot be written, or a different or newer boot loader is already installed. Currently only applies to random seed and update operations.

I think this would actually fix a case of NixOS installs failing on systems like... say the raspberry pi4, with u-boot ->efi-> systemd-boot. Currently that's not an option because without --graceful, the install fails because it can't access UEFI variables.

It seems like graceful might be ... too graceful?

I really don't get why systemd doesn't default to:

• warn and skip if the installed version is the same or older
• have a --reinstall flag to force reinstall
• leave --graceful the way it is

Seems like it would've allowed more appropriate, granular behavior.

Edit: Hmm, after playing with it, I might understand the original rationale. The version check in bootctl update only skips copying if the installed one is newer. It seems to always update even if it's the same version. It's also rather noisy, though that could probably be suppressed.

Instead of comparing if the new version is greater, I think it would then be sufficient to just check if the version strings are different: If the new version is lower, bootctl update will do nothing (as before); if it is the newer, we'll install the new version.

Instead of comparing if the new version is greater, I think it would then be sufficient to just check if the version strings are different

This is exactly what I suggested here, though I was unaware that bootctl had a version check to prevent downgrades. I'm fine with either approach ¯\_(ツ)_/¯

@SuperSandro2000 Is there anything left blocking this from being merged?

@SuperSandro2000 Is there anything left blocking this from being merged?

me not knowing anything about systemd-boot.

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/could-not-find-any-previously-installed-systemd-boot/15627/3

The change works 👍. Working systemd-boot post ~248 exposes some users to systemd/systemd#19191 (see #143847). The? documented way to rollback the bootloader is also broken #144007.

The change works +1. Working systemd-boot post ~248 exposes some users to systemd/systemd#19191 (see #143847). The? documented way to rollback the bootloader is also broken #144007.

Should we patch our systemd with systemd/systemd@e98d271 before merging this?

cc @flokli (re patching systemd)

cc @flokli (re patching systemd)

I replied on the issue for that (#143847 (comment)). Let's do the discussion about patching or not there.

I just merged #144422 into staging.

Could this be rebased on top of (and target) staging as well? I'd like to avoid this making bootctl install work again, and potentially breaking machines affected by #143847 until the fix from staging ends up in master.

Could this be rebased on top of (and target) staging as well?

Done!

Thanks!

Those should now both end in the staging-next cycle after #144730.

WHY IS THIS PARSED ANYWAY

I’m sorry but why the heck are we parsing display output of bootctl? Is there no machine-readable output format?