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.

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

pkgs/top-level: make package sets composable #303849

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
2 changes: 2 additions & 0 deletions lib/modules.nix
Original file line number Diff line number Diff line change
Expand Up @@ -844,6 +844,8 @@ let

in warnDeprecation opt //
{ value = addErrorContext "while evaluating the option `${showOption loc}':" value;
# raw value before "apply" above
rawValue = addErrorContext "while evaluating the option `${showOption loc}':" res.mergedValue;
inherit (res.defsFinal') highestPrio;
definitions = map (def: def.value) res.defsFinal;
files = map (def: def.file) res.defsFinal;
Expand Down
15 changes: 10 additions & 5 deletions nixos/modules/misc/nixpkgs.nix
Original file line number Diff line number Diff line change
Expand Up @@ -70,19 +70,24 @@ let
++ lib.optional (opt.localSystem.highestPrio < (lib.mkOptionDefault { }).priority) opt.localSystem
++ lib.optional (opt.crossSystem.highestPrio < (lib.mkOptionDefault { }).priority) opt.crossSystem;

# pkgs/top-level/default.nix takes great strides to pass the *original* localSystem/crossSystem args
# on to nixpkgsFun to create package sets like pkgsStatic, pkgsMusl. This is to be able to infer default
# values again. Since cfg.xxxPlatform and cfg.xxxSystem are elaborated via apply, those can't be passed
# directly. Instead we use the rawValue before the apply/elaboration step, via opt.xxx.rawValue.
defaultPkgs =
if opt.hostPlatform.isDefined then
let
# This compares elaborated systems on purpose, **not** using rawValue.
isCross = cfg.buildPlatform != cfg.hostPlatform;
systemArgs =
if isCross then
{
localSystem = cfg.buildPlatform;
crossSystem = cfg.hostPlatform;
localSystem = opt.buildPlatform.rawValue;
crossSystem = opt.hostPlatform.rawValue;
}
else
{
localSystem = cfg.hostPlatform;
localSystem = opt.hostPlatform.rawValue;
};
in
import ../../.. (
Expand All @@ -96,9 +101,9 @@ let
inherit (cfg)
config
overlays
localSystem
crossSystem
;
localSystem = opt.localSystem.rawValue;
crossSystem = opt.crossSystem.rawValue;
};

finalPkgs = if opt.pkgs.isDefined then cfg.pkgs.appendOverlays cfg.overlays else defaultPkgs;
Expand Down
104 changes: 104 additions & 0 deletions pkgs/test/top-level/stage.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
# 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;
Copy link
Member

Choose a reason for hiding this comment

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

Extensive use of this makes the test less effective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without those, I have errors like this quickly:

error: x86_64 Darwin package set can only be used on Darwin systems.

Those are the errors thrown in pkgs/top-level/stage.nix. Any idea how I could target those more specifically - without replicating the logic around those into the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went through the code again and I think discardEvaluationErrors does not necessarily make the test less effective. In fact it makes it more specific. The calls to this function are placed only around conditionals, so the evaluation errors can only come from evaluating those package sets.

These tests are not meant to test the evaluation of each package set, but to test the composability of them, i.e. they are supposed to test the code in stage.nix, not further downstream. If stdenv for a certain package set is broken, this is not the place to test it, imho.


# 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);
Copy link
Member

Choose a reason for hiding this comment

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

stdenv is a derivation, and == on derivations is limited to outPath.
So this would be equivalent and avoid confusion:

Suggested change
isIdempotent = set: discardEvaluationErrors (pkgs.${set}.stdenv == pkgs.${set}.${set}.stdenv);
isIdempotent = set: discardEvaluationErrors (pkgs.${set}.stdenv.outPath == pkgs.${set}.${set}.stdenv.outPath);

However, I think you do want to check more:

Suggested change
isIdempotent = set: discardEvaluationErrors (pkgs.${set}.stdenv == pkgs.${set}.${set}.stdenv);
isIdempotent = set: discardEvaluationErrors (pkgs.${set}.stdenv.outPath == pkgs.${set}.${set}.stdenv.outPath
&& pkgs.${set}.stdenv.hostPlatform == pkgs.${set}.${set}.stdenv.hostPlatform
&& pkgs.${set}.stdenv.buildPlatform == pkgs.${set}.${set}.stdenv.buildPlatform
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm.. according to the test failures I got during development, I assumed the outPath changes when host or build platform change. Is that not correct?

I tried adding the hostPlatform & buildPlatform checks. Instead of ==, I had to use lib.systems.equals. I didn't get any test failures this way. This doesn't mean I don't need those changes, but do you have any example where a change to hostPlatform / buildPlatform would not affect stdenv's outPath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to support the idea that those platforms should affect stdenv's outpath:

inherit buildPlatform hostPlatform targetPlatform;

Copy link
Contributor Author

@wolfgangwalther wolfgangwalther Jan 11, 2025

Choose a reason for hiding this comment

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

For now, I made the .outPath comparison explicit, but didn't add the platform checks, yet.

Edit: I reverted the change to use outPath, because nixfmt then requires it to be split across two lines awkwardly. This'd be a net-negative for readability.


# 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:
discardEvaluationErrors (
pkgsCross.mingwW64.${set}.stdenv.hostPlatform.config == "x86_64-w64-mingw32"
)
&& 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)
&& discardEvaluationErrors (pkgs.pkgsArocc.${set}.stdenv.hostPlatform.useArocc)
&& discardEvaluationErrors (pkgs.pkgsZig.${set}.stdenv.hostPlatform.useZig)
&& discardEvaluationErrors (pkgs.pkgsLinux.${set}.stdenv.buildPlatform.isLinux)
&& 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");
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";
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
18 changes: 7 additions & 11 deletions pkgs/top-level/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -122,22 +122,18 @@ in let
config = lib.showWarnings configEval.config.warnings configEval.config;

# A few packages make a new package set to draw their dependencies from.
# (Currently to get a cross tool chain, or forced-i686 package.) Rather than
# give `all-packages.nix` all the arguments to this function, even ones that
# don't concern it, we give it this function to "re-call" nixpkgs, inheriting
# whatever arguments it doesn't explicitly provide. This way,
# `all-packages.nix` doesn't know more than it needs too.
# Rather than give `all-packages.nix` all the arguments to this function,
# even ones that don't concern it, we give it this function to "re-call"
# nixpkgs, inheriting whatever arguments it doesn't explicitly provide. This
# way, `all-packages.nix` doesn't know more than it needs to.
#
# It's OK that `args` doesn't include default arguments from this file:
# they'll be deterministically inferred. In fact we must *not* include them,
# because it's important that if some parameter which affects the default is
# substituted with a different argument, the default is re-inferred.
#
# To put this in concrete terms, this function is basically just used today to
# use package for a different platform for the current platform (namely cross
# compiling toolchains and 32-bit packages on x86_64). In both those cases we
# want the provided non-native `localSystem` argument to affect the stdenv
# chosen.
# To put this in concrete terms, we want the provided non-native `localSystem`
# and `crossSystem` arguments to affect the stdenv chosen.
#
# NB!!! This thing gets its `config` argument from `args`, i.e. it's actually
# `config0`. It is important to keep it to `config0` format (as opposed to the
Expand All @@ -146,7 +142,7 @@ in let
# via `evalModules` is not idempotent. In other words, if you add `config` to
# `newArgs`, expect strange very hard to debug errors! (Yes, I'm speaking from
# experience here.)
nixpkgsFun = newArgs: import ./. (args // newArgs);
nixpkgsFun = f0: import ./. (args // f0 args);

# Partially apply some arguments for building bootstraping stage pkgs
# sets. Only apply arguments which no stdenv would want to override.
Expand Down
Loading