Platform normalization #24610

Ericson2314 · 2017-04-04T00:08:39Z

Motivation for this change

Finally deal with the awkward friction between {build,host,target}Platform.system and {build,host,target}Platform.config. This is a followup to #22575 and fixes #24170 .

@nbp this uses your old system code and tests. I think I've used the module type system idiomatically, but I am unsure about the "CPU families" bit. In lieu of real sets, I used a map [why do we say attribute set"...], but perhaps matchAttrs should be mutually recursive with a matchLists that treats the list as a set (or looks for sublists].

~~@nbp Also, I broke the existing module system test infra with some mysterious call package exception. Not sure what's up with that, but want to get this up for review.~~ Fixed.

This can be reviewed or even merged commit-by-commit, but watch out: github is putting the commits out or order again (argh!).

CC @wizeman

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

mention-bot · 2017-04-04T00:08:41Z

@Ericson2314, thanks for your PR! By analyzing the history of the files in this pull request, we identified @viric, @dezgeg and @nbp to be potential reviewers.

Mic92 · 2017-04-04T11:12:03Z

pkgs/top-level/release-lib.nix

+  assertTrue = bool:
+    if bool
+    then pkgs.runCommand "evaluated-to-true" {} "true"
+    then pkgs.runCommand "evaluated-to-false" {} "false";


this looks like an syntax error.

ok this was fixed later.

Well, I want each commit to stand on its own so that should be correct from the start!

Fixed from the start!

Mic92 · 2017-04-04T11:15:17Z

lib/tests/systems.nix

+{
+  all = assertTrue (mseteq all (linux ++ darwin ++ cygwin ++ freebsd ++ openbsd ++ netbsd ++ illumos));
+
+  arm = assertTrue (mseteq arm [ "armv5tel-linux" "armv6l-linux" "armv7l-linux" "aarch64-linux" ]);


I think there was a commit, which removed explicitly aarch64 from arm, because the cpu architecture is different.

This was a241abf

Just as I include x86_64 in the x86 family, so I still include aarch64 in the arm one. But the arm list there also constrains the word width to 32 bits---that should do the trick.

shlevy · 2017-04-04T12:21:26Z

Just quickly glanced through this, why are we using the module system type system here? This seems like a really complex change and it's not clear to me why we're doing it this way.

Ericson2314 · 2017-04-04T16:33:01Z

@shlevy Well, on some level, I used it because the code that already existed by @nbp and wanting to learn more about it :).

But, assuming we indeed do want to have a white list of known components rather than plain strings, I don't actually think this could be simplified much.

Ericson2314 · 2017-04-04T16:51:59Z

Ok fixed the module tests (mistake in 2nd commit). Removing the "WIP" as this thing is no longer obviously broken, but it needs more review for style / objectives before merging.

Ericson2314 · 2017-04-11T14:36:57Z

@nbp You have time to review this? Besides use of the module system, it would be good to figure out some plan for all the crazy edge cases when converting between LLVM triples and Nix "doubles" (e.g. stuff like darwin10 for OS, windows in general).

edolstra · 2017-04-11T15:30:03Z

pkgs/top-level/release-lib.nix

+  assertTrue = bool:
+    if bool
+    then pkgs.runCommand "evaluated-to-true" {} "touch $out"
+    else pkgs.runCommand "evaluated-to-false" {} "false";


Why is this? Why not just use assert?

Basically a workaround around hydra not tracking evaluation errors per revisions. I'd like this to count towards the "test failed vs test pass" numbers like everything else.

IMHO such tests are better done at build time (i.e. by having a single job that does nix-instantiate --eval tests.nix, probably by adding it to lib/tests/release.nix). I don't want to clutter Nixpkgs/NixOS jobsets with lots of tiny Nixpkgs lib tests...

This is basically what the module tests are doing, they basically spawn multiple Nix expressions tests within one job which contains all module tests.

I feel like recursive nix is a very heavy-handled solution here. If we don't want sub jobs so flattened, hydra should change the way it treats sub jobs.

Note that this is not what I would call recursive nix, as the module system does not evaluate any derivation, it only use the interpreter with nix-instanciate command, and check for the output of the command.

@Ericson2314 so what is easier, an extra job to test these, or changing Hydra so it tracks evaluation errors by revision?

Hmm? In a later PR I rewrote the tests the semi-recursive-Nix way.

edolstra · 2017-04-11T15:30:41Z

lib/systems/parse.nix

+                         , abi    ? assert false; null # ditto
+                         } @ args: let
+    getCpu = name:
+      attrByPath [name] (throw "Unknow cpuType `${name}'.")


Unknow -> Unknown. Also don't use backticks as quote characters please.

Thanks, fixed.

@edolstra the backtick-quote used to be the way names were reported by Nix tools.

edolstra · 2017-04-11T15:37:35Z

lib/trivial.nix

@@ -108,6 +108,9 @@ rec {
  # Flip the order of the arguments of a binary function.
  flip = f: a: b: f b a;

+  # Apply function if argument is non-null
+  mapNullable = f: a: if isNull a then a else f a;


Functions used in only one place shouldn't be added to lib.

I can do that, but I've wanted this many times. I bet I could grep for == null|isNull and probably find many more uses. I'd be happy to do so after this PR to prove its utility :).

#24974 other uses :D.

edolstra · 2017-04-11T15:52:45Z

To be honest I've never understood what lib/systems.nix etc. are for. Can anybody explain what the goal is?

If the goal is to provide a handful of properties of platforms, it would be simpler to just enumerate them, given that we only support something like 6 platforms. E.g.

platformProperties = {
  "i686-linux" = {
    is64Bit = false;
    execFormat = "elf";
    ...
  };
  "x86_64-darwin" = {
    is64Bit = true;
    execFormat = "macho";
    ...    
  };
}

Though some properties seem questionable. For example, why do we care whether a platform is 64 bit or big endian? (What do we even mean with 64 bit? LP64, LLP64, ...?)

The other thing I've never understood is why pkgs/top-level/platforms.nix (nixpkgs/lib/systems/platforms.nix in this PR) contains Linux kernel configuration. Kernel configuration should really be moved to pkgs/os-specific/linux/kernel (maybe just added to common-config.nix), and most can be eliminated (e.g. lines like IPV6 m are not platform-specific).

Ericson2314 · 2017-04-11T16:14:58Z

To be honest I've never understood what lib/systems.nix etc. are for. Can anybody explain what the goal is?

My end goal here is to eventually replace all the stdenv.is* predicates, which are woefully inadequate/misleading for cross compilation.

If the goal is to provide a handful of properties of platforms, it would be simpler to just enumerate them, given that we only support something like 6 platforms. E.g.

While we only support a few build platforms, people can and do specify all sorts of obscure stuff for the host platform. I want to be more general so the same predicates can be used for all of build, host, and target.

For example, why do we care whether a platform is 64 bit or big endian? (What do we even mean with 64 bit? LP64, LLP64, ...?)

What CPU attributes we track is less important than a mechanism to do so. If this specific needs to be given more detailed or removed, I'd be happy to do so.

The other thing I've never understood is why pkgs/top-level/platforms.nix (nixpkgs/lib/systems/platforms.nix in this PR) contains Linux kernel configuration.

Yeah I agree that file / the {build,host.target}Platform.platform (formerly crossPlatform.platform) attribute set is an undocumented, unorganized cesspool. I think this is a step in the right direction---at least get the file in a sane location before we triage its contents. The only kernel config I'd even consider keeping is that which effects the contents of the headers---that way downstream derivations can require that some kernel feature be enabled. Everything else kernel-wise deserves a better home and I like your suggestion.

nbp · 2017-04-11T15:18:33Z

lib/attrsets.nix

@@ -424,11 +424,11 @@ rec {
       matchAttrs { cpu = { bits = 64; }; } sys
       => true
   */
-  matchAttrs = pattern: attrs:
-    fold or false (attrValues (zipAttrsWithNames (attrNames pattern) (n: values:
+  matchAttrs = pattern: attrs: assert isAttrs pattern;


Can we remove the FIXME from the comment now?

nbp · 2017-04-11T15:54:14Z

lib/systems/parse.nix

-    };
+  isKernel = x: isType "kernel" x
+    && isExecFormat x.execFormat
+    && all isKernelFamily (attrValues x.families);


These conditions should be assertions at the creation of anything with _type = "kernel", otherwise this assertion would be checked every time these sets are accessed instead of the only time these sets are created. Thus the type "kernel" would imply that these assertions are valid as well, without recursively going down in the set.

Inductive enforcement makes sense. Likewise for isCpuType which you wrote I'd think :D?

yes, please.

nbp · 2017-04-11T15:54:33Z

lib/systems/parse.nix

-    inherit name cpu arch kernel;
+    && isVendor  x.vendor
+    && isKernel  x.kernel
+    && isAbi     x.abi;


Same here for isSystem.

nbp · 2017-04-11T16:00:49Z

lib/systems/parse.nix

-    l = lib.splitString "-" s;
-
+  mkSystemFromSkeleton = { cpu
+                         , vendor ? assert false; null # optional, but fallback too complex for here


nit: These are actually inferred below, update the comment to say so.

nbp · 2017-04-11T16:23:00Z

lib/systems/default.nix

+  elaborate = args: let
+    final = {
+      # Prefer to parse `config` as it is strictly more informative.
+      parsed = parse.mkSystemFromString (if args ? config then args.config else args.system);


What is this config attribute and from where does it come from?

It's an LLVM target triple. The name is unfortunate but not my choice. See the code block in https://nixos.org/wiki/CrossCompiling#Cross-compiling_in_practice for example.

Sadly that wiki page is gone now - Can we not rename this attribute to llvmTargetConfig?

https://web.archive.org/web/20160322211219/https://nixos.org/wiki/CrossCompiling

I'm all for improving the name but we must be careful as this is a public interface to how Nixpkgs is instantiated.

Also, while I follow LLVM most closely, other tools use these too.

ah right. In that case, maybe rename args to compileConfig?

nbp · 2017-04-11T16:25:38Z

lib/systems/parse.nix

+                         , abi    ? assert false; null # ditto
+                         } @ args: let
+    getCpu = name:
+      attrByPath [name] (throw "Unknow cpuType `${name}'.")


@edolstra the backtick-quote used to be the way names were reported by Nix tools.

This is a semantic change, but probably a safe one. In any event, this is very old hardware that probably no one uses anymore anyways.

Previously, platforms was a random thing in top-level

The old hard-coded lists are now used to test system parsing. In the process, make an `assertTrue` in release lib for eval tests; also use it in release-cross

Warning, this changes the compatibility claims of existing packages!

Ericson2314 · 2017-04-17T21:28:05Z

I've addressed all concerns but the nature of the tests. There is some future worked blocked on this I'd like to get to, so I am merging now. @edolstra if you'd like me to redo the tests to match what @nbp did, just ping and I'll make another PR doing that.

cstrahan · 2017-04-17T23:16:56Z

lib/systems/parse.nix

+                         } @ args: let
+    getCpu = name:
+      attrByPath [name] (throw "Unknown CPU type: ${name}")
+        cpuTypes;


@Ericson2314 This might have been pointed out, but... I believe you can also do this to the same effect:

cpuTypes.${name} or (throw "Unknown CPU type: ${name}")

At least, if I'm not mistaken. Which is better is a matter of opinion, I suppose (though I guess it's also possible that one performs better than the other).

That's true and nicer. And those functions can be inlined too. Blame @nbp who originally wrote the code a decade ago 56ed820 :D:D:D

Feel free to make another PR---in general I expect this file to get a lot attention and grow in complexity as this stuff gets used---I've got more to stuff to upstream so not sure whether I have time!

edolstra · 2017-04-18T10:04:48Z

@Ericson2314 Yes please, it would be nice to have one Hydra job for Nixpkgs lib tests.

Worthwhile to do now that NixOS#24610 makes it less abysmal.

Ericson2314 added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Apr 4, 2017

Ericson2314 assigned shlevy, Ericson2314 and nbp Apr 4, 2017

Ericson2314 added the 2.status: work-in-progress This PR isn't done label Apr 4, 2017

Ericson2314 changed the title ~~Platform normalization~~ [WIP] Platform normalization Apr 4, 2017

Mic92 reviewed Apr 4, 2017

View reviewed changes

Ericson2314 force-pushed the platform-normalization branch from 3ab0ad4 to ff55591 Compare April 4, 2017 16:12

Ericson2314 force-pushed the platform-normalization branch from ff55591 to 09139e3 Compare April 4, 2017 16:51

Ericson2314 changed the title ~~[WIP] Platform normalization~~ Platform normalization Apr 4, 2017

Ericson2314 force-pushed the platform-normalization branch from 09139e3 to 9f44340 Compare April 4, 2017 20:39

shlevy removed their assignment Apr 7, 2017

Ericson2314 force-pushed the platform-normalization branch from 9f44340 to d52ccd4 Compare April 10, 2017 19:46

edolstra reviewed Apr 11, 2017

View reviewed changes

nbp reviewed Apr 11, 2017

View reviewed changes

Ericson2314 force-pushed the platform-normalization branch 3 times, most recently from db0a3e2 to 3ccd13a Compare April 11, 2017 20:54

Ericson2314 mentioned this pull request Apr 17, 2017

All stdenvs should have a generic libc attribute #6221

Closed

Ericson2314 force-pushed the platform-normalization branch from 3ccd13a to 6f1d655 Compare April 17, 2017 19:44

Ericson2314 added 7 commits April 17, 2017 17:13

release-cross: For fuloong, just use the predefined platform

213d29f

This is a semantic change, but probably a safe one. In any event, this is very old hardware that probably no one uses anymore anyways.

top-level: Less indirection for lib in release*.nix

b477851

lib: Collect system/platform related files

2227789

Previously, platforms was a random thing in top-level

lib: Fix matchAttrs

fffcee3

lib: Fix system parsing, and use for doubles lists

8c99aab

The old hard-coded lists are now used to test system parsing. In the process, make an `assertTrue` in release lib for eval tests; also use it in release-cross

lib: Fix preexisting bugs in old platforms code (now bugs in tests)

d86caa3

Warning, this changes the compatibility claims of existing packages!

Elaborate localSystem and crossSystem in a consistent manner

3efc661

Ericson2314 force-pushed the platform-normalization branch from 6f1d655 to 3efc661 Compare April 17, 2017 21:13

Ericson2314 merged commit f0b634c into NixOS:master Apr 17, 2017

Ericson2314 deleted the platform-normalization branch April 17, 2017 21:28

cstrahan reviewed Apr 17, 2017

View reviewed changes

This was referenced Apr 18, 2017

cross-compilation: native derivations and stdenv.cross #14965

Closed

release-cross: Fix typo preventing evaluation #24981

Merged

Ericson2314 removed the 2.status: work-in-progress This PR isn't done label Apr 18, 2017

Ericson2314 mentioned this pull request Apr 18, 2017

[WIP] Cross compilation changes, with enphasis on (haskell) GHC HEAD #24983

Closed

3 tasks

Ericson2314 added a commit to Ericson2314/nixpkgs that referenced this pull request Apr 18, 2017

doc: Document the structure of {build,host,target}Platforms

c13457f

Worthwhile to do now that NixOS#24610 makes it less abysmal.

eqyiel mentioned this pull request Jul 2, 2017

Infinite recursion after updating to 17.03 #24170

Closed

Ericson2314 mentioned this pull request Dec 1, 2019

[RFC 0049] Flakes NixOS/rfcs#49

Closed

Platform normalization #24610

Platform normalization #24610

Conversation

Ericson2314 commented Apr 4, 2017 • edited Loading

Motivation for this change

Things done

mention-bot commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlevy commented Apr 4, 2017

Ericson2314 commented Apr 4, 2017

Ericson2314 commented Apr 4, 2017 • edited Loading

Ericson2314 commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edolstra commented Apr 11, 2017

Ericson2314 commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Apr 17, 2017

Choose a reason for hiding this comment

Ericson2314 Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Ericson2314 Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

edolstra commented Apr 18, 2017

Ericson2314 commented Apr 4, 2017 •

edited

Loading

Ericson2314 commented Apr 4, 2017 •

edited

Loading

Ericson2314 Apr 11, 2017 •

edited

Loading

Ericson2314 Apr 11, 2017 •

edited

Loading

Ericson2314 Apr 11, 2017 •

edited

Loading

Ericson2314 Apr 17, 2017 •

edited

Loading

Ericson2314 Apr 18, 2017 •

edited

Loading