Skip to content

Conversation

hehongbo
Copy link
Contributor

Previously, we used platforms.gnu, plus platforms.linux for Grub2's available platforms, but not all of them can build if we ask for a specific grub variant (like EFI). So canEfi is there to compare stdenv.hostPlatform.system against efiSystemsBuild. But it's used as an assert, which is bad and fixed by grub2_efi: turn asserts into meta.broken #427180.

However, as we know, the Grub2 package has been unmaintained for a while and has different variants with their available platforms loosely defined; it has never been restructured in a unified way. See:

  • There is if xenSupport that overrides meta.platforms if we're building pkgs.grub2_xen. That's the PVGrub2 used by Xen.
  • pkgs.grub2_efi, however, has no such treatment. An assert (and later meta.broken) is used to block platforms that can't do EFI.
  • The pkgs.grub2_xen is documented to be available for x86_64-linux and i686-linux, but efiSystemsBuild is reused for building it, but not all of them can produce a proper PVGrub2 binary.

So I think it would be worthwhile to restructure them and document the supporting status of each variant clearly in meta.platforms, and this is a part of what this PR is trying to do.

Regarding the use of meta.broken (and the use of it on the other 2 argument misuse situations in particular), I'm sorry for questioning this in this way, but I believe it's semantically incorrect, and potentially misleading to the user. Assertions are the correct contract for disallowed feature combinations: they’re not broken, they’re logically nonsensical. meta.broken, however, is for actual broken packages, or known-unbuildable targets; it would mislead end users, who might think support is missing rather than invoked incorrectly.

  • We have assert !(efiSupport && xenSupport); before, that reads "don't build a Grub2 package that is both EFI and Xen, that's nonsense."
  • meta.broken = efiSupport && xenSupport;, however, reads "the EFI+Xen variant of Grub (like pkgs.grub2_efi_xen) is broken"
  • For the ZFS support, there is no pkgs.grub2_zfs; the Grub2 with ZFS support is built locally once the corresponding option is set. But imagine someone who tries to install NixOS on their zpool without reading much into the documentation, they might build the Grub2 with ZFS support but forget to pass zfs as its dependency, then seeing the broken message might mislead them that the ZFS support of Grub2 is currently broken, and they should try another day.
  • The pkgs.grub2_xen is for booting guests that use the old PV guest type. We (the Xen team) are cooking up the support of PVH (as #374753), which is the new guest type that a modern Xen Project hypervisor can run, and will eventually replace the old PV type. Until then, we will have more conditions to stack to the meta.broken, which makes it more misleading for future users and contributors.

Also, correct me from wrong: Unlike efiSupport, which will be built once pkgs.grub2_efi is evaluated, zfsSupport and xenSupport default to false, so our Hydra/CI won't evaluate them, right?

So I reverted the other 2 argument misuse situations back to asserts, but this one is open to discussion:

  • Do you think we should give loud, early assertion failures if they do not harm CI?
  • Or, if we don't like assertions, can we just remove them to avoid misleading? (In my attempts, build with xenSupport = true; efiSupport = true; will not result in failure, though the artifact might be meaningless; and even zfsSupport = true; zfs = null; will not fail the build, but whether or not this thing can boot a zpool is another story.)

cc @wolfgangwalther

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jul 27, 2025
Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

Do you think we should give loud, early assertion failures if they do not harm CI?

I think using asserts for this purpose is fine, actually. The problem with "asserts should not be hit in CI" is not so much with these asserts themselves, but more with the fact that if we hit them, we'd let CI produce invalid combinations of arguments.

So yes, I'm fine with moving these back to asserts, if we're not hitting them.

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Aug 3, 2025
Previously, we used `platforms.gnu` and `platforms.linux` for Grub2's available
platforms, but blocking unsupported platforms via asserts. An attempt has been
made through marking unsupported platforms "broken" for EFI, but it would be
better if we just document the supported platforms in `meta.platforms` in the
first place. Also, the Grub2 package already has platform support for
PC/EFI/Xen, which deserves to be restructured in a unified way.

Regarding `meta.broken`, it shouldn't be used on invalid combinations of
arguments, as it will cause misunderstandings.

Changes have been made:
- Remove `meta.broken`, along with the 3 conditions in it.
- Remove `canEfi`, chain `efiSystemsBuild` on `meta.platform` if
  `pkgs.grub2_efi` is asked (building with `efiSupport = true;`)
- Avoid reusing `efiSystemsBuild` for Xen (PV), build a dedicated
  `xenSystemsBuild` for Xen, and chain that into `meta.platforms` in the same
  format.
- Revert the other 2 conditions (building with invalid arguments) back to
  asserts.

Signed-off-by: Hongbo <[email protected]>
@hehongbo hehongbo force-pushed the pr/grub-platform-refactor branch from 6d3cc3f to f92169c Compare August 3, 2025 18:08
@wolfgangwalther wolfgangwalther merged commit b52a58d into NixOS:master Aug 4, 2025
25 of 27 checks passed
@wolfgangwalther
Copy link
Contributor

Thank you!

@hehongbo hehongbo deleted the pr/grub-platform-refactor branch August 4, 2025 14:28
colemickens pushed a commit to colemickens/nixpkgs that referenced this pull request Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants