-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
musl-cross, native-musl, cross fixes, oh my! #34645
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yayaayyyayayayayyayayayayyayayaayayayy!
Here's a few comments from a quick (relative to the size of this at least!) first pass. Very excited to merge this!
}.${localSystem.system} | ||
or (abort "unsupported platform for the pure Linux stdenv") | ||
, bootstrapFiles ? | ||
let table = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just use config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think "config" is canonical for a a given system/libc pair. I'm not sure how important that is, but that's why I did things this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah makes sense.
lib/systems/default.nix
Outdated
@@ -26,7 +26,9 @@ rec { | |||
libc = | |||
/**/ if final.isDarwin then "libSystem" | |||
else if final.isMinGW then "msvcrt" | |||
else if final.isLinux then "glibc" | |||
else if final.isLinux && final.isGlibc then "glibc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not wild about how libc
and the new is*
predicates are somewhat disconnected. But I see its the best choice given how things are today. Maybe the solution is to move libc
into parsed
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. Related: It seems "abi" dictates the libc to be used, at least if it's not "unknown"...
Hmm, I trust you on this but happy to give feedback on your thoughts.
lib/systems/examples.nix
Outdated
@@ -63,6 +63,20 @@ rec { | |||
platform = platforms.fuloong2f_n32; | |||
}; | |||
|
|||
muslpi = raspberryPi // { | |||
config = "armv6l-unknown-linux-musleabihf"; | |||
libc = "musl"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the libc = ...
s needed? I'd love to remove them everywhere and have inference take care of it; the more minimal these attribute sets are the better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so! This one is "needed" only because I base it off of "raspberryPi" which explicitly sets libc to glibc; if it didn't I wouldn't need to override it.
AFAIK they're inferred properly by default in all examples in this file.
Which, by the way, is magical :D.
musl-cross.nix
Outdated
@@ -0,0 +1,5 @@ | |||
{ src ? ./default.nix }: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errr should we have new top level files? I do want an easier way to get people going, but not sure what should be it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no! This was just for my testing--especially early on before recent improvements -- to avoid error-prone and/or long nix incantations. This should 0% be merged anywhere, I'll drop it sorry.
@@ -314,7 +324,44 @@ stdenv.mkDerivation ({ | |||
+ stdenv.lib.optionalString (langJava || langGo) '' | |||
export lib=$out; | |||
'' | |||
; | |||
+ stdenv.lib.optionalString (buildPlatform != hostPlatform) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these? I haven't needed them so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check, the commits re:gcc6 (and in a few other places in this PR) are still a bit "exploratory" and showing something that works vs "ready to merge" and clean-- and likely are not minimal.
Not at all being defensive :) just FYI, and good question I'll check. I hope you're right!
|
||
cross = if buildPlatform != hostPlatform then hostPlatform else null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of these sorts of cross args. glibc is a bad derivation style!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xD. Turns out it's only needed for adding the cross suffix... O:).
stdenv.mkDerivation rec { | ||
name = "musl-${version}"; | ||
name = "musl-${version}" + | ||
lib.optionalString (cross != null) "-${cross.config}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapter should already do this, right? (I recently removed redundency from glibc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Yes adapter of course does do this, I assumed/guessed this was for libcCross which is a bit "special".
That said, it does result in a lot of redundancy as-is because "build != host" is not how you check "am I being built as libcCross"! Also it might not be needed even then...yep it sure doesn't appear to be needed then either.
Ah yes, I probably was following along glibc, from before that change.
I'll remove this, although immediately reverting it to avoid mass-rebuild while working on other issues.
@@ -0,0 +1,132 @@ | |||
From 3714c5be3212d2af61545439eeb432e5d84a8d39 Mon Sep 17 00:00:00 2001 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not fetchPatch
this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check. In general I preferred, mostly for convenience, to make local copies of patches.
This isn't the "Nixpkgs Way", I'll see what can be fetchpatch'd here and elsewhere.
pkgs/stdenv/linux/default.nix
Outdated
@@ -95,7 +107,7 @@ let | |||
|
|||
# stdenv.glibc is used by GCC build to figure out the system-level | |||
# /usr/include directory. | |||
inherit (prevStage) glibc; | |||
# inherit (prevStage) glibc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay! stdenv.glibc
is an awful attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Very glad you agree. So far doesn't seem to be used for anything worthwhile.
pkgs/stdenv/generic/default.nix
Outdated
@@ -117,7 +117,8 @@ let | |||
# Utility flags to test the type of platform. | |||
inherit (hostPlatform) | |||
isDarwin isLinux isSunOS isHurd isCygwin isFreeBSD isOpenBSD | |||
isi686 isx86_64 is64bit isArm isAarch64 isMips isBigEndian; | |||
isi686 isx86_64 is64bit isArm isAarch64 isMips isBigEndian | |||
isMusl isGlibc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to deprecate these in 18.03
to force people to think about cross. What do you think? If so, maybe we shouldn't add anymore here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me! I'll make a pass removing these. I do like having "isMusl" and "isGlibc"-- they're less error-prone than libc == "stringvalue"
. But accessing them from the appropriate platform (hostPlatform
) sounds good to me.
lib/systems/examples.nix
Outdated
@@ -63,6 +63,18 @@ rec { | |||
platform = platforms.fuloong2f_n32; | |||
}; | |||
|
|||
muslpi = (removeAttrs raspberryPi [ "libc" ]) // { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err I meant literally remove libc
from raspberryPi
above. It should be inferred just fine!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, sure thing. I agree that's nicer/cleaner, I just already feel I touch "unrelated" things in this PR and leaving existing examples as-is seemed easiest way to make it easy for reviewers to see I don't break them :).
Anyway, removed!
Whoops rebasing onto master while targetting staging was a dumb idea. Will fix shortly. |
++ optional stdenv.cc.isClang ./libcxx38-and-above.patch; | ||
++ optional stdenv.cc.isClang ./libcxx38-and-above.patch | ||
++ optional stdenv.hostPlatform.isMusl (fetchpatch { | ||
url = "https://raw.githubusercontent.com/richfelker/musl-cross-make/master/patches/gcc-5.3.0/0001-musl.diff"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use a commit hash here, just to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, agreed. Done!
fc88a46
to
325bc53
Compare
FYI, I just noticed that some of the new musl stuff won't build with gcc-7, e.g. https://hydra.nixos.org/build/69495411 |
This doesn't fix the issue for me, does it for you? This is what I get:
|
AFAICT this happens on gcc6 as well, perhaps on all non-musl. Not sure there's much to be done about this, at least for now--I think @Ericson2314 has a PR that'd let us express that this is not meant for non-musl systems...? |
Oh, right, I looked wrong, into still-failing instead of newly-failing. |
I'm really not in favour of adding musl support to Nixpkgs. Adding support for non-standard libcs is a major undertaking. From my experience with dietlibc and klibc, it will require patches everywhere, which is maintenance nightmare, for no discernable advantage. Since this is a major expansion of the scope of Nixpkgs, a decision on whether to add support for other libcs should really be done via the RFC process. |
First pass at putting together an RFC for musl: NixOS/rfcs#23 WIP, looking for early feedback and any and all willing to serve as co-authors of the RFC. |
If nixpkgs rejects this SLNOS will happily adopt this patchset. Just sayin'.
|
What is SNLOS, btw? |
@Ericson2314 https://github.com/SLNOS/nixpkgs — a NixOS external branch with suckless.org influences |
https://github.com/SLNOS/nixpkgs — a NixOS external branch with suckless.org influences
But the public mirror is almost empty ATM as we are not a public fork (ATM). I regularly PR patches from SLNOS to nixpkgs (mostly mine, sometimes authored by others, see `git log --author "SLNOS" .` in nixpkgs). The only thing we regularly publish on that github page is the "no-bundle" fork of tor-browser nixpkgs currently uses for the "tor-browser from source" builds.
You can read up on the mission in "The Church of Suckless NixOS is looking for followers" thread on ML from Fri, 17 Mar 2017 14:00:00 +0000 if you're interested.
While musl support wasn't listed on TODO list from that thread it was implied there and it is on it explicitly right now. We want options for Alpine-like and sta.li-like SLNOS builds (eventually).
FYI, ATM SLNOS provides
* Gentoo-like use-flags for pkgs,
* partial support for properly ccache-accelerated builds (nixpkgs' `ccacheStdenv` is broken and doesn't solve the main time waste on mass rebuilds: stdenv itself),
* monoidal services for os,
* bootloader management separated from os (you can have NixOS, SLNOS and, with some ugly hacking, GuixSD, all installed and bootable in parallel in a single nix-store),
* partial support for s6 init,
* a bunch of security- and privacy-oriented stuff,
* (including no PulseAudio by default),
* a bunch of other random crap.
We are >400 patches and counting over master already, so adopting this tiny little branch is practically nothing.
|
@dtzWill hydra builds of |
Haha sorry, and no. They just are only meant to be used with a musl stdenv, where they work fine. Should probably set meta.platforms appropriately, we couldn't do this before. |
#37337 will make |
@dtzWill, why did you add |
@orivej in my tree I found builds using SDL needed iconv so I figured it was supposed to be propagated (or "part of libc/stdenv"). Why, is this causing problems/unexpected? |
(I was using the full gnu libiconv, not what's available in musl which I think is what we're doing on master currently). |
@dtzWill Can you explain a bit how to use this? You have a "Quickstart with Binary Cache" section but I can't figure out what to configure in nix to actually use musl for everything. |
Hey, just want to let you know that with the work in here I've managed to build some fully static Haskell executables! https://github.com/nh2/static-haskell-nix |
configureFlags = | ||
[ "--enable-cplusplus" ] | ||
++ lib.optional enableLargeConfig "--enable-large-config"; | ||
++ lib.optional enableLargeConfig "--enable-large-config" | ||
++ lib.optional (stdenv.hostPlatform.libc == "musl") "--disable-static"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dtzWill @matthewbauer Why was this needed?
Doesn't the true-by-default dontDisableStatic
do this already?
I'm trying to purge hardcoded --disable-static
flags everywhere, and this one left me unsure.
Clang now supports OpenMP, and musl has no problem with it either. Related to NixOS#7023 and NixOS#34645. See also NixOS#79818.
Motivation for this change
Support cross-compiling for musl, support musl-native, generalize a bit beyond glibc, many cross fixes.
A few fixes are "borrowed" from @bgamari's big PR (#30882).
PR History
To help folks track changes made across updates to this PR, I'm tagging each revision to help diff and whatnot:
stdenv.glibc
TODO
Minor / Misc / Maybe
mkdir
to be used. This doesn't occur when building the crossstdenv.cc
, FWIW. Example error:getent
utilityQuickstart with Binary Cache
To enjoy musl goodness right away, consider taking advantage of the ALLVM project's binary cache!
To do so, add the following to your configuration.nix:
(Please add it after the nixos one O:))
Equivalent of "small" jobset is evaluated and cached for a few build configurations,
but certainly not everything works yet. This is just a (hopefully solid) start!
Credits and Teasers: ALLVM
This PR (and many of the related fixes submitted recently) are based on work done as part of the ALLVM project, many thanks to our funding 👍.
We look forward to sharing more with everyone soon! Stay tuned :).