-
-
Notifications
You must be signed in to change notification settings - Fork 15k
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
coreutils: fix musl cross compilation #61250
Conversation
Thanks for doing this! But I think we would prefer to use fetchpatch directly to https://git.savannah.gnu.org/cgit/gnulib.git/patch/?id=453ff940449bbbde9ec00f0bbf82a359c5598fc7 |
The patch as is does not apply to the tarball that is distributed on the mirrors. Partly because the sources changed in between, partly because the stuff that happens during |
Maybe we could only keep the modifications to the patch in nixpkgs and patch the patch during prePatch? Edit: In case you are concerned with the size. |
Ok I guess that's alright. It's just a pretty big patch, but we should be able to remove it when the next coreutils release happens? |
It is already merged upstream, so yes. I didn't find any info related to release planning with a quick search, but recent releases have taken between 3 and 9 months. On another note: Do you have an idea why this triggers mass-rebuilds for the other libcs? |
Because you set stdenv.mkDerivation ({
pname = "foo";
/*
...
*/
} // optionalAttrs stdenv.hostPlatform.isMusl {
NIX_CFLAGS_COMPILE = [ "-Wno-error" ];
}) |
That makes sense. I was hoping empty fields would be skipped when calculating the output hash, but I guess that would create problems... I'll change it according to your suggestion. |
5020f35
to
66cf1c6
Compare
Changed target to master since it doesn't cause a huge amount of builds any more. |
@matthewbauer @tobim This breaks the build for me, because a test fails:
This is on commit 3c37e94 (the merge of this to Did it build for you? |
Before this PR, the relevant test wasn't run:
Still kinda suspicious for it to return the wrong date in the test. We should figure out what's going on. |
@dtzWill do you know what might be going on? Also, what are these date related changes for? Could you add some comments there to explain them? |
Indeed musl seems to be the difference. I've compiled coretuils Running
|
I've started debugging a bit: In Printing: fprintf(stderr, "tm_hour = %d\n", tm->tm_hour);
fprintf(stderr, "tm_isdst = %d\n", tm->tm_isdst); glibc:
musl:
|
I ran your command on 59a733e (don't think the commits that came afterwards would change anything):
The test that fails for you does not show up at all for me. |
@tobim That's very odd. You also have only I get the same error on your commit. As per your ticks in the issue description, you're building on NixOS with sandboxing, just like me. However, I have the suspicion that some impurity made it into my build. I've just built it on another NixOS machine, and the issue did not occur. For the builds on my Ubuntu machine, I use nix's This does not explain, however, why I see the same test failure between glibc and musl when building coreutils only on Ubuntu, where nixpkgs isn't involved at all. |
@tobim @matthewbauer OK, I think I found at least the NixOS-related answer: The machine I built on actually had This means that when the sandbox is on, the relevant test simply does not run, because But the problem remains that the test shouldn't fail; if times are parsed incorrectly somehow, we probably want to get that fixed. I've posted the problem on the coreutils mailing list: https://lists.gnu.org/archive/html/coreutils/2019-05/msg00031.html |
I've discussed with He also replied to the mailing list thread, but it isn't in the archive yet so I can't link it yet. I think we should disable this test for now somehow, so that coreutils builds fine with musl even without sandbox. |
https://lists.gnu.org/archive/html/coreutils/2019-05/msg00039.html |
PR to patch the test at #61471 |
See NixOS#61250 (comment) Using upstream patch.
See NixOS#61250 (comment) Using upstream patch.
See NixOS#61250 (comment) Using upstream patch.
This should not affect glibc systems, but targeting staging to be safe.
I only tested this with gcc set to version 8 as default, so the CFLAGS workaround might not be strictly necessary.
Motivation for this change
See #59934.
Things done
Backported https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=453ff940449bbbde9ec00f0bbf82a359c5598fc7.
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)