lib.systems: introduce toolchain, cc, and bintools attributes#365057
lib.systems: introduce toolchain, cc, and bintools attributes#365057RossComputerGuy wants to merge 3 commits intoNixOS:masterfrom
Conversation
7a3eca0 to
2a005fa
Compare
9c8b44b to
f6da63d
Compare
|
The main idea and motivation is in: pkgs/stdenv/cross/default.nix |
f6da63d to
ec79349
Compare
10cd2ba to
2b4d320
Compare
K900
left a comment
There was a problem hiding this comment.
ack, would be nice to make this rebuild 0
|
@K900 there was a typo with xwayland, that should be fixed now. It'd be great if we can get this PR through before it becomes more work to rebase. |
|
Rebasing, we also have an stdenv board now. I hope this doesn't stall out again because there's a bunch of conflicts and I don't want to rebase this regularly. |
philiptaron
left a comment
There was a problem hiding this comment.
Thanks for your work here Tristan. I'm looking to @emilazy to give the ✅ , but I think we are agreed:
- Using well-known strings with symbolic meaning is the right direction, rather than
is*anduse*functions attached to the stdenv. - Splitting apart the concept of the "toolchain" into "cc", "bintools", "cxxlib", "unwinderlib", "rtlib", and "linker" allows derivations to tune their behavior wisely in the face of the combinatorial explosion of possibilities.
- We should expect to continue to evolve this system over the course of the next few releases as the codebase adopts and reacts to these well-known strings -- and introduces more, because the innovation train isn't stopping soon.
I'd like to do a build on staging once the merge conflicts are resolved and at least one of @reckenrode or @emilazy accepts, then merge.
What say you all?
lib/systems/default.nix
Outdated
There was a problem hiding this comment.
Not blocking:
If (lib.listToAttrs (lib.map (key: lib.nameValuePair key final.${key}) (lib.attrNames toolchain))) were extracted to a let binding, we could write a good assertion message here.
There was a problem hiding this comment.
This might be fine as is. I'm not sure, maybe just a comment would be okay? Let's see what others think.
There was a problem hiding this comment.
I am mostly worried about whether elaborate is in the hot path of anything. I think that since these values didn’t exist before it’s okay if the error message is bad for now. assert toolchain == lib.mapAttrs (key: _: final.${key}) toolchain seems better and surely faster, regardless.
There was a problem hiding this comment.
I am mostly worried about whether
elaborateis in the hot path of anything. I think that since these values didn’t exist before it’s okay if the error message is bad for now.
I think the people who tend to change these sorts of things are in the minority. They likely would be in a Matrix chat one of us are in anyway so clarification on the failure is possible there. We shouldn't rely on that but I don't see many people utilizing this feature until it is flushed out more.
assert toolchain == lib.mapAttrs (key: _: final.${key}) toolchainseems better and surely faster, regardless.
Oh yeah, that is a better way of checking.
pkgs/by-name/al/alsa-lib/package.nix
Outdated
There was a problem hiding this comment.
me getting confused: why would this be the hostPlatform and not the buildPlatform's linker?
There was a problem hiding this comment.
According to https://nixos.org/manual/nixpkgs/stable/#ssec-cross-platform-parameters, host is the platform where things run. The things we run are being built with a different linker. crossPlatform in nixpkgs goes to hostPlatform and targetPlatform.
pkgs/by-name/el/elfutils/package.nix
Outdated
There was a problem hiding this comment.
this file in particular looks a lot better
pkgs/top-level/stage.nix
Outdated
There was a problem hiding this comment.
I like these changes a lot.
|
Considering the size of this PR, complexity, available time, and other factors I think this should be fine as is. We always can work on improving this with additional work. This is something I very much would like to get into 25.05 so I can work on other work like reworking the CPU model support and going towards LLVM bootstrapping inside the stdenv. I know @philiptaron mentioned he'll start a build up this weekend so we'll see what happens from that. |
|
I think that a PR being huge and complex and having limited available qualified reviewer time are the opposite of reasons to rush a PR… I still intend to try and find the time to review this over the weekend. However, I continue to think it would be best for everyone to split it up into stages as I previously recommended, since this is still only part of the picture as it is. Landing in 25.05 or not shouldn’t matter for follow‐up work, since these are purely read‐only properties that wouldn’t be exposed interfaces for platform specification or lead to the deprecation of the existing |
|
Ok, I've split it into just the first commit as a MVP for this feature. This likely will be split into 4 or 6 subsequent PR's. |
emilazy
left a comment
There was a problem hiding this comment.
Thanks, I appreciate you splitting off the migration changes since it’ll make them easier to review without blocking this improvement! It looks good to me now modulo a few nits.
lib/systems/default.nix
Outdated
There was a problem hiding this comment.
Will this not override the input when these are set? (Might just be final being named confusingly…)
There was a problem hiding this comment.
No because args is merged on top, that's how libc, isStatic, and a bunch of other things work.
lib/systems/default.nix
Outdated
There was a problem hiding this comment.
I think this needs to stay outside of toolchain, since overriding it was already technically “supported” (regardless of whether it actually did anything useful) so it’s a breaking change to include it in the checks, sadly.
There was a problem hiding this comment.
Alright, understandable.
lib/systems/default.nix
Outdated
There was a problem hiding this comment.
I think this needs a cxxrtlib to go with it, per #365057 (comment).
There was a problem hiding this comment.
Alright, should we imply the cxxrtlib based on the cxxlib or should it be based on the "toolchain" in use?
lib/systems/default.nix
Outdated
There was a problem hiding this comment.
I think this and maybe other things might be wrong for MinGW + threads + etc., but I’m not sure what would be right and it can probably wait until this work actually starts touching bootstrap in a way that will flag it up.
There was a problem hiding this comment.
Yeah, I think @Ericson2314 would know? However, he can be busy and difficult to get ahold of sometimes. If he can clarify like before the end of the week then I think we can go from there but if it's longer than a week then follow up PR's can fix it.
lib/systems/default.nix
Outdated
There was a problem hiding this comment.
I am mostly worried about whether elaborate is in the hot path of anything. I think that since these values didn’t exist before it’s okay if the error message is bad for now. assert toolchain == lib.mapAttrs (key: _: final.${key}) toolchain seems better and surely faster, regardless.
|
Thank you @emilazy, I've addressed the feedback now. |
|
The PR's base branch is set to master, but 932 commits from the python-updates branch are included. Make sure you know the right base branch for your changes, then:
|
|
Accidentally did |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/5394 |
|
I've realized that this might be necessary to make |
emilazy
left a comment
There was a problem hiding this comment.
Thanks, this looks like a good basis for migrating checks and then the bootstrap. A few details left but I think we can land this soon.
There is one design point I am still unsure about, which is the stringly‐typed nature of the API. Using the cxx vs. c++ ambiguity as an example, a check like stdenv.hostPlatform.cxxlib == "libc++" would be silently wrong right now. That might just be part of the Nix experience, but it is possible we could do better and make this less error‐prone – e.g. expose something stdenv.hostPlatform.cxxlib == lib.toolchain.cxxlib.libcxx to compare against, or lib.toolchain.isLibcxx stdenv.hostPlatform.
Maybe the existing lib.systems.inspect functionality offers everything we need here and we can just define patterns? But I think we should figure out what we want these checks to look like before migrating them across the tree. @alyssais Do you have any opinions on this?
lib/systems/default.nix
Outdated
There was a problem hiding this comment.
@reckenrode What do we want for Darwin here? I’m not sure what the balance of LLVM vs. cctools is currently.
Maybe it would be best to split this up into separate variables, but I’m not sure what the correct split would be.
There was a problem hiding this comment.
Darwin’s bintools is almost all LLVM except for the following: ranlib, otool, install_name_tool, and ld. I’m currently testing a change to use ranlib from LLVM, so it might only be the last three for 25.11.
I’d really like to change the linker to ld64 to match what Darwin is actually using. Technically, cctools has another linker, but it’s old and not used anymore.
There was a problem hiding this comment.
(In short, I think llvm is right for Darwin for bintools here.)
lib/systems/default.nix
Outdated
There was a problem hiding this comment.
We don’t use libcxxrt on any platform right now (but in the future it will probably make sense to use libc++ and libcxxrt on FreeBSD rather than the current libstdc++ plus libsupc++ stack). This should always be libsupc++ when using libstdc++.
Also, we should be consistent around cxx vs. c++ if we diverge from project names. I think I would favour using the upstream project names (so libstdc++, libc++abi, libcxxrt).
There was a problem hiding this comment.
I've switched to replace ++ with xx. The idea behind using xx is then it's a bit nicer to interact with directly since you don't have to wrap in a string if it is an attribute name.
lib/systems/default.nix
Outdated
There was a problem hiding this comment.
| "libunwind-darwin-host" | |
| "libunwind-system" |
“Host” is confusing wrt. hostPlatform. I think we don’t need to encode the Darwin detail because that can be conditioned on separately.
There was a problem hiding this comment.
If we’re going to distinguish the system libunwind on Darwin, should we also distinguish using the system libc++ (once that lands)?
lib/systems/default.nix
Outdated
There was a problem hiding this comment.
I think this should just be libgcc. _s is for shared library.
lib/systems/examples.nix
Outdated
There was a problem hiding this comment.
I don’t think we should set these while they’re redundant to useLLVM (since they couldn’t be any other possible value, and adjusting them won’t work, and it means that e.g. lib.systems.examples.aarch64-freebsd // { useLLVM = false; } becomes broken for no good reason).
There was a problem hiding this comment.
It's necessary for eval to work.
There was a problem hiding this comment.
Since we have lib.systems.toolchain, we can use that to easily help clean out stdenv.hostPlatform of the toolchain attributes. Now that logic is handled outside of this file for setting the new toolchain attributes.
100% agree on this |
Not especially, although reusing an existing mechanism sounds nice if it's workable. |
If we’re going to distinguish the system libunwind and/or libc++ on Darwin, I would like to have some way to do these checks that treats them as libunwind and libc++ when you don’t care about that detail. Otherwise, everywhere that checks will have to condition on both variants, check a prefix, or be wrong for Darwin. (Obviously, there needs to be a way to distinguish when you do care, but I doubt that will be needed much outside of the LLVM derivation.) |
Agreed, I think having a way to distinguish between a specific unwindlib (Apple's vs LLVM's) and a generic unwindlib (GCC vs LLVM) is definitely needed. I was thinking this could be done inside the specific derivations. We could have |
|
Would be nice to continue on this. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/6078 |
Things done
Replaces
useLLVM,useArocc, anduseZigwithtoolchain,cc,linker, andbintoolsattributes. This might not produce any rebuilds but we'll see. This has the advantage of preventingusing${compiler}flags from colliding and not working correctly if we were to stack multiplepkgs*together.nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.