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

lib.systems.inspect: deprecate isEfi #264297

Closed
wants to merge 1 commit into from
Closed

lib.systems.inspect: deprecate isEfi #264297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion lib/systems/inspect.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ with lib.lists;
let abis_ = abis; in
let abis = lib.mapAttrs (_: abi: builtins.removeAttrs abi [ "assertions" ]) abis_; in

rec {
let result = rec {
# these patterns are to be matched against {host,build,target}Platform.parsed
patterns = rec {
# The patterns below are lists in sum-of-products form.
Expand All @@ -31,9 +31,11 @@ rec {
];
isx86 = { cpu = { family = "x86"; }; };
isAarch32 = { cpu = { family = "arm"; bits = 32; }; };
isArmv6 = { cpu = { family = "arm"; version = "6"; }; };
isArmv7 = map ({ arch, ... }: { cpu = { inherit arch; }; })
(lib.filter (cpu: lib.hasPrefix "armv7" cpu.arch or "")
(lib.attrValues cpuTypes));
isArmv8 = { cpu = { family = "arm"; version = "8"; }; };
isAarch64 = { cpu = { family = "arm"; bits = 64; }; };
isAarch = { cpu = { family = "arm"; }; };
isMicroBlaze = { cpu = { family = "microblaze"; }; };
Expand Down Expand Up @@ -114,4 +116,22 @@ rec {
platformPatterns = mapAttrs (_: p: { parsed = {}; } // p) {
isStatic = { isStatic = true; };
};
};

warnEfiDeprecated = throw "(U)EFI support is a feature of specific motherboards rather than entire architectures or platforms; this predicate has been deprecated because its name is misleading";

in
result // {
patterns = result.patterns // {
isEfi = warnEfiDeprecated result.patterns.isEfi;
};

# Unfortunately a strictness bug in lib.systems.equals means that
# we can't lib.warn on accesses to boolean values. If
# https://github.com/NixOS/nixpkgs/pull/238331 is merged we can go
# back to using Nix `==` and this warning can be enabled.
#
#predicates = result.predicates // {
# isEfi = argument: warnEfiDeprecated (result.predicates.isEfi argument);
#};
Comment on lines +129 to +136
Copy link
Author

Choose a reason for hiding this comment

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

This problem was introduced by

After that PR, we are no longer able to deprecate boolean-valued attributes of platform objects because lib.systems.equals is too strict.

We could get back to using ==, which is lazy enough to handle this correctly, by merging

}
4 changes: 3 additions & 1 deletion pkgs/development/libraries/gnu-efi/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ stdenv.mkDerivation rec {
description = "GNU EFI development toolchain";
homepage = "https://sourceforge.net/projects/gnu-efi/";
license = licenses.bsd3;
platforms = platforms.linux;
platforms = with lib.systems.inspect.patterns; map (pat: pat // isLinux) ([
isArmv6 ] ++ isArmv7 ++ [ isArmv8 isRiscV isx86
]);
Copy link
Member

Choose a reason for hiding this comment

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

Using doubles strings rather than patterns would make the code here less complicated, and have the same effect, right?

Copy link
Member

Choose a reason for hiding this comment

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

I thought @Ericson2314 wanted to get rid of double strings but maybe I misinterpreted him.

Copy link
Member

Choose a reason for hiding this comment

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

It's not something we're going to move away from one package at a time regardless…

Copy link
Author

@ghost ghost Nov 1, 2023 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

Using doubles strings rather than patterns would make the code here less complicated, and have the same effect, right?

Not exactly.

The isEfi predicate that I removed uses a pattern, so to get precisely the same effect I used a pattern here. The main issue is that the { cpu = { family = "arm"; version = 6,7; }; } patterns will match any string with the prefix armv6/armv7, and likewise for version = 8 with aarch64. There are quite a lot of these, with more being added all the time apparently (aarch64ec!?)

Also double-strings are icky gross.

It's not something we're going to move away from one package at a time regardless…

I dunno, isn't incremental migration the easy way to do it?

maintainers = with maintainers; [ ];
};
}
4 changes: 2 additions & 2 deletions pkgs/os-specific/linux/systemd/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@
, withCryptsetup ? true
, withRepart ? true
, withDocumentation ? true
, withEfi ? stdenv.hostPlatform.isEfi
, withEfi ? with stdenv.hostPlatform; isArmv6 || isArmv7 || isArmv8 || isRiscV || isx86
, withFido2 ? true
, withFirstboot ? false # conflicts with the NixOS /etc management
, withHomed ? !stdenv.hostPlatform.isMusl
Expand Down Expand Up @@ -756,7 +756,7 @@ stdenv.mkDerivation (finalAttrs: {
# runtime; otherwise we can't and we need to reboot.
interfaceVersion = 2;

inherit withCryptsetup withHostnamed withImportd withKmod withLocaled withMachined withPortabled withTimedated withUtmp util-linux kmod kbd;
inherit withCryptsetup withHostnamed withImportd withKmod withLocaled withMachined withPortabled withTimedated withUtmp util-linux kmod kbd withEfi;

tests = {
inherit (nixosTests) switchTest;
Expand Down
4 changes: 1 addition & 3 deletions pkgs/top-level/all-packages.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21804,9 +21804,7 @@ with pkgs;

gnu-config = callPackage ../development/libraries/gnu-config { };

gnu-efi = if stdenv.hostPlatform.isEfi
then callPackage ../development/libraries/gnu-efi { }
else null;
gnu-efi = callPackage ../development/libraries/gnu-efi { };

gnutls = callPackage ../development/libraries/gnutls {
inherit (darwin.apple_sdk.frameworks) Security;
Expand Down