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

nixpkgs.{system,localSystem,crossSystem} *values* are useless #49765

Open
roberth opened this issue Nov 4, 2018 · 20 comments
Open

nixpkgs.{system,localSystem,crossSystem} *values* are useless #49765

roberth opened this issue Nov 4, 2018 · 20 comments

Comments

@roberth
Copy link
Member

roberth commented Nov 4, 2018

Issue description

Of course these options serve a purpose inside the nixpkgs.nix module, to set pkgs. However, these options tend to be used elsewhere, even though they are hard to use, leading to problems with cross compiled NixOS.

For this reason, I suggest that we make these config values unusable by providing an appropriate error message showing what else to do.

Why aren't they useful

system is a legacy option that will now tend to be set to its default, builtins.currentSystem.

localSystem will only consistently represent the system where stuff was built. This is typically not useful information, yet it is used erroneously in some modules.

crossSystem is meaningful, but is not always set.

What can we do

Maybe system can be fixed by inspecting options.nixpkgs.localSystem and options.nixpkgs.crossSystem. However, it is hard to reason about all possible uses of the module system and this feels like laying down a minefield for further changes to the nixpkgs.nix module.

A better solution seems to be to just treat these options as write-only. Users will set them and no other module than nixpkgs.nix can use its value. We can then, by means of an error message, direct module authors to the right value. This should probably be pkgs.targetPlatform. I don't think NixOS needs to provide its own way to determine its platform. pkgs can do that for NixOS just fine.

How to implement

Simply set all three options to private...

The module system does not yet have that option option, but it's almost there in #49766

Alternatives

Overhaul the nixpkgs options once more

So we ask people to set

  • nothing at all to use builtins.currentSystem
  • nixpkgs.targetSystem to set the system of the resulting NixOS
  • nixpkgs.hostSystem to set the system to build it on

Boils down to the same thing of course, but now we have a nixpkgs.targetSystem option that is actually useful.

I don't know how the old nixpkgs.system fits into this.
This means we'll need to reevaluate all the corner cases once more.
It will probably still confuse people and cause them to use nixpkgs.hostSystem which will then supposedly fail as it should by being null most of the time.

roberth referenced this issue Nov 4, 2018
Take two of #40708 (4fe2898).

That PR attempted to bidirectionally default `config.nixpkgs.system` and
`config.nixpkgs.localSystem.system` to each be updated by the other. But
this is not possible with the way the module system works. Divergence in
certain cases in inevitable.

This PR is more conservative and just has `system` default `localSystem`
and `localSystem` make the final call as-is. This solves a number of
issues.

 - `localSystem` completely overrides `system`, just like with nixpkgs
 proper. There is no need to specify `localSystem.system` to clobber the
 old system.

 - `config.nixpkgs.localSystem` is exactly what is passed to nixpkgs. No
 spooky steps.

 - `config.nixpkgs.localSystem` is elaborated just as nixpkgs would so
 that all attributes are available, not just the ones the user
 specified.

The remaining issue is just that `config.nixpkgs.system` doesn't update
based on `config.nixpkgs.localSystem.system`. It should never be
referred to lest it is a bogus stale value because
`config.nixpkgs.localSystem` overwrites it.

Fixes #46320
@roberth roberth changed the title nixpkgs.{system,localSystem,crossSystem} values are useless nixpkgs.{system,localSystem,crossSystem} *values* are useless Nov 4, 2018
@matthewbauer
Copy link
Member

matthewbauer commented Nov 4, 2018

I think the best move is to just replace theae usages with pkgs.buildPlatform and pkgs.hostPlatform. It would be niceto have a way to forbid these usages but i don’t know if it’s that useful for any other options.

@coretemp
Copy link
Contributor

coretemp commented Nov 5, 2018

I think the amount of time invested in the development of this feature (cross-compilation) along with the confusion it has created related to the documentation that comes with it is too high myself.

If you invest a few hundred hours in a feature, documenting it for at least 5 hours seems appropriate to me.

@roberth
Copy link
Member Author

roberth commented Nov 5, 2018

Hey coretemp. I too had to look up how to use the cross compilation and I understand your frustration. In particular I find the documentation for the distinction between a system string and { system : string, config : ? } and such lacking.

That said I do appreciate the amount of effort that it takes to work on a such a complicated project. I have been working on some improving some of the glue, like adding pkgs.nixos, and I find that the documentation is a significant part of the work. I imagine that the cross compilation is similar in this regard, except much much more involved. Because of its size, I understand that documentation has suffered, but I absolutely agree that it's not done until the documentation is in a good shape.

I am glad to see that you filed an issue, #49510. If there's anything else you think must be improved, please file another issue.

@Ericson2314
Copy link
Member

Ericson2314 commented Nov 5, 2018

@roberth yes the crossSystem interface sucks for precisely the reason that there is no easy consistent way to get the build-time and run-time platforms.

@coretemp I did not invent such a thing. crossSystem already existed for years. I kept it around to not break existing documentation (at the time, the old wiki). I regret doing this; that documentation is little read now anyways.

Also, keep in mind that most of the work gone into cross compilation by me at least was reading undocumented interfaces, and then consolidating/deduplicating them. So we went from one internally inconsistent state with very little documentation to one mostly internally consistent state with some more documentation. Some things I kind of only want to document thoroughly once they are in a good enough state to not spill copious amounts of ink on a shitty interface I want flexibility to fix.


All that said, I think the solution is to change the arguments to Nixpkgs itself (with some sort of back compat provision) and then change the NixOS interface accordingly.

@Ericson2314
Copy link
Member

@matthewbauer I recall using pkgs.anything could cause infinite recursion problems maybe? That's why a proper fix would definitely be preferred.

@matthewbauer
Copy link
Member

It shouldn't in NixOS modules unless something very weird is going on. You have to use the final "pkgs" - not config.nipkgs.pkgs though.

I've found a couple usages of localSystem but it seems pretty rare (dysnomia and containers). Were there any others?

@Ericson2314
Copy link
Member

You have to use the final "pkgs" - not config.nipkgs.pkgs though.

Oh, ok. Well that would be great to use pkgs everywhere and fix remove system from https://github.com/NixOS/nixpkgs/blob/master/nixos/lib/eval-config.nix#L11-L12

@coretemp
Copy link
Contributor

coretemp commented Nov 6, 2018

@Ericson2314 OK, thank you for that clarification.

@stale
Copy link

stale bot commented Jun 3, 2020

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 3, 2020
@xaverdh
Copy link
Contributor

xaverdh commented Sep 2, 2020

We still pass around system in eval-config, so I think this is not stale..

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 2, 2020
@xaverdh
Copy link
Contributor

xaverdh commented Oct 23, 2020

just dropping a thought that was floating around in my brain:
nixpkgs.targetSystem, which is currently nixpkgs.crossSystem.system should probably be set by nixos-generate-config in hardware-configurationx.nix since its a static property of the target system.

@stale
Copy link

stale bot commented Jun 7, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 7, 2021
@alarsyo
Copy link
Contributor

alarsyo commented Jul 14, 2021

Still important

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 14, 2021
@stale
Copy link

stale bot commented May 2, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 2, 2022
@roberth roberth removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 2, 2022
@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/enabling-march-and-mtune-for-kernel-builds/20395/1

@roberth
Copy link
Member Author

roberth commented Jul 17, 2022

just dropping a thought that was floating around in my brain: nixpkgs.targetSystem, which is currently nixpkgs.crossSystem.system should probably be set by nixos-generate-config in hardware-configurationx.nix since its a static property of the target system.

Done in

while the system parameter becomes optional in favor of the well-defined in-module variables in

This should become the new way of doing things in NixOS, but I'm taking a very conservative approach so as to validate the solution and to avoid any unnecessary disruption.

Nixpkgs would also benefit from such a change, allowing buildPlatform and hostPlatform to be set directly, although there the pain isn't felt as much because all consumers already have access to well-defined variables to read from. It's just the nixpkgs invocation that's a little odd.

@arianvp
Copy link
Member

arianvp commented May 17, 2024

I'm kind of confused why we started using hostPlatform/buildPlatform names in NixOS whilst nixpkgs itself still uses localSystem/crossSystem as the names. This really tripped me up. Which we deprecated system but kept the localSystem,crossSystem names.

@roberth
Copy link
Member Author

roberth commented May 17, 2024

Fwiw, system was soft-deprecated before I soft-deprecated localSystem/crossSystem (so I'm not actually 100% sure I understand your comment).

Nonetheless your criticism applies. In retrospect, I should have started with Nixpkgs' parameters first, and make the NixOS change after.
I don't have the capacity to work on this now, but feel free to apply similar changes to the Nixpkgs entrypoints.

@arianvp
Copy link
Member

arianvp commented May 17, 2024

Thanks for the context! I think i'll add a note in the code just to clarify!

@roberth
Copy link
Member Author

roberth commented Sep 30, 2024

Ready for review:

This lets us further diminish the role of these mediocre variables and nudges users towards the consistently available ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants