nixpkgs module: Clean up platform options #38485

Ericson2314 · 2018-04-05T20:17:05Z

system is replaced with the more general localSystem
crossSystem's description mentions localSystem (and vice versa).

Motivation for this change

localSystem is the better name we want to use going forward. It's been around for quite a while, actually.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

Ericson2314 · 2018-04-05T20:17:43Z

nixos/modules/misc/nixpkgs.nix

-      default = null;
+    localSystem = mkOption {
+      type = types.attrs; # TODO utilize lib.systems.parsedPlatform
+      example = lib.systems.examples.aarch64-multiplatform;


This would be a fully elaborated platform, and quite large. I could put something minimal too.

Ericson2314 · 2018-04-06T00:27:05Z

nixos/modules/services/misc/dysnomia.nix

@@ -158,7 +158,7 @@ in

    dysnomia.properties = {
      hostname = config.networking.hostName;
-      system = if config.nixpkgs.system == "" then builtins.currentSystem else config.nixpkgs.system;
+      inherit (config.nixpkgs.localSystem) system;


Don't need fallback cause I added default one in module.

dezgeg · 2018-04-06T00:28:00Z

nixos/modules/misc/nixpkgs.nix


        Ignored when <code>nixpkgs.pkgs</code> is set.
      '';
    };

-    system = mkOption {


Renaming this breaks nixops.

Ericson2314 · 2018-04-06T00:28:35Z

nixos/lib/eval-config.nix

@@ -36,7 +36,7 @@ let
    _file = ./eval-config.nix;
    key = _file;
    config = {
-      nixpkgs.system = lib.mkDefault system_;
+      nixpkgs.localSystem = lib.mkDefault { inherit system; };


The ? builtins.currentSystem fallback above is no longer needed. At some future point I'll make this work with an // optionalAttrs (args ? system) { .. } instead.

Ericson2314 · 2018-04-06T01:40:03Z

I put system back as a deprecated parameter for nixops @dezgeg

Ericson2314 · 2018-04-06T14:47:35Z

N.B. local system was added in https://github.com/NixOS/nixpkgs/pull/22575/files, pre 17.09 I believe.

Ericson2314 · 2018-04-06T14:51:11Z

~~This passes, I'll merge it. If somebody wants me to remove the deprecation part pending further discussion I'll do it.~~

Oops didn't mean to right that even before @edolstra's comment.

edolstra · 2018-04-06T15:04:35Z

IMHO a system -> localSystem rename is unnecessary and creates a lot of pain for the vast majority of NixOS users who don't care about cross-compilation. Also, the name localSystem seems suboptimal in a distributed context (e.g. in a NixOps network, it suggests that it's the system type on which NixOps runs, while it's actually the type of the remote system).

Ericson2314 · 2018-04-06T15:13:29Z

@edolstra I've removed the deprecation part for a separate PR.

This isn't just for cross compilation. If you want to do one of

Specify alternative libc
Specify exact ARM version

or many other such things for native builds it is necessary too.

I choose localSystem without much feedback one way or another a year ago. It's been the name in nixpkgs for 3 releases but probably little used. I wouldn't mind changing it as long as that doesn't slow down what I hope is a deprecation of system.

Ericson2314 · 2018-04-06T15:22:11Z

@edolstra BTW it would be much conceptually better to make one setting always be the run platform, and make the other optional one the build platform for cross, than what we do here (one is always build, the optional one is run), but I didn't want to break the crossSystem interface. Maybe that was a mistake.

- `localSystem` is added, it strictly supercedes system - `crossSystem`'s description mentions `localSystem` (and vice versa). - No more weird special casing I don't even understand TEMP

dezgeg · 2018-04-06T16:49:05Z

This isn't just for cross compilation. If you want to do one of 1) Specify alternative libc 2)Specify exact ARM version

Renaming stuff is not at all necessary for doing that. Setting ARM version is already possible by doing nixpkgs.system = "armv6l-linux"; or nixpkgs.system = "armv7l-linux";. And fine-tuning the platform options was already possible a long ago like this:
https://github.com/NixOS/nixpkgs/blob/16.03/nixos/modules/installer/cd-dvd/system-tarball-fuloong2f.nix#L159

what I hope is a deprecation of system.

I consider this a bad idea.

Ericson2314 · 2018-04-06T16:58:55Z

OK my bad with the ARM example. But if we had this, would we have needed to implement that in Nix? It's not like we do the same for i686 i386 sse3 etc.

Ericson2314 · 2018-04-06T17:08:03Z

Also, concretely, there is more in https://github.com/NixOS/nixpkgs/blob/master/lib/systems/default.nix#L20-L46 here than system and platform. We need to allow specifying that stuff in NixOS somehow.

dezgeg · 2018-04-07T17:53:27Z

OK my bad with the ARM example. But if we had this, would we have needed to implement that in Nix?

Yes we would because the ARM version affects uname -m output and thus what builtins.currentSystem returns.

It's not like we do the same for i686 i386

Yes we do, actually. If you boot with an old enough processor, uname -m will return e.g. i586 which would mean builtins.currentSystem will start returning i586-linux. (So effectively, we don't support such ancient systems).

Also, concretely, there is more in https://github.com/NixOS/nixpkgs/blob/master/lib/systems/default.nix#L20-L46 here than system and platform. We need to allow specifying that stuff in NixOS somehow.

But most people don't have any need to change them from the defaults. The existing interface works just fine for them.

Ericson2314 · 2018-04-08T14:12:44Z

@dezgeg Ok fair point with uname, I didn't know that's what we were following. I'll go add those other x86 ones to the parser for consistency.

Say we leave system. "most people" may not care but some absolutely do so we still need to expose localSystem. What about top level platform? It's not even exposed on NixOS. Should it also continue to be exposed on nixpkgs or is it obscure enough that people can migrate to localSystem.platform? Finally if foo and localSystem.foo are both defined, who wins?

I guess I'll relent and abandon plans to get rid of system, but I'd really hope we can at least get rid of platform. Having too many ways to do things complicates code and leaves the more advanced way less discoverable.

dtzWill · 2018-04-19T18:52:43Z

It'd be nice if there was a sane way to set localSystem in a NixOS config. My immediate use-case is musl, "surprise!", but at a high-level it's strange to not be able to reasonably set Nixpkgs options from a NixOS configuration.

On the subject, at least my needs would be met by a mechanism to specify part of the configuration-- this has advantages when targeting multiple "systems", instead of what I have to do now: either hardcode x86_64-unknown-linux-musl or do something completely terrible like introducing a map of system -> localSystem.config. I'm not sure about other use cases.

Ericson2314 · 2018-04-19T19:00:39Z

Yeah this no longer deprecates anything, and we have until 18.09 to bike-shed the localSystem name / make other improvements.

dtzWill · 2018-04-20T18:25:54Z

This is great, thanks!

Few quick questions/comments now that I've started using it:

Setting localSystem.config = "x86_64-unknown-linux-musl" is not enough, it appears if you want to set config you need to set it in addition to specifying system. Since system can be 100% derived from config this was unexpected, although not a big deal. This becomes slightly more of a problem/mismatch when I realized this also meant that things like localSystem = lib.systems.examples.riscv64 (or any of the examples I think) doesn't work-- that is, elsewhere the unspecified attributes are "inferred" but not here. Is this a known difference, and regardless would it be much work to resolve it?
system is passed around various places--and a surprising number of places (particularly in NixOS tests) have code like:
import <nixpkgs> { inherit system; }. or otherwise do things that doesn't consider or allow specifying localSystem. For example build-vms.nix, as seen in this (kludge) commit where I bake in the localSystem.config to use
As-is the localSystem specified is ignored which is unfortunate since you need to have/build not just the packages for your NixOS system but for ones you don't use in order to test. If localSystem contains important values for correct execution this might break things in surprising ways. Also, it'd probably be good to minimize the number of times the root of a nixpkgs tree is imported?

I'm not sure what the best solution for this is, I'm not sure how reasonable it would be to try to plumb localSystem everywhere or what other options there might be. But just thought I'd share/report to get some discussion going :).

Thanks!

On a slightly related note: it'd be "nice" if there was a good way to invoke NixOS tests and other infrastructure with control over localSystem (or crossSystem!). Unfortunately this is probably a large task, especially for things like release.nix which wants to map over system--it'd need to be reworked and it might not work out to be a very smooth transition. Anyway just a thought :).

(it'd be great if Borg could --at least when asked-- do some musl testing, for example. Anyway...)

Ericson2314 · 2018-04-24T18:53:02Z

@dtzWill I fell you on all of those. The long story is I'm waiting to see how #37252 shakes out to better make lib.systems.elaborate and lib.systems.parse* meld with the module system. But I'm happy to take ideas for the short story.

Ericson2314 requested review from shlevy, dezgeg and nbp April 5, 2018 20:17

Ericson2314 commented Apr 5, 2018

View reviewed changes

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) labels Apr 5, 2018

shlevy approved these changes Apr 5, 2018

View reviewed changes

Ericson2314 force-pushed the nixos-nixpkgs-options branch 2 times, most recently from 917f0f3 to f942916 Compare April 6, 2018 00:06

GrahamcOfBorg added 8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Apr 6, 2018

Ericson2314 commented Apr 6, 2018

View reviewed changes

shlevy approved these changes Apr 6, 2018

View reviewed changes

dezgeg reviewed Apr 6, 2018

View reviewed changes

Ericson2314 commented Apr 6, 2018

View reviewed changes

Ericson2314 requested a review from dtzWill April 6, 2018 01:36

Ericson2314 force-pushed the nixos-nixpkgs-options branch from f942916 to bfa13a8 Compare April 6, 2018 01:37

Ericson2314 force-pushed the nixos-nixpkgs-options branch from bfa13a8 to 0debd0b Compare April 6, 2018 15:09

Ericson2314 force-pushed the nixos-nixpkgs-options branch from 0debd0b to 17a7eae Compare April 6, 2018 15:13

nixpkgs module: Clean up platform options

c6f7d43

- `localSystem` is added, it strictly supercedes system - `crossSystem`'s description mentions `localSystem` (and vice versa). - No more weird special casing I don't even understand TEMP

Ericson2314 force-pushed the nixos-nixpkgs-options branch from 17a7eae to c6f7d43 Compare April 6, 2018 16:42

Ericson2314 merged commit 53686e8 into NixOS:master Apr 19, 2018

Ericson2314 deleted the nixos-nixpkgs-options branch April 19, 2018 19:00

Ericson2314 mentioned this pull request Sep 7, 2018

Cross compilation changes have made undocumented, breaking changes to NixOS #46320

Closed

samueldr mentioned this pull request Sep 7, 2018

nixpkgs module: Fix defaulting of localSystem and system #46341

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixpkgs module: Clean up platform options #38485

nixpkgs module: Clean up platform options #38485

Ericson2314 commented Apr 5, 2018

Ericson2314 Apr 5, 2018

Ericson2314 Apr 6, 2018 •

edited

dezgeg Apr 6, 2018

Ericson2314 Apr 6, 2018

Ericson2314 commented Apr 6, 2018

Ericson2314 commented Apr 6, 2018

Ericson2314 commented Apr 6, 2018 •

edited

edolstra commented Apr 6, 2018

Ericson2314 commented Apr 6, 2018 •

edited

Ericson2314 commented Apr 6, 2018 •

edited

dezgeg commented Apr 6, 2018

Ericson2314 commented Apr 6, 2018 •

edited

Ericson2314 commented Apr 6, 2018

dezgeg commented Apr 7, 2018

Ericson2314 commented Apr 8, 2018

dtzWill commented Apr 19, 2018

Ericson2314 commented Apr 19, 2018

dtzWill commented Apr 20, 2018

Ericson2314 commented Apr 24, 2018 •

edited

nixpkgs module: Clean up platform options #38485

nixpkgs module: Clean up platform options #38485

Conversation

Ericson2314 commented Apr 5, 2018

Motivation for this change

Things done

Ericson2314 Apr 5, 2018

Choose a reason for hiding this comment

Ericson2314 Apr 6, 2018 • edited

Choose a reason for hiding this comment

dezgeg Apr 6, 2018

Choose a reason for hiding this comment

Ericson2314 Apr 6, 2018

Choose a reason for hiding this comment

Ericson2314 commented Apr 6, 2018

Ericson2314 commented Apr 6, 2018

Ericson2314 commented Apr 6, 2018 • edited

edolstra commented Apr 6, 2018

Ericson2314 commented Apr 6, 2018 • edited

Ericson2314 commented Apr 6, 2018 • edited

dezgeg commented Apr 6, 2018

Ericson2314 commented Apr 6, 2018 • edited

Ericson2314 commented Apr 6, 2018

dezgeg commented Apr 7, 2018

Ericson2314 commented Apr 8, 2018

dtzWill commented Apr 19, 2018

Ericson2314 commented Apr 19, 2018

dtzWill commented Apr 20, 2018

Ericson2314 commented Apr 24, 2018 • edited

Ericson2314 Apr 6, 2018 •

edited

Ericson2314 commented Apr 6, 2018 •

edited

Ericson2314 commented Apr 6, 2018 •

edited

Ericson2314 commented Apr 6, 2018 •

edited

Ericson2314 commented Apr 6, 2018 •

edited

Ericson2314 commented Apr 24, 2018 •

edited