Repository navigation
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
osnyx
force-pushed
the
PL-135147-grub-install-condition
branch
from
February 24, 2026 15:04
c202bc8 to
63e57d7
Compare
Member
Author
|
This draft PR is only filed for an internal revision round. A follow-up PR will be filed against upstream nixpkgs instead. |
Trigger re-installation of grub files when the store path of the grub package changes. This is consistent with how side-effects are executed in other parts of NixOS, e.g. systemd service management, and ensures that the actually running grub stays in sync with the grub package in the Nix store that is part of the system closure. In particular, this causes security patches or dependency changes to be taken into effect without the need to bump the package version. Impact: This change causes a re-installation of grub for all systems, due to the lack of a persisted grub store path in existing state files. Background: So far we only re-installed grub outside of the store when its its version, the package name, or some flags changed. This leads to the various security patches only fixing new installations of grub and not existing ones. This has been like this for over a decade, but it remains unclear why the implementation decision had been to be overly cautious with modifying boot loader side effects. Tested: - on a grub BIOS system in a qemu VM - migration from an old grub state file works - checked that grub is re-installed when the package store path changes - checked that grub is not re-installed at system switch events with equal package store paths - NOT tested on a grubEfi system Fixes nixpkgs issue 486315 PL-135147
osnyx
force-pushed
the
PL-135147-grub-install-condition
branch
from
February 24, 2026 15:21
63e57d7 to
71411ed
Compare
sysvinit
requested changes
Feb 26, 2026
sysvinit
left a comment
sysvinit
left a comment
Member
There was a problem hiding this comment.
Couple of small style nits but otherwise looks good.
| efiMountPoint => '$', | ||
| extraGrubInstallArgs => '@', | ||
| grub => '$', | ||
| grubEfi => '$', |
Member
There was a problem hiding this comment.
Probably better to put the grub and grubEfi arguments before the existing extraGrubInstallArgs, so the latter can still capture a variable number of trailing arguments without needing to wrap them in an arrayref. Compare def func(grub, grubEfi, *extraGrubInstall) in Python.
| # because it is read line-by-line. | ||
| sub readGrubState { | ||
| my $defaultGrubState = GrubState->new(name => "", version => "", efi => "", devices => "", efiMountPoint => "", extraGrubInstallArgs => () ); | ||
| my $defaultGrubState = GrubState->new(name => "", version => "", efi => "", devices => "", efiMountPoint => "", extraGrubInstallArgs => [], grub => "", grubEfi => "" ); |
Member
There was a problem hiding this comment.
See above, with the reordered args extraGrubInstallArgs can remain an empty list () rather than being an arrayref [].
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Trigger re-installation of grub files when the store path of the grub package changes.
This is consistent with how side-effects are executed in other parts of NixOS, e.g. systemd service management, and ensures that the actually running grub stays in sync with the grub package in the Nix store that is part of the system closure.
In particular, this causes security patches or dependency changes to be taken into effect without the need to bump the package version.
Background:
So far we only re-installed grub outside of the store when its its version, the package name, or some flags changed.
This leads to the various security patches only fixing new installations of grub and not existing ones. This has been like this for over a decade, but it remains unclear why the implementation decision had been to be overly cautious with modifying boot loader side effects.
Tested:
Fixes nixpkgs issue 486315
PL-135147
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.