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.

Sign up for GitHub

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

Jump to bottom

iso-image: Fix issues with UEFI installer image #124076

Merged
merged 4 commits into from
May 23, 2021
Merged

iso-image: Fix issues with UEFI installer image #124076

merged 4 commits into from
May 23, 2021

Conversation

samueldr
Copy link
Member

@samueldr samueldr commented May 22, 2021 •
edited
Loading

Tries to handle #123376

Things done

Verified on:

  • qemu (-cdrom)
  • Acer c720p
  • Pinebook (A64)
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Added a release notes entry if the change is major or breaking
  • Fits CONTRIBUTING.md.

@samueldr samueldr mentioned this pull request May 22, 2021
Closed
@hyperfekt
Copy link
Contributor

hyperfekt commented May 22, 2021 •
edited
Loading

The date issue is fixed, the one of failed theming is not. I can't comment on the resolution one since I don't have that.

@hyperfekt
Copy link
Contributor

@ncfavier Can you confirm the font problem is fixed? In that case we should merge a PR with either just the (hd0) → ($root) commit or that and the revert backed out and resolve the blocker.

@ncfavier
Copy link
Member

Building the ISO now.

To be clear, ($root)/foo is the same as just /foo. I don't think we should merge this until we figure out a way to properly set the $root, which probably involves using the search command.

@hyperfekt
Copy link
Contributor

hyperfekt commented May 22, 2021 •
edited
Loading

Well, that's just an issue of lacking theming, and the other commits restore the ability to boot. Which is why, as I see it, the latter should prevent a release of 21.05 of while the former should not. This is why I'd like to separate the two parts and merge them separately, so we can take our time and find out how to solve the theming issue on all platforms without making @jonringer sad.

@ncfavier
Copy link
Member

Fair enough.

@jonringer
Copy link
Contributor

without making @jonringer sad.

at this point, all software makes me sad

@samueldr samueldr force-pushed the fix/grub-hard-to-debug-failures branch from 67fb897 to a7cd8f8 Compare May 22, 2021 23:33
@ncfavier
Copy link
Member

ncfavier commented May 22, 2021 •
edited
Loading

OK, I can confirm that this fixes the date bug and the resolution bug (at least on my machine).

d64034d5e79cf454a995d3270931d02c67287e0b and f9e461833f1dc4af94ec4fc6f83e70f6d2844f89 cancel each other out, so you might want to rebase them out.

For the record, here's a list of common VBE video modes.

@hyperfekt
Copy link
Contributor

Fix is still good after the force-push.

@samueldr
Copy link
Member Author

samueldr commented May 23, 2021 •
edited
Loading

AFAIUI vbe is irrelevant here since we're in an UEFI gop (Graphics Output Protocol) driver.

@ncfavier
Copy link
Member

From the manual:

Note that you can only use modes which your graphics card supports via VESA BIOS Extensions (VBE)

I have no idea though.

@samueldr
Copy link
Member Author

Yeah, GRUB's documentation is still heavily biased to their "legacy" program.

We're exclusively using the UEFI version of GRUB with the iso.

@ncfavier
Copy link
Member

Aha you're right, videoinfo lists modes that aren't in that table.

@jonringer jonringer added the 9.needs: port to stable A PR needs a backport to the stable release. label May 23, 2021
@samueldr
Copy link
Member Author

samueldr commented May 23, 2021 •
edited
Loading

haha! detecting as (hd0,msdos2) breaks themes here! An artificial reproducer!

So I guess the issue was that GRUB detected itself as (hd0,msdos2). (For the theme issue.)

File names are lowercased when ls'd! This differs from the iso FS.

@hyperfekt
Copy link
Contributor

The latest commit is unsuccessful in fixing the theming issue for me, in case that is what it is supposed to do.

@samueldr
Copy link
Member Author

samueldr commented May 23, 2021 •
edited
Loading

@hyperfekt yeah, I pushed it eagerly, and then I figured out that reproduces the themeing issue.

When booted with that root things are awfully broken... ls /EFI/boot/grub-theme doesn't work. But manually running set root=(hd0) and it works just fine.

@samueldr
iso-image: Force gfxmode
18f7e4d 
https://www.gnu.org/software/grub/manual/grub/html_node/gfxmode.html
@samueldr
iso-image: change date on all files
369e2ba 
It may be that in some conditions dates earlier than 1980 on FAT on GRUB
2.06~ish will cause failures

NixOS#123376 (comment)
@samueldr
iso-image: unqualified root → ($root)
d288150 
This technically changes nothing. In practice `$root` is always the
"CWD", whether searched for automatically or not.

But this serves to announce we are relying on `$root`... I guess...
@samueldr
iso-image: Improve disk detection
ca0ebe3 
This should help in rare hardware-specific situations where the root is
not automatically detected properly.

We search using a marker file. This should help some weird UEFI setups
where the root is set to `(hd0,msdos2)` by default.

Defaulting to `(hd0)` by looking for the ESP **will break themeing**. It
is unclear why, but files in `(hd0,msdos2)` are not all present as they
should be.

This also fixes an issue introduced with cb5c4fc
where rEFInd stopped booting in many cases. This is because it ended up
using (hd0) rather than using the `search` which was happening
beforehand, which in turn uses (hd0,msdos2), which is the ESP.
Putting back the `search` here fixes that.
@samueldr samueldr force-pushed the fix/grub-hard-to-debug-failures branch from 831b352 to ca0ebe3 Compare May 23, 2021 01:49
@samueldr
Copy link
Member Author

samueldr commented May 23, 2021 •
edited
Loading

I need to test on AArch64 still.

But this should be good. Can I get a verification for your particular issues?

@hyperfekt
Copy link
Contributor

hyperfekt commented May 23, 2021 •
edited
Loading

This is the one. Theming works now, and the date thing is still resolved. Good job! 🥳

@jonringer
Copy link
Contributor

awesome turn around. Thanks for working on this all :)

@samueldr samueldr marked this pull request as ready for review May 23, 2021 02:42
@samueldr
Copy link
Member Author

Verified with Tow-Boot on my Pinebook A64 that it works just as well as it did.

@jonringer
Copy link
Contributor

@GrahamcOfBorg test grub

@samueldr samueldr changed the title iso-image: Fix issues with GRUB iso-image: Fix issues with UEFI installer image May 23, 2021
jonringer
jonringer approved these changes May 23, 2021
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

diff LGTM

tests still checkout on x86, aarch64 was tested

@jonringer jonringer merged commit 20b023b into NixOS:master May 23, 2021
@samueldr samueldr deleted the fix/grub-hard-to-debug-failures branch May 23, 2021 04:31
@ncfavier
Copy link
Member

ncfavier commented May 23, 2021 •
edited
Loading

This works for me, but only by accident: the nixos-installer-image marker file ends up on both (hd0) and (hd0,msdos2), so whichever GRUB tries first is picked. We should make sure to use a file that's only on (hd0), or use the volume label since that's already how linux finds its root.

EDIT: actually, shouldn't we just move the theme files to (hd0,msdos2)? I'm not sure why there's even anything GRUB-related on (hd0) at all.

@samueldr
Copy link
Member Author

It's not actually a different FS. It's isohybrid trickery.

We should look into why some files are not present.

But for the time being the failure state is seemingly lack of theme. Which isn't much of an issue as long as the user can boot in the system!

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/21-05-has-been-released/13407/5

@jonringer jonringer added 8.has: port to stable A PR already has a backport to the stable release. and removed 9.needs: port to stable A PR needs a backport to the stable release. labels Jun 3, 2021
@codelamb

This comment has been minimized.

Sign in to view
@samueldr samueldr mentioned this pull request Jul 30, 2021
Closed
@samueldr
Copy link
Member Author

(I "moved" this comment into a new issue as it is irrelevant to this PR see #132094 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncfavier ncfavier ncfavier left review comments

@jonringer jonringer jonringer approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 8.has: port to stable A PR already has a backport to the stable release. 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@samueldr @hyperfekt @ncfavier @jonringer @nixos-discourse @codelamb