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: Allow passing {host,build}Platform directly + Nixpkgs fn docs #324614

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

roberth
Copy link
Member

@roberth roberth commented Jul 4, 2024

Description of changes

  • Add hostPlatform and buildPlatform as parameters to the Nixpkgs function.
  • Document the Nixpkgs function.
  • Test Nixpkgs initialization

This new interface has already been applied successfully in NixOS. nixos-generate-hardware has been generating the "default" hostPlatform in hardware-configuration.nix for over a year now without much trouble, and with the benefit of not having to specify system (or similar) in nixosSystem anymore.
Furthermore, the hostPlatform option is always defined and reliably produces the host platform for users who don't use the legacy options. This is in contrast to nixpkgs.crossSystem, which is usually not even defined, and nixpkgs.localSystem which is usually the wrong platform to refer to in a config file.

Ideally we'd clean up the nixpkgs.{system,localSystem,crossSystem} options to reduce complexity and confusion. But the interface in Nixpkgs is still based on the old terminology and behavior. By introducing these parameters also in Nixpkgs, the users' experience with NixOS carries over to Nixpkgs as well.

Further simplifications in the code base are now possible, specifically

  • stage.nix and related files still work with {local,cross}System, and have logic like

    ${if stdenv.hostPlatform == stdenv.buildPlatform
      then "localSystem" else "crossSystem"} = <...>
    

    ... which is really just

    hostPlatform = <...>
    

    This can now be simplified by refactoring this code to work with {host,build}Platform variables instead.

  • NixOS can forward its platform options directly to its Nixpkgs call. This pays off when the *[sS]ystem options are removed.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@roberth roberth requested a review from Ericson2314 as a code owner July 4, 2024 16:50
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Jul 4, 2024
@@ -0,0 +1,147 @@
# Nixpkgs as a Function {#sec-nixpkgs-function}

Depending on how you use Nixpkgs, you may interact with it as a [Nix function](https://nix.dev/manual/nix/2.23/language/constructs.html#functions).
Copy link
Contributor

@Shawn8901 Shawn8901 Jul 4, 2024

Choose a reason for hiding this comment

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

Is it intended that this manual link directly targets a version whilst all others in this document target latest? That kind of looks like being unintentional on first view.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!
I've changed it to latest and gathered up the remaining URLs.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jul 4, 2024
@roberth roberth force-pushed the add-nixpkgs-function-platform-arguments-and-docs branch from de0c0b6 to d159fca Compare July 4, 2024 20:11
@Shawn8901
Copy link
Contributor

Shawn8901 commented Jul 5, 2024

I am not sure where the inconsistency comes from here directly, but possibly its something that could be addressed.

when defining the hostPlatform in the configuration one has to do like

nixpkgs.hostPlatform.system = "x86_64-linux";

But one can also be more specific like

nixpkgs.hostPlatform  = { system = "x86_64-linux"; gcc.arch = "x86-64-v3"; }; 

(edit: or is that no longer a thing? havent used it for a while?)

whilst we are here passing { hostPlatform = "x86_64-linux"; }, i find it absolut confusing that one time the hostPlatform is a complex object that has a system and on the other variant it is directly defining the system.

And we should possibly try to avoid to use the same "word" for different things/types to no increase the already confusing situation about system vs hostPlatform that get asked from time to time on some community platforms (which to set when and why).
To i highly appreciate that you try to work on that 🙂

Might it be reasonable to pass the same type that is used when defining it in the config itself?

@roberth
Copy link
Member Author

roberth commented Jul 5, 2024

A string is shorthand for a { system = theString; } platform attrset, that is correct.

Shorthand notations do add complexity, I agree, but most users the string notation is more than sufficient.
NixOS allows a string, so I feel like rejecting it here would cause at least as much confusion. Or is it just annoyance?
Ultimately it's a problem with lib.systems.elaborate then. It accepts mere strings, so we'd have to add our own check, or factor out the parts that don't convert a string to { system = theString; }.

Might it be reasonable to pass the same type that is used when defining it in the config itself?

What do you mean by "the config", the NixOS config? I think it already does that.

@Shawn8901
Copy link
Contributor

Shawn8901 commented Jul 5, 2024

What do you mean by "the config", the NixOS config? I think it already does that.

ah so nixpkgs.hostPlatform.system = "x86_64-linux"; and nixpkgs.hostPlatform = "x86_64-linux"; are equivalent, and i would also be able to pass hostPlatform.system = "x86_64-linux" instead the "short-handed" hostPlatform = "x86_64-linux" ?

If so i missunderstood it before (so that i can pass both), and i would be then fine i guess, tho maybe a hint that its the same in the docs would be nice (tho i am not sure if the as-function.md is the right place, possibly not, maybe a ref in the link list would be nice if there is any doc how *Platform is specified).

With the config i meant in either hardware-configuration.nix or configuration.nix, like how nixos-generate-config does generate.

, so I feel like rejecting it here would cause at least as much confusion.

it its the same, i totally agree that it should not be rejected when passing as argument to the function.

edit: okay i saw in the tests, that there is also a *Platform.system = "<some system>";, so i assume that it can both be used 🙂 Might have been better from my side to fully check the diff first and not stop after the markdown document. So i guess the point is clear and fine for me

@roberth
Copy link
Member Author

roberth commented Jul 5, 2024

Don't worry about it. Sometimes we don't know what we don't know. Can't overthink all the time.

…ystem

This new interface has already been applied successfully in NixOS.
`nixos-generate-hardware` has been generating the "default" hostPlatform
in `hardware-configuration.nix` for over a year now [without much trouble],
and with the benefit of not having to specify `system` (or similar) in
`nixosSystem` anymore.
Furthermore, the `hostPlatform` option is always defined and reliably
produces the host platform for users who don't use the legacy options.
This is in contrast to `nixpkgs.crossSystem`, which is usually not
even defined, and `nixpkgs.localSystem` which is usually the wrong
platform to refer to in a config file.

Ideally we'd clean up the `nixpkgs.{system,localSystem,crossSystem}`
options to reduce complexity and confusion. But the interface in
Nixpkgs is still based on the old terminology and behavior.
By introducing these parameters also in Nixpkgs, the users' experience
with NixOS carries over to Nixpkgs as well.

Further simplifications in the code base are now possible, specifically

- stage.nix and related files still work with {local,cross}System,
  and have logic like

      ${if stdenv.hostPlatform == stdenv.buildPlatform
        then "localSystem" else "crossSystem"} = <...>

  ... which is really just

      hostPlatform = <...>

  This can now be simplified by refactoring this code to work with
  {host,build}Platform variables instead.

- NixOS can forward its platform options directly to its Nixpkgs call.
  This pays off when the `*[sS]ystem` options are removed.

[without much trouble]: #237218
@roberth roberth force-pushed the add-nixpkgs-function-platform-arguments-and-docs branch from d8c80cb to 4fd048a Compare September 30, 2024 10:13
@roberth
Copy link
Member Author

roberth commented Sep 30, 2024

Trivial rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants