nixos/iso-image: fix isoImage.grubTheme = null; logic#156754
nixos/iso-image: fix isoImage.grubTheme = null; logic#156754Artturin merged 1 commit intoNixOS:masterfrom
Conversation
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin: 0
|
Untested, but the logic looks sound. |
|
Do you need someone to test this? |
|
ideally, that would be great :) |
|
I rebased the PR on the latest master |
|
Hm. It doesn't even build. May have stumbled upon deeper bug. Without patch, at head With patch ( and Derivation attached |
|
Ok, squashfs takes awhile... But this works diff --git a/nixos/modules/installer/cd-dvd/iso-image.nix b/nixos/modules/installer/cd-dvd/iso-image.nix
--- a/nixos/modules/installer/cd-dvd/iso-image.nix
+++ b/nixos/modules/installer/cd-dvd/iso-image.nix
@@ -317,13 +317,13 @@ let
set textmode=true
terminal_output gfxterm console
}
- '' + lib.optionalString (config.isoImage.grubTheme != null) ''
+ ${lib.optionalString (config.isoImage.grubTheme != null) ''
hiddenentry 'GUI mode' --hotkey 'g' {
$(find ${config.isoImage.grubTheme} -iname '*.pf2' -printf "loadfont (\$root)/EFI/boot/grub-theme/%P\n")
set textmode=false
terminal_output gfxterm
}
- '' + ''
+ ''}
# If the parameter iso_path is set, append the findiso parameter to the kernel
# line. We need this to allow the nixos iso to be booted from grub directly. |
stale
bot
added
the
2.status: stale
|
Can I help to get this merged? |
stale
bot
removed
the
2.status: stale
|
can this be merged to fix this bug? |
|
testing with: |
|
@ofborg test installer |
wegank
added
the
2.status: stale
|
@Artturin I believe this broke the installer iso on master: The parent 914616a builds ine, since fa4b963 I get the following Line 608 in the mentioned file is "usage_size=..." below, but it might be due to the single quote on 602 (first in this snippet)? |
|
The code is messy enough that I don't really feel too much like digging into it, but at first glance it looks like the added conditional confuses the nix parser and lets it return the last string instead of the pkg.runCommand call. You can verify by i.e. adding a lib.traceVal around efiDir or brackets around the efiDir declaration. The later fixes the build, but I am fine with reverting and think it's reasonable to expect people to at least build an uncustomized isoImage before merging to that module. |
|
I ran the simple installer test which i thought would test this but didn't, my bad. |
|
The installer test ironically doesn't test the ISO. It tests the procedure of installing and rebuilding nixos with |
|
|
|
#359374 does string interpolation like suggested earlier in this thread, I thought jonringer fixed it a another way since he pushed after that comment |
9 participants
Motivation for this change
To fix:
closes: #156753
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes