pkgs/top-level: make package sets composable (reapply) #376988
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,7 +73,10 @@ let | |
| defaultPkgs = | ||
| if opt.hostPlatform.isDefined then | ||
| let | ||
| isCross = | ||
| !(lib.systems.equals (lib.systems.elaborate cfg.buildPlatform) ( | ||
| lib.systems.elaborate cfg.hostPlatform | ||
| )); | ||
| systemArgs = | ||
| if isCross then | ||
| { | ||
|
|
@@ -195,13 +198,10 @@ in | |
| }; | ||
|
|
||
| hostPlatform = lib.mkOption { | ||
| type = lib.types.either lib.types.str lib.types.attrs; | ||
| example = { | ||
| system = "aarch64-linux"; | ||
| }; | ||
| defaultText = lib.literalExpression ''(import "''${nixos}/../lib").lib.systems.examples.aarch64-multiplatform''; | ||
| description = '' | ||
| Specifies the platform where the NixOS configuration will run. | ||
|
|
@@ -213,22 +213,13 @@ in | |
| }; | ||
|
|
||
| buildPlatform = lib.mkOption { | ||
| type = lib.types.either lib.types.str lib.types.attrs; | ||
| default = cfg.hostPlatform; | ||
| example = { | ||
| system = "x86_64-linux"; | ||
| }; | ||
| # Make sure that the final value has all fields for sake of other modules | ||
| # referring to this. | ||
|
|
||
| defaultText = lib.literalExpression ''config.nixpkgs.hostPlatform''; | ||
| description = '' | ||
| Specifies the platform on which NixOS should be built. | ||
|
|
@@ -245,14 +236,11 @@ in | |
| }; | ||
|
|
||
| localSystem = lib.mkOption { | ||
| type = lib.types.attrs; | ||
| default = { inherit (cfg) system; }; | ||
| example = { | ||
| system = "aarch64-linux"; | ||
| }; | ||
| defaultText = lib.literalExpression ''(import "''${nixos}/../lib").lib.systems.examples.aarch64-multiplatform''; | ||
| description = '' | ||
| Systems with a recently generated `hardware-configuration.nix` | ||
|
|
@@ -280,7 +268,7 @@ in | |
| # is a relation between at least 2 systems in the context of a | ||
| # specific build step, not a single system. | ||
| crossSystem = lib.mkOption { | ||
| type = lib.types.nullOr lib.types.attrs; | ||
| default = null; | ||
| example = { | ||
| system = "aarch64-linux"; | ||
|
|
@@ -416,6 +404,18 @@ in | |
| ${lib.concatMapStringsSep "\n" (file: " - ${file}") opt.config.files} | ||
| ''; | ||
| } | ||
| { | ||
| assertion = | ||
| (opt.hostPlatform.isDefined -> builtins.isAttrs cfg.buildPlatform -> !(cfg.buildPlatform ? parsed)) | ||
|
Member
There was a problem hiding this comment.
Contributor
Author
There was a problem hiding this comment. Well,
Contributor
There was a problem hiding this comment.
It could be defined as a string, but previously the
Contributor
There was a problem hiding this comment. I guess what we really need here is a
Contributor
Author
There was a problem hiding this comment. The important part is, that it must be possible to pass the inputs to the module (localSystem, crossSystem, buildPlatform, hostPlatform) onwards unchanged. No matter what you do as But there is really no need to be able to access
Contributor
There was a problem hiding this comment.
It seems that up until now, it was intended for users to be able to read those options. But I agree, accessing I'm trying to think about whether there's a sane way to enforce that though... Obviously, the option descriptions should be updated with a note explaining they are write-only, and Perhaps an # Bypass `apply` function by merging `foo` manually:
(
lib.modules.mergeDefinitions
options.foo.loc
options.foo.type
options.foo.definitionsWithLocations
).mergedValue |
||
| && (opt.hostPlatform.isDefined -> builtins.isAttrs cfg.hostPlatform -> !(cfg.hostPlatform ? parsed)) | ||
| && (builtins.isAttrs cfg.localSystem -> !(cfg.localSystem ? parsed)) | ||
| && (builtins.isAttrs cfg.crossSystem -> !(cfg.crossSystem ? parsed)); | ||
| message = '' | ||
| Passing fully elaborated systems to `nixpkgs.localSystem`, `nixpkgs.crossSystem`, `nixpkgs.buildPlatform` | ||
| or `nixpkgs.hostPlatform` will break composability of package sets in nixpkgs. For example, pkgs.pkgsStatic | ||
| would not work in modules anymore. | ||
| ''; | ||
|
Comment on lines
+413
to
+417
Contributor
There was a problem hiding this comment.
I guess this assertion is intended to solve this, but it seems that the top-level assertion is being checked before the
Contributor
Author
There was a problem hiding this comment.
Yes!
Maybe... I could
Contributor
There was a problem hiding this comment. Maybe this would be better off being handled by option-type checking. E.g. create a custom That way the platform option definitions are checked by the module system and we can still normalise them to attrs to allow reading them if needed. This could be implemented using a # might be simpler to use `lib.types.addCheck`
check =
v:
if builtins.isString v then
true
else if builtins.isAttrs v then
# TODO: we could print the offending attr names here
[ ] == builtins.filter (n: builtins.elem n platformAttrsBlacklist) (builtins.attrNames v)
else
false;It could also be used to normalise the final value: # could either be done in the option's `apply`, or the type's `merge` function
apply = platform: builtins.removeAttrs (lib.systems.elaborate platform) platformAttrsBlacklist;
Contributor
Author
There was a problem hiding this comment. Your suggestion implies that it would be correct to "take a fully elaborated system, remove forbidden attributes, and pass it to the nixos/nixpkgs module". This is not the case, unfortunately. Some attributes are not incorrect per se, but can still be incorrectly passed on. |
||
| } | ||
| ]; | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| # run like this: | ||
| # nix-build pkgs/test/top-level/stage.nix | ||
| { | ||
| localSystem ? { | ||
| system = builtins.currentSystem; | ||
| }, | ||
| }: | ||
|
|
||
| with import ../../top-level { inherit localSystem; }; | ||
|
|
||
| let | ||
| # To silence platform specific evaluation errors | ||
| discardEvaluationErrors = e: (builtins.tryEval e).success -> e; | ||
|
|
||
| # Basic test for idempotency of the package set, i.e: | ||
| # Applying the same package set twice should work and | ||
| # not change anything. | ||
| isIdempotent = set: discardEvaluationErrors (pkgs.${set}.stdenv == pkgs.${set}.${set}.stdenv); | ||
|
|
||
| # Some package sets should be noops in certain circumstances. | ||
| # This is very similar to the idempotency test, but not going | ||
| # via the super' overlay. | ||
| isNoop = | ||
| parent: child: | ||
| discardEvaluationErrors ( | ||
| (lib.getAttrFromPath parent pkgs).stdenv == (lib.getAttrFromPath parent pkgs).${child}.stdenv | ||
| ); | ||
|
|
||
| allMuslExamples = builtins.attrNames ( | ||
| lib.filterAttrs (_: system: lib.hasSuffix "-musl" system.config) lib.systems.examples | ||
| ); | ||
|
|
||
| allLLVMExamples = builtins.attrNames ( | ||
| lib.filterAttrs (_: system: system.useLLVM or false) lib.systems.examples | ||
| ); | ||
|
|
||
| # A package set should only change specific configuration, but needs | ||
| # to keep all other configuration from previous layers in place. | ||
| # Each package set has one or more key characteristics for which we | ||
| # test here. Those should be kept, even when applying the "set" package | ||
| # set. | ||
| isComposable = | ||
| set: | ||
| ( | ||
| # Can't compose two different libcs... | ||
| builtins.elem set [ "pkgsLLVMLibc" ] | ||
| || discardEvaluationErrors ( | ||
| pkgsCross.mingwW64.${set}.stdenv.hostPlatform.config == "x86_64-w64-mingw32" | ||
| ) | ||
| ) | ||
| && ( | ||
| # Can't compose two different libcs... | ||
| builtins.elem set [ "pkgsLLVMLibc" ] | ||
| || discardEvaluationErrors (pkgsCross.mingwW64.${set}.stdenv.hostPlatform.libc == "msvcrt") | ||
| ) | ||
| && discardEvaluationErrors (pkgsCross.ppc64-musl.${set}.stdenv.hostPlatform.gcc.abi == "elfv2") | ||
| && discardEvaluationErrors ( | ||
| builtins.elem "trivialautovarinit" pkgs.pkgsExtraHardening.${set}.stdenv.cc.defaultHardeningFlags | ||
| ) | ||
| && discardEvaluationErrors (pkgs.pkgsLLVM.${set}.stdenv.hostPlatform.useLLVM) | ||
| && ( | ||
| # Can't compose two different libcs... | ||
| builtins.elem set [ | ||
| "pkgsMusl" | ||
| "pkgsStatic" | ||
| ] | ||
| || discardEvaluationErrors (pkgs.pkgsLLVMLibc.${set}.stdenv.hostPlatform.isLLVMLibc) | ||
| ) | ||
| && discardEvaluationErrors (pkgs.pkgsArocc.${set}.stdenv.hostPlatform.useArocc) | ||
| && discardEvaluationErrors (pkgs.pkgsZig.${set}.stdenv.hostPlatform.useZig) | ||
| && discardEvaluationErrors (pkgs.pkgsLinux.${set}.stdenv.buildPlatform.isLinux) | ||
| && ( | ||
| # Can't compose two different libcs... | ||
| builtins.elem set [ "pkgsLLVMLibc" ] | ||
| || discardEvaluationErrors (pkgs.pkgsMusl.${set}.stdenv.hostPlatform.isMusl) | ||
| ) | ||
| && discardEvaluationErrors (pkgs.pkgsStatic.${set}.stdenv.hostPlatform.isStatic) | ||
| && discardEvaluationErrors (pkgs.pkgsi686Linux.${set}.stdenv.hostPlatform.isx86_32) | ||
| && discardEvaluationErrors (pkgs.pkgsx86_64Darwin.${set}.stdenv.hostPlatform.isx86_64); | ||
| in | ||
|
|
||
| # Appends same defaultHardeningFlags again on each .pkgsExtraHardening - thus not idempotent. | ||
| # assert isIdempotent "pkgsExtraHardening"; | ||
| # TODO: Remove the isDarwin condition, which currently results in infinite recursion. | ||
| # Also see https://github.com/NixOS/nixpkgs/pull/330567#discussion_r1894653309 | ||
| assert (stdenv.hostPlatform.isDarwin || isIdempotent "pkgsLLVM"); | ||
| # TODO: This currently results in infinite recursion, even on Linux | ||
| # assert isIdempotent "pkgsLLVMLibc"; | ||
| assert isIdempotent "pkgsArocc"; | ||
| assert isIdempotent "pkgsZig"; | ||
| assert isIdempotent "pkgsLinux"; | ||
| assert isIdempotent "pkgsMusl"; | ||
| assert isIdempotent "pkgsStatic"; | ||
| assert isIdempotent "pkgsi686Linux"; | ||
| assert isIdempotent "pkgsx86_64Darwin"; | ||
|
|
||
| assert isNoop [ "pkgsStatic" ] "pkgsMusl"; | ||
| assert lib.all (sys: isNoop [ "pkgsCross" sys ] "pkgsMusl") allMuslExamples; | ||
| assert lib.all (sys: isNoop [ "pkgsCross" sys ] "pkgsLLVM") allLLVMExamples; | ||
|
|
||
| assert isComposable "pkgsExtraHardening"; | ||
| assert isComposable "pkgsLLVM"; | ||
| # TODO: Results in infinite recursion | ||
| # assert isComposable "pkgsLLVMLibc"; | ||
| assert isComposable "pkgsArocc"; | ||
| # TODO: unexpected argument 'bintools' - uncomment once https://github.com/NixOS/nixpkgs/pull/331011 is done | ||
| # assert isComposable "pkgsZig"; | ||
| assert isComposable "pkgsMusl"; | ||
| assert isComposable "pkgsStatic"; | ||
| assert isComposable "pkgsi686Linux"; | ||
|
|
||
| # Special cases regarding buildPlatform vs hostPlatform | ||
| assert discardEvaluationErrors (pkgsCross.gnu64.pkgsMusl.stdenv.hostPlatform.isMusl); | ||
| assert discardEvaluationErrors (pkgsCross.gnu64.pkgsi686Linux.stdenv.hostPlatform.isx86_32); | ||
| assert discardEvaluationErrors (pkgsCross.mingwW64.pkgsLinux.stdenv.hostPlatform.isLinux); | ||
| assert discardEvaluationErrors ( | ||
| pkgsCross.aarch64-darwin.pkgsx86_64Darwin.stdenv.hostPlatform.isx86_64 | ||
| ); | ||
|
|
||
| # pkgsCross should keep upper cross settings | ||
| assert discardEvaluationErrors ( | ||
| with pkgsStatic.pkgsCross.gnu64.stdenv.hostPlatform; isGnu && isStatic | ||
| ); | ||
| assert discardEvaluationErrors ( | ||
| with pkgsLLVM.pkgsCross.musl64.stdenv.hostPlatform; isMusl && useLLVM | ||
| ); | ||
|
|
||
| emptyFile |
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.
Please add this back (or something like it), so that the type of
config.nixpkgs.hostPlatformis always a platform attrset.Uh oh!
There was an error while loading. Please reload this page.
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.
So, if we do that, then we can't do any of this PR, essentially.
Citing from the commit message:
It seems the intent of the
apply = elaboratewas, that consumers could look atconfig.nixpkgs.hostPlatformand use it for downstream logic... but the comment cited above already says otherwise.Also, almost every consumer already depended on
stdenv.hostPlatforminstead - except for two tests. I think this makes more sense.What's your reasoning to not do it like this / what am I missing?