Skip to content

pkgs/top-level: make package sets composable (platform override approach)#380342

Closed
wolfgangwalther wants to merge 7 commits intoNixOS:masterfrom
wolfgangwalther:compose-package-sets-3
Closed

pkgs/top-level: make package sets composable (platform override approach)#380342
wolfgangwalther wants to merge 7 commits intoNixOS:masterfrom
wolfgangwalther:compose-package-sets-3

Conversation

@wolfgangwalther
Copy link
Contributor

@wolfgangwalther wolfgangwalther commented Feb 8, 2025

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 = platform and 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:

crossSystem =
let system = lib.systems.elaborate crossSystem0; in
if crossSystem0 == null || lib.systems.equals system localSystem
then localSystem
else system;

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.

  • Built on platform(s) from lib/systems/flake-systems.nix:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • armv6l-linux: attribute 'armv6l' missing (fails the same way before this PR)
    • armv7l-linux
    • i686-linux
    • mipsel-linux
    • aarch64-darwin
    • armv5tel-linux
    • powerpc64le-linux: attribute 'powerpc64le' missing (fails the same way before this PR)
    • riscv64-linux: attribute 'riscv64' missing (fails the same way before this PR)
    • x86_64-freebsd: infinite recursion encountered (fails the same way before this PR)
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Feb 8, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 8, 2025
@nix-owners nix-owners bot requested a review from infinisil February 8, 2025 12:33
@wolfgangwalther
Copy link
Contributor Author

I tested nix-build nixos/release-small.nix -A nixos.tests.containers-imperative, which lead to the first revert. Builds fine.

I also built userborn.tests successfully, which was the first failure mentioned after the reapply: #376988 (comment). The fix for the test has been reverted in the meantime, so we are starting from a clean slate here.

@PedroHLC since you mimicked the top-level/stage.nix code in https://github.com/chaotic-cx/nyx/blob/894d1db77131a4a449d1993c7ba314ee15dd4e36/shared/make-microarch.nix#L23-L31, you might want to have a look at af0075c13aa0d7d3e2c60bf80abd6afe9dbc1a00. Although, I think you might not even need any adjustments there. You will need to adjust to #380005, though - which is something you can already do with lib.systems.equals.

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?

@emilazy
Copy link
Member

emilazy commented Feb 8, 2025

(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.)

@PedroHLC
Copy link
Member

PedroHLC commented Feb 8, 2025

@wolfgangwalther if you think changes are required enough to comment them on the PR, please make them generic and append to doc/release-notes.

@wolfgangwalther
Copy link
Contributor Author

if you think changes are required enough to comment them on the PR, please make them generic and append to doc/release-notes.

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 added a commit to MattSturgeon/nixvim that referenced this pull request Feb 8, 2025
Copy link
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

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.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Feb 9, 2025
@MattSturgeon
Copy link
Contributor

MattSturgeon commented Feb 25, 2025

@roberth @emilazy friendly bump 🙂

…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.
@wolfgangwalther wolfgangwalther force-pushed the compose-package-sets-3 branch from 155c8c2 to 3d17de9 Compare April 1, 2025 19:00
@wolfgangwalther
Copy link
Contributor Author

rebased after the treewide nixfmt.

@emilazy what's your opinion on this and #380005?

@nix-owners nix-owners bot requested a review from hsjobeki April 1, 2025 19:01
};
}
// {
override = allArgs.override or (newArgs: elaborate (allArgs // newArgs));
Copy link
Contributor

@MattSturgeon MattSturgeon Apr 3, 2025

Choose a reason for hiding this comment

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

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"; } // newArgs

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 17, 2025
@Atry
Copy link
Contributor

Atry commented Apr 23, 2025

Currently pkgs.postgresqlPackages.override does not exist, as a result, there is no way for an overlay to add an PostgreSQL extension to postgresqlPackages.

Will this pull request solve the problem?

@wolfgangwalther
Copy link
Contributor Author

Currently pkgs.postgresqlPackages.override does not exist, as a result, there is no way for an overlay to add an PostgreSQL extension to postgresqlPackages.

Will this pull request solve the problem?

No, this is only about stuff like pkgsMusl, pkgsStatic, pkgsCross etc., but not about package sets like postgresqlPackages etc.

@Atry
Copy link
Contributor

Atry commented Apr 23, 2025

Why is postgresqlPackages not extensible, while other language packages are extensible?

@emilazy
Copy link
Member

emilazy commented Apr 24, 2025

Please open a separate issue or Discourse thread to discuss postgresqlPackages, which is unrelated to this PR.

@doronbehar
Copy link
Contributor

Hey all, just out of interest, would this PR help solve:

?

@RossComputerGuy
Copy link
Member

No, that's unrelated similar to postgres. This only is relevant to the attributes which start with pkgs.

@wolfgangwalther
Copy link
Contributor Author

Not going to pursue this anymore. Feel free to pick up my code eventually.

@wolfgangwalther wolfgangwalther deleted the compose-package-sets-3 branch December 10, 2025 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 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.

8 participants