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

Revert "nixos/grub: generate BLS entries" #385879

Merged
merged 1 commit into from
Mar 1, 2025

Conversation

samueldr
Copy link
Member

@samueldr samueldr commented Feb 28, 2025

Reverts #95901

See: #95901 (comment)

If this commit is in Hydra evals, they should be canceled so that no channel bumps contains it, as it can lead to breakage on user's systems. At the time of writing, it is not in any channel bump.

Test available here, please cherry-pick into the fixing PR:

cc @rnhmjoj

cc ”all grub maintainers” ENOTFOUND


This new behaviour is breaking assumptions under the BLS:

The Boot Loader Specification

This document defines a set of file formats and naming conventions that
allow the boot loader menu entries to be shared between multiple
operating systems
and boot loaders installed on one device.

(Emphasis my own.)

The changes here are currently breaking multi-boot setups where a BLS-aware bootloader is handling some boot options, and a NixOS is booted with GRUB.

It will remove the whole loader/entries folder, which may have included entries from another operating system.

Note that this also differs in behaviour compared to:

Not re-using the same generator for all BLS entries generation has led to accidentally having diverging implementations.

Additionally, this behaviour should be behind an option, so it can be disabled. It probably should be a default-false option, too. The added BLS entries will be shown in the BLS entries menu, which may not be desirable when specifically configuring NixOS for GRUB usage.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Feb 28, 2025
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Feb 28, 2025

You're not being very explicit with what is actually breaking. I guess removing the entries directory before installing the new one. This part:

# Atomically replace the BLS entries directory
my $entriesDir = "$bootPath/loader/entries";
rename $entriesDir, "$entriesDir.bak" or die "cannot rename $entriesDir to $entriesDir.bak: $!\n";
rename "$entriesDir.tmp", $entriesDir or die "cannot rename $entriesDir.tmp to $entriesDir: $!\n";
rmtree "$entriesDir.bak" or die "cannot remove $entriesDir.bak: $!\n";

Anyway, go on with the revert.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Feb 28, 2025

Not re-using the same generator for all BLS entries generation has led to accidentally having diverging implementations.

I unfortunately don't see this as an option: the systemd-boot script is in python, grub one is in perl and someone is attempting to rewrite it in rust. The implementations would probably differ anyway since GRUB can be both EFI or not.

The added BLS entries will be shown in the BLS entries menu, which may not be desirable when specifically configuring NixOS for GRUB usage.

Why not? Isn't that the point of these entries?

@samueldr
Copy link
Member Author

Oh oops you're right, sorry, looks like indeed I did not explicitly say that it would be removing all entries when generating the BLS entries. (It was supposed to.)

I'm amending the test case commit so it says it.

@samueldr
Copy link
Member Author

samueldr commented Feb 28, 2025

Not re-using the same generator for all BLS entries generation has led to accidentally having diverging implementations.

I unfortunately don't see this as an option: the systemd-boot script is in python, grub one is in perl

I'm not sure I see what the issue would be with the languages in use here. If it's about the system closure size, isn't python already part of it?

Otherwise, and it's more of a tangent, when I was involved with NixOS, and part of bootloader discussions, pretty much this exact topic was brought up regarding bootspec and goals in its implementation. Part of the goals was indeed to detach the generation of entries from the installation of the bootloaders, so that the different schemes generating entries could be enabled, even without installing the bootloaders themselves.

(If you don't know what bootspec is, and I don't intend to be patronizing, NixOS is just so big, look at /run/current-system/boot.json. It's all of the structured data needed to generate boot entries for a generation.)

and someone is attempting to rewrite it in rust.

Great. Though I'm not sure how this changes anything here.

The implementations would probably differ anyway since GRUB can be both EFI or not.

BLS entries are not (intended to be) tied to the bootloader using them. So EFI or not shouldn't change anything regarding generating BLS entries. Or am I missing something?

The added BLS entries will be shown in the BLS entries menu, which may not be desirable when specifically configuring NixOS for GRUB usage.

Why not? Isn't that the point of these entries?

Mismatched expectations.

Assuming a GRUB+EFI on “Wintel” platforms:

The BLS specification assumes a single "namespace" for all entries (if you will allow the simplification). The sorting semantics are not really great either.

So if a user is using systemd-boot to select between different operating systems, and relying on the GRUB support in NixOS to simplify it to a single entry in that menu, after this update the menu would be filled with unexpected entries, and may even push entries down, depending on how the sorting happens exactly.

Assuming non-systemd-boot BLS support:

Anything goes. A bootloader/platform firmware could be using those entries in preference over the EFI bootloader scheme. The BLS spec is pretty laissez-faire about this, since it's an implementation detail from their point of view.

About BLS entries and GRUB

There's also different BLS support implementations being made for GRUB, and I don't know if any of them is intended to be brought into upstream.

If it happens, I do not know how having both BLS and grub configuration would behave. This is not really an argument for or against, but something to consider. It might not match the expectations from the GRUB maintainers and developers that two bootloader configuration schemes would be used at the same time.


Another option to move forward?

I don't know if I'm missing anything but I have had this thought:

If the goal here is to make systemd kexec work, would it work out for this use-case to add a discrete option, not tied to the bootloader, which would generate the entries under /run/boot-loader-entries/? (Without putting them in the /boot folder.)

If I got the details right, since it wouldn't be tied to GRUB, it should allow using systemd kexec independent from the bootloading scheme. In theory, even schemes where NixOS is not “in control”.

@samueldr
Copy link
Member Author

Anyway, go on with the revert.

BTW, just saying, I'm not a committer anymore, so I will not be able to do so myself.

@mweinelt
Copy link
Member

mweinelt commented Mar 1, 2025

Cancelled unstable-small until this is sorted.

@ElvishJerricco
Copy link
Contributor

I spot several things wrong with the original PR. I will leave a review on the original PR after merging this revert. If the work is to be redone (and I'm not sure it should be), my review can be addressed in a new PR.

@ElvishJerricco ElvishJerricco merged commit f836b89 into NixOS:master Mar 1, 2025
51 of 52 checks passed
@samueldr samueldr deleted the revert-95901-grub-bls branch March 1, 2025 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants