I don't like the denylist approach, because it implies that "parsed" per se is wrong, and other attributes might be fine. One could assume that "by removing parsed from an elaborated system, we're good to go". This is not the case.

"parsed" is just a symptom of an elaborated system. Anything that passes non-user-defined stuff forward is problematic.

I'd rather hardcode the parsed check below and not have this list.

"parsed" is just a symptom of an elaborated system. Anything that passes non-user-defined stuff forward is problematic.

Sure, but how do we define what is user-defined?

You're more familiar with this than me, so I'm probably missing some context.

My intention was that platformAttrsBlacklist should contain all attr names that should not be user-defined.

If there's a better approach, I'm certainly open to it.

Could we solve this by adding a _type to elaborated platforms?

That way we can check:

check =
  platform:
  if platform._type or null == "elaborated-platform" then
    # TODO: trace, warn, or throw
    false
  else
    true;

This is the odd thing about lib.systems.elaborate. Essentially all derived values should not be set by the user. But some of the attrs are sometimes derived and sometimes not.

Valid Example:

{
  config = "riscv64-none-elf";
  libc = "newlib";
}

Invalid Example:

{
  config = "powerpc64le-unknown-linux-musl";
  libc = "newlib";
}

This will be very hard to encode properly - essentially you'd be fixing systems.elaborate that way.

Sure, but how do we define what is user-defined?

There is some place where the user explicitly specifies their configuration for the system. They might run native and just have a single { system = "..."; } or so. They might try to run a system fully on musl, so maybe { config = "x86_64-unknown-linux-musl"; }. Or built via LLVM, or they might be cross-compiling something etc... - in all those cases they explicitly specify some values.

Then, two things happens to those values:

• they are passed on, e.g. from nixos to nixpkgs
• they might be extended by package set modifiers like pkgsStatic etc.

Everything that is either defined by the user directly or added via a package set like pkgsStatic is allowed. Everything that is only derived via systems.elaborate is not.

You can't hardcode this.

Let me phrase it otherwise: We can't guard against invalid values, which are passed to the nixos/nixpkgs module. We can only guess from the input that the user used a wrong code pattern (i.e. they passed an elaborated system), so we try to give them a hint, otherwise everything else breaks down.

Could we solve this by adding a _type to elaborated platforms?

Yes, this could work, I think.

Not sure whether that might have been what was meant by a previous comment saying this:

TODO make lib.systems itself use the module system.

@roberth did you add that comment?

TODO make lib.systems itself use the module system.

I believe that TODO was suggesting that the lib.systems functions could themselves use modules in order to produce the elaborated system. Similar to how nixpkgs's config uses the module system, for example.

That's unrelated to the suggestion of adding an _type.

See reasoning above, I don't think we can list "unsupported" attributes.

For example libc might sometimes be correct and sometimes be incorrect to pass - depending on the circumstances.

We should probably extend this to say something about using nixpkgs.pkgs.stdenv.xxxPlatform instead for reading.

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.

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

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.

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.

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.

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.

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.

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

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.

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!