-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
syslinux: Use Nixpkgs' gnu-efi #317030
syslinux: Use Nixpkgs' gnu-efi #317030
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me, I can't really judge how risky it is to use a different version of gnu-efi than is referenced by the syslinux submodule...
The main risk would be that it stops building with it due to backwards-incompatible changes. Technically the added patch works around a backwards-incompatible change, but barely so. (A feature was split off out into its own header.) If it causes issues down the line, I believe it should be fine to disable building the EFI outputs entirely. Anyways they are not used within Nixpkgs nor NixOS. To do so would be relatively simple, removing the references to I actually hesitated and almost went with just removing the EFI outputs. |
This resolves awkwardness with the gnu-efi submodule. Though this builds only the arch-appropriate EFI program, but since it was not used within Nixpkgs, it shouldn't be an issue.
31e1dcc
to
d38bc8b
Compare
Successfully created backport PR for |
Somehow, and I genuinely have no idea how, this breaks the ISO boot: https://cache.nixos.org/log/z24ffw9730kzaaljzy18g67jjnn2ix9h-vm-test-run-boot-bios-cdrom.drv |
oof, thanks for the quick revert :/ I can indeed reproduce this with |
created #317158 |
rev = "b40487005223a78c3bb4c300ef6c436b3f6ec1f7"; | ||
sha256 = "sha256-GqvRTr9mA2yRD0G0CF11x1X0jCgqV4Mh+tvE0/0yjqk="; | ||
fetchSubmodules = true; | ||
src = fetchFromRepoOrCz { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know that existed. fetchgit defaults to fetching submodules, I assume this doesnt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also uses a zip instead of git.
(fetchpatch { | ||
# Fixes build with "modern" gnu-efi | ||
# https://github.com/samueldr/syslinux/commit/68defee52f4eba82eefaeea17f21c7498448dd6b | ||
url = "https://github.com/samueldr/syslinux/commit/68defee52f4eba82eefaeea17f21c7498448dd6b.patch"; | ||
hash = "sha256-5xIqM8Gq8D86HL1i7fBTHRB/ZR2XtCz5VsT4bI5vneo="; | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh we would have been probably better off to just vendor that tiny patch to not rely on your repo 😅
Description of changes
#316878 aimed to change how
gnu-efi
was fetched. My understanding (after futzing about) is that sourceforge's git endpoint now doesn't provide direct commit access.Instead of awkwardly fetching an older
gnu-efi
, prefer using Nixpkgs'gnu-efi
.Note that within Nixpkgs only the "legacy bios" syslinux is used. We could also drop the EFI build instead.
Closes #316878.
Things done
Ran the
.efi
output, from before the change, and after. Same result.nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.