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

install-grub.pl: support bindmounts #374398

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kira-bruneau
Copy link
Contributor

@kira-bruneau kira-bruneau commented Jan 16, 2025

Specifically, this fixes grub generation when bind-mounting your /nix & /boot directory with NixOS impermanence.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@kira-bruneau kira-bruneau requested review from nh2 and roberth January 16, 2025 23:09
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 16, 2025
@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Jan 16, 2025

@pluiedev Just a heads-up, you'll probably want to port this in #317026 once this PR is merged.

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Jan 16, 2025

It looks like the code in GetFs was originally added in #3961 to replace df -T. findmnt uses /proc/self/mountinfo internally though, so it shouldn't have the same problem df -T originally did.

See:

@kira-bruneau kira-bruneau force-pushed the install-grub branch 3 times, most recently from ca00777 to 277371b Compare January 17, 2025 19:38
@kira-bruneau kira-bruneau changed the title install-grub.pl: use findmnt to get the boot filesystem install-grub.pl: support bindmounts and btrfs subvolumes Jan 17, 2025
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 17, 2025
@github-actions github-actions bot added 6.topic: python 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: haskell 8.has: documentation This PR adds or changes documentation 8.has: changelog 6.topic: emacs Text editor 6.topic: rust 6.topic: policy discussion 6.topic: vim 6.topic: hardware 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: testing Tooling for automated testing of packages and modules 6.topic: module system About "NixOS" module system internals 6.topic: jitsi and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Jan 17, 2025
@github-actions github-actions bot removed 6.topic: vim 6.topic: hardware 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: testing Tooling for automated testing of packages and modules 6.topic: module system About "NixOS" module system internals 6.topic: jitsi 6.topic: java Including JDK, tooling, other languages, other VMs 6.topic: flakes The experimental Nix feature 6.topic: lib The Nixpkgs function library 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 6.topic: deepin Desktop environment and its components 6.topic: dotnet Language: .NET 6.topic: nvidia 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions labels Jan 17, 2025
@NixOS NixOS deleted a comment from nix-owners bot Jan 17, 2025
@NixOS NixOS deleted a comment from nix-owners bot Jan 17, 2025
@NixOS NixOS deleted a comment from nix-owners bot Jan 17, 2025
@NixOS NixOS deleted a comment from nix-owners bot Jan 17, 2025
@NixOS NixOS deleted a comment from nix-owners bot Jan 17, 2025
@NixOS NixOS deleted a comment from nix-owners bot Jan 17, 2025
@kira-bruneau kira-bruneau changed the title install-grub.pl: support bindmounts and btrfs subvolumes install-grub.pl: support bindmounts Jan 17, 2025
@kira-bruneau kira-bruneau marked this pull request as draft January 17, 2025 20:10
The current implementation of GetFS uses heuristics to find the best
filesystem for the given boot path. Using findmnt is more robust.
Comment on lines -220 to -243

# BTRFS is a special case in that we need to fix the referenced path based on subvolumes
if ($fs->type eq 'btrfs') {
my ($status, @id_info) = runCommand("@btrfsprogs@/bin/btrfs", "subvol", "show", @{[$fs->mount]});
if ($status != 0) {
die "Failed to retrieve subvolume info for @{[$fs->mount]}\n";
}
my @ids = join("\n", @id_info) =~ m/^(?!\/\n).*Subvolume ID:[ \t\n]*([0-9]+)/s;
if ($#ids > 0) {
die "Btrfs subvol name for @{[$fs->device]} listed multiple times in mount\n"
} elsif ($#ids == 0) {
my ($status, @path_info) = runCommand("@btrfsprogs@/bin/btrfs", "subvol", "list", @{[$fs->mount]});
if ($status != 0) {
die "Failed to find @{[$fs->mount]} subvolume id from btrfs\n";
}
my @paths = join("", @path_info) =~ m/ID $ids[0] [^\n]* path ([^\n]*)/;
if ($#paths > 0) {
die "Btrfs returned multiple paths for a single subvolume id, mountpoint @{[$fs->mount]}\n";
} elsif ($#paths != 0) {
die "Btrfs did not return a path for the subvolume at @{[$fs->mount]}\n";
}
$path = "/$paths[0]$path";
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btrfs no longer needs to be treated specially here since $fs->root will contain the subvolume path and will be resolved the same way bind mounts are resolved.

@kira-bruneau kira-bruneau marked this pull request as ready for review January 18, 2025 02:10
Specifically this allows bind-mounting your `/boot` directory with
NixOS impermanence.
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: 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.

1 participant