pkgs/top-level: make package sets composable (platform override approach)#380342
pkgs/top-level: make package sets composable (platform override approach)#380342wolfgangwalther wants to merge 7 commits intoNixOS:masterfrom
Conversation
fecfd15 to
5922ec2
Compare
5922ec2 to
155c8c2
Compare
|
I tested I also built @PedroHLC since you mimicked the It would be good to double check that this PR works fine for nix-darwin and nixvim (although there is no reason at all to believe it shouldn't). @MattSturgeon can you confirm? |
|
(I plan to comment on this and #380005 in the next few days. Sorry for the placeholder comment, figured it’s better than saying nothing at the tail end of my break! Please ping if I don’t comment again soon; I’ll have a fair amount to catch up on.) |
|
@wolfgangwalther if you think changes are required enough to comment them on the PR, please make them generic and append to |
I think there are no changes required for you downstream caused by this PR. I only pinged you explicitly, because you wanted to keep an eye. The other PR already has release notes. |
MattSturgeon
left a comment
There was a problem hiding this comment.
I can't sign this off, as much of it is beyond my understanding. But the bits I do understand look good to me.
It would be good to double check that this PR works fine for nix-darwin and nixvim (although there is no reason at all to believe it shouldn't). @MattSturgeon can you confirm?
I agree, I don't see any reason why this would affect downstream users.
Just to be safe, I ran some tests in nix-community/nixvim#2993. When backporting this PR to a nixpkgs rev that passes nixvim's test suite, this PR also passes nixvim's test suite.
I tested this both with and without the previously recommended changes to the nixpkgs module.
Also: some of nixvim's test suite tests integration with nix-darwin and home-manager, so to a (very) limited extend this also covers those projects.
…e bottom No change, just move appendOverlays and extend to the bottom, since they will be changed much less often. This makes it easier to compare the other package sets side-by-side.
Sharing a first piece of common code between all package sets makes it easier to maintain and less likely to introduce a new package set without this.
This adds some basic tests to compose package sets. The cases that are currently broken are commented out, they include things like: - pkgsStatic.pkgsMusl losing the isStatic flag - pkgsCross.ppc64-musl.pkgsMusl losing the gcc.abi setting - pkgsCross.mingwW64.pkgsStatic losing the config string - pkgsLLVM.pkgsMusl losing the useLLVM flag - pkgsLLVM.pkgsStatic losing the useLLVM flag - pkgsLLVM.pkgsi686Linux losing the useLLVM flag And probably more.
The various pkgsXYZ top-level package sets did not pass localSystem / crossSystem to lower levels, so far. This change propagates original arguments to lower levels, which include the overrides made by an upper package sets. There is an extensive test-suite to test various combinations of package sets in pkgs/test/top-level. There are a few basic promises made: - Package sets must be idempotent. pkgsMusl.pkgsMusl === pkgsMusl. - Once pkgsCross is used any subsequent package sets should affect the **host platform** and not the build platform. Examples: - pkgsMusl.pkgsCross.aarch64-multiplatform is a cross compilation from musl to glibc/aarch64 - pkgsCross.aarch64-multiplatform.pkgsMusl is a cross compilation to musl/aarch64 - Modifications from an earlier layer should not be lost, unless explicitly overwritten. Examples: - pkgsStatic.pkgsMusl should still be static. - pkgsStatic.pkgsCross.gnu64 should be static, but with glibc instead of musl. Exceptions / TODOs: - pkgsExtraHardening is currently not idempotent, because it applies the same flags over and over again. Resolves NixOS#136549 Resolves NixOS#114510 Resolves NixOS#212494 Resolves NixOS#281596
When using pkgsCross with a system that ends up the same as the localSystem, then modifications for package sets like pksgMusl need to be done for **both** localSystem and crossSystem. Consider the following on x86_64-linux: pkgsCross.gnu64.pkgsMusl Before this change, this would result in a musl buildPlatform, but a gnu hostPlatform. This breaks the promise of "stacking" package sets on top of each other. After this change, it results in a musl buildPlatform and a musl hostPlatform. This works better. One could expect this to result in the same as pkgsCross.musl64, i.e. a gnu buildPlatform and a musl hostPlatform, however I couldn't get this to work without increasing memory usage for ci/eval by many, many GB. This is caused by usage of pkgsi686Linux inside the main package set, which follows the same hybrid pattern.
155c8c2 to
3d17de9
Compare
| }; | ||
| } | ||
| // { | ||
| override = allArgs.override or (newArgs: elaborate (allArgs // newArgs)); |
There was a problem hiding this comment.
Does this work as intended when chained?
E.g.
(
(
lib.systems.elaborate "x86_64-linux"
).override { foo = true; }
).override { bar = true; }Using lib.pipe
lib.pipe "x86_64-linux" [
lib.systems.elaborate
(p: p.override { foo = true; })
(p: p.override { bar = true; })
]In the above example, the "original" allArgs is { system = "x86_64-linux"; }, so:
override = newArgs: { system = "x86_64-linux"; } // newArgsThe first time .override is used, I belive you'd get { system = "x86_64-linux"; } // { foo = true; },
while the second time I think you'd get { system = "x86_64-linux"; } // { bar = true; }.
Notice that the foo attr is lost on subsequent uses of .override. If .override was used a third time, bar would also be missing.
EDIT: actually, I think I oversimplified this in my head. Since override usually isn't part of allArgs, I believe it should chain correctly. At lease, up until the point when an elaborated system is supplied to override.
I think I'll need to experiment in the repl to verify my thought process here.
There was a problem hiding this comment.
EDIT: actually, I think I oversimplified this in my head. Since
overrideusually isn't part ofallArgs, I believe it should chain correctly. At lease, up until the point when an elaborated system is supplied tooverride.
I think I'll need to experiment in the repl to verify my thought process here.
I'd think so, too - because I have quite a few tests chaining things (which is the whole point of this PR!), so I assume it does work correctly.
|
Currently Will this pull request solve the problem? |
No, this is only about stuff like |
|
Why is |
|
Please open a separate issue or Discourse thread to discuss |
|
Hey all, just out of interest, would this PR help solve: ? |
|
No, that's unrelated similar to postgres. This only is relevant to the attributes which start with |
|
Not going to pursue this anymore. Feel free to pick up my code eventually. |
Same goal as in #303849 and #376988, which were reverted because of breaking changes to NixOS. This time with a new approach, based on the idea in #376988 (comment).
This makes the changes in the main commit 1f9012beb7caf8bfef77b8b3136a8dd58755e609 embarrassingly simple.
@MattSturgeon I took the
_type = platformand the override things we discussed in #380005 in here. Technically, this makes the two PRs independent now: This PR here should already work without yours merged, relying on:nixpkgs/pkgs/top-level/default.nix
Lines 96 to 100 in 354b12d
However, by doing this, we commit to "platforms can not be compared with equality", so we should still continue with #380005.
Things done
The tests can be run with
nix-build pkgs/test/top-level/stage.nix.lib/systems/flake-systems.nix:attribute 'armv6l' missing(fails the same way before this PR)attribute 'powerpc64le' missing(fails the same way before this PR)attribute 'riscv64' missing(fails the same way before this PR)infinite recursion encountered(fails the same way before this PR)Add a 👍 reaction to pull requests you find important.