Skip to content

nixos/nixpkgs: use type-checking to enforce buildPlatform & hostPlatform restrictions & restore hostPlatform == buildPlatform support #379411

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

Closed
MattSturgeon wants to merge 2 commits into from
Closed

nixos/nixpkgs: use type-checking to enforce buildPlatform & hostPlatform restrictions & restore hostPlatform == buildPlatform support #379411

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c7d1d4
nixos/nixpkgs: use type-checking to enforce buildPlatform & hostPlatf…
MattSturgeon Feb 4, 2025
56fed62
nixos/nixpkgs: restore `hostPlatform == buildPlatform` support
MattSturgeon Feb 4, 2025
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
63 changes: 43 additions & 20 deletions nixos/modules/misc/nixpkgs.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,32 @@ let
description = "An evaluation of Nixpkgs; the top level attribute set of packages";
};

# Additional check applied to platformType definitions, intended to guard
# against using elaborated platforms as option definitions.
# We guess whether a platform is elaborated by checking for a `parsed` attr.
checkPlatformDef =
platform:
if platform ? parsed then
builtins.trace
''
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.
In practice, this usually means you have assigned `pkgs.stdenv.hostPlatform` or `pkgs.stdenv.buildPlatform` to the `nixpkgs` platform options.
If so, you should instead assign `pkgs.stdenv.hostPlatform.system` or `pkgs.stdenv.buildPlatform.system`.''
false
else
true;

platformType =
lib.types.coercedTo lib.types.str (system: { inherit system; }) (
lib.types.addCheck lib.types.attrs checkPlatformDef
)
// {
name = "platform-input";
description = "platform argument, as accepted by lib.systems.elaborate";
descriptionClass = "nonRestrictiveClause";
};

hasBuildPlatform = opt.buildPlatform.highestPrio < (lib.mkOptionDefault { }).priority;
hasHostPlatform = opt.hostPlatform.isDefined;
hasPlatform = hasHostPlatform || hasBuildPlatform;
Expand All @@ -73,10 +99,7 @@ let
defaultPkgs =
if opt.hostPlatform.isDefined then
let
isCross =
!(lib.systems.equals (lib.systems.elaborate cfg.buildPlatform) (
lib.systems.elaborate cfg.hostPlatform
));
isCross = cfg.buildPlatform != cfg.hostPlatform;
systemArgs =
if isCross then
{
Expand Down Expand Up @@ -198,7 +221,7 @@ in
};

hostPlatform = lib.mkOption {
type = lib.types.either lib.types.str lib.types.attrs;
type = platformType;
example = {
system = "aarch64-linux";
};
Expand All @@ -213,13 +236,25 @@ in
};

buildPlatform = lib.mkOption {
type = lib.types.either lib.types.str lib.types.attrs;
type = platformType;
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.
apply =
buildPlatform:
let
elaborated = builtins.mapAttrs (_: lib.systems.elaborate) {
inherit (cfg) hostPlatform;
inherit buildPlatform;
};
in
# If equivalent to `hostPlatform`, make it actually identical so that `==` can be used
# See https://github.com/NixOS/nixpkgs/issues/278001
if lib.systems.equals elaborated.hostPlatform elaborated.buildPlatform then
cfg.hostPlatform
else
buildPlatform;
Comment on lines 244 to 257
Copy link
Contributor

@wolfgangwalther wolfgangwalther Feb 4, 2025

Choose a reason for hiding this comment

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

While technically this does not hurt, it does invite reading from config.nixpkgs.xxxPlatform, which I think should be discouraged... or maybe not? I don't really know

It just needs to be clear that only user input can be read here, nothing else.

Copy link
Contributor Author

@MattSturgeon MattSturgeon Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, so long as these options remain readable, this apply should remain.

If we prefer to make the options write-only, then we would have something like:

apply = _: throw "`nixpkgs.buildPlatform` should not be read. Use `pkgs.stdenv.buildPlatform` instead.";

Or we could silently apply pkgs.stdenv.*Platform here:

apply = _: pkgs.stdenv.buildPlatform;

Or we could do that, but lib.warn about it:

apply =
  _:
  lib.warn
    "`nixpkgs.buildPlatform` should not be read. Use `pkgs.stdenv.buildPlatform` instead."
    pkgs.stdenv.buildPlatform;

With any of these approaches, we would need to manually merge opt.buildPlatform's definitions for our internal usage, as per #376988 (comment).

Copy link
Contributor

@wolfgangwalther wolfgangwalther Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Or we could silently apply pkgs.stdenv.*Platform here:
[...]
With any of these approaches, we would need to manually merge opt.buildPlatform's definitions for our internal usage

Isn't that essentially what I tried in eec2100 ?

This didn't work out.

Copy link
Contributor Author

@MattSturgeon MattSturgeon Feb 4, 2025

Choose a reason for hiding this comment

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

Isn't that essentially what I tried in eec2100 ?

Very similar yes. The apply would be different, but we'd still need something like rawValue or manually merging the option defs to get the un-applied value.

This didn't work out.

What was the issue with that approach? I'm trying to work out if the same issues would apply to my suggestion here.

Copy link
Contributor

@wolfgangwalther wolfgangwalther Feb 5, 2025

Choose a reason for hiding this comment

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

For future reference, if anyone ever comes back to this PR.

Isn't that essentially what I tried in eec2100 ?

What was the issue with that approach?

It's not so much that the approach was wrong.. it just wasn't enough - and lead to my later change naturally.

To be able to follow through on my first try, you'd need to make sure that nobody passes elaborated systems into those options - otherwise it doesn't matter whether you apply or not, pass the rawValue on or not. If you don't add an assertion to guard against that, you will end up with really hard to debug, obscure errors in entirely different places, though.

Once you add asserts against passing elaborated systems.. you suddenly can't pass config.nixpkgs.hostPlatform etc. onwards. But this is not only done in the nixos/nixpkgs module itself - it's done by random consumers, too. So you can't really use an undocumented, internal interface (the one I added with rawValue) to do that. You need access to the original values - and they are there, if we don't apply. Which we don't really need to, for any reason.

Copy link
Contributor Author

@MattSturgeon MattSturgeon Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If it's helpful, I could implement this as a standalone PR without your wider changes?

That could either be merged before you re-apply your changes or it could be cherry-picked into your changes, as appropriate.

Otherwise, I can just review whatever you come up with.

Copy link
Contributor

@wolfgangwalther wolfgangwalther Feb 5, 2025

Choose a reason for hiding this comment

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

If you'd like to follow up on this, that would be great.

I won't push this myself for now, because as outlined in #376988 (comment)... I never really intended to work on the NixOS modules :D. I was dragged into this by accident.

The pkgs/top-level is on hold for now, because:

  • we need a deprecation phase for the nixos module (especially about passing elaborated systems)
  • we can't do the package set composability without having finished the deprecation phase

I would love to come back to this once the NixOS modules are ready for it - and if you can push this side, I'm more than happy to help you whereever I can.

Copy link
Contributor

@wolfgangwalther wolfgangwalther Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Use option-type's check to reject elaborated platform definitions

  2. Use option's apply to enforce write-only and/or return the actual pkgs.stdenv.hostPlatform

  3. Use rawValue and/or manual definition merging to bypass the apply internally

I think it would also be great to have some kind of readonly config.nixpkgs.<something>, which bundles all the system, localSystem, crossSystem, hostPlatform and buildPlatform inputs in one place. This can then be easily passed on instead of accessing rawValue for everything manually. This would make the linux-builder case discussed in #376988 (comment) much easier to handle (and there are quite a few more cases like that).

Copy link
Contributor Author

@MattSturgeon MattSturgeon Feb 5, 2025

Choose a reason for hiding this comment

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

If you'd like to follow up on this, that would be great.

[...]

I would love to come back to this once the NixOS modules are ready for it - and if you can push this side, I'm more than happy to help you whereever I can.

Awesome 😎

I think it would also be great to have some kind of readonly config.nixpkgs.<something>, which bundles all the system, localSystem, crossSystem, hostPlatform and buildPlatform inputs in one place. This can then be easily passed on instead of accessing rawValue for everything manually.

As outlined in #376988 (comment), I think this is better handled in pkgs/top-level rather than the nixpkgs module. The reason being when we want to "pass on" platform input, we usually want to do so for whichever pkgs is actually in use; not necessarily the one being configured by the nixpkgs module.

Perhaps internal un-elaborated options could be useful for avoiding the apply without hacks though, but still I'm uneasy about them being used externally to "pass on" the input values, for the reasons outlined above.

Copy link
Contributor

@wolfgangwalther wolfgangwalther Feb 5, 2025

Choose a reason for hiding this comment

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

when we want to "pass on" platform input, we usually want to do so for whichever pkgs is actually in use; not necessarily the one being configured by the nixpkgs module.

Yes, I start to see this, after you made multiple comments in various places all along the same line... ;)

I think this is better handled in pkgs/top-level rather than the nixpkgs module.

This is an interesting thought, which I will have to think about a bit more!

defaultText = lib.literalExpression ''config.nixpkgs.hostPlatform'';
description = ''
Specifies the platform on which NixOS should be built.
Expand Down Expand Up @@ -404,18 +439,6 @@ in
${lib.concatMapStringsSep "\n" (file: " - ${file}") opt.config.files}
'';
}
{
assertion =
(opt.hostPlatform.isDefined -> builtins.isAttrs cfg.buildPlatform -> !(cfg.buildPlatform ? parsed))
&& (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.
'';
}
];
};

Expand Down