lib.systems: introduce toolchain, cc, and bintools attributes #365057
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
|
I've realized that this might be necessary to make |
d98315d to
b29c2c0
Compare
b29c2c0 to
e2732d8
Compare
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
| bintools = | ||
| if final.useLLVM || final.useArocc || final.useZig || final.isDarwin then "llvm" else "gnu"; |
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
| cxxrtlib = | ||
| if final.useLLVM || final.useArocc || final.useZig || final.isDarwin then | ||
| "libcxxabi" | ||
| else if final.isLinux then | ||
| "libsupc++" | ||
| else | ||
| "libcxxrt"; |
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
| if final.useLLVM || final.useArocc || final.useZig then | ||
| "libunwind" | ||
| else if final.isDarwin then | ||
| "libunwind-darwin-host" |
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
| else if final.isDarwin then | ||
| "libunwind-darwin-host" | ||
| else | ||
| "libgcc_s"; |
There was a problem hiding this comment.
I think this should just be libgcc. _s is for shared library.
| useLLVM = true; | ||
| cc = "clang"; | ||
| bintools = "llvm"; | ||
| cxxlib = "libcxx"; | ||
| cxxrtlib = "libcxxabi"; | ||
| unwindlib = "libunwind"; | ||
| rtlib = "compiler-rt"; |
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 |
e2732d8 to
cc0beaf
Compare
cc0beaf to
8f6e971
Compare
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.