Conversation
Enabling LTO causes test failures when compiling for x86_64-darwin (at least under Rosetta 2). The Nix package only enables LTO when using GCC, so we'll copy that for now. I don't feel safe leaving it on for aarch64-darwin unless we can understand why it's failing on x86_64-darwin in case this is an indication of UB. Fixes NixOS#337036.
| # Enable LTO, since it improves eval performance a fair amount | ||
| # LTO is disabled on static due to strange linking errors | ||
| (lib.mesonBool "b_lto" (!stdenv.hostPlatform.isStatic)) | ||
| (lib.mesonBool "b_lto" (!stdenv.hostPlatform.isStatic && stdenv.cc.isGNU)) |
There was a problem hiding this comment.
Can we disable it for Darwin instead?
Clang LTO on Linux should be fine.
And add a comment there why its disabled on Darwin.
There was a problem hiding this comment.
Have you tested it? I would suspect it is more likely to be LLVM exposing UB than a Darwin issue. And the Nix derivation uses isGNU here.
There was a problem hiding this comment.
Tried building lix from master with clangStdenv and it built fine. (x86_64-linux)
There was a problem hiding this comment.
Interesting. What about with libc++?
There was a problem hiding this comment.
This does work if that's what you are asking:
❯ nix build --impure --expr '(builtins.getFlake "github:nixos/nixpkgs/master").legacyPackages."${builtins.currentSystem}".libcxx.override {stdenv = (builtins.getFlake "github:nixos/nixpkgs/master").legacyPackages."${builtins.currentSystem}".clangStdenv;}' --print-out-paths
/nix/store/nqpv4xbh5zqnvfn80702gahiklxxjcr3-libcxx-18.1.8
PS: if you have a shorthand for these overrides in cli, let me know!
There was a problem hiding this comment.
what in the hell. maybe we have a missing include but I'm guessing those sys constants are just broken. (though libcxxStdenv is SURELY not using an alternative libc right? just libc++?)
it may also be broken toolchain on the nixpkgs side for all i know, like this release blocker for lix 2.92: #177129
There was a problem hiding this comment.
This does seem to only be a missing include. Sent a patch upstream in https://gerrit.lix.systems/c/lix/+/2286
Using this override still fails when linking with things like the aws-sdk though (probably because they're using glibc?), and being a bit more forceful with libcxx usage like so
let
pkgs = import <nixpkgs> {
config = {
replaceStdenv = { pkgs }: pkgs.libcxxStdenv;
};
overlays = [ ];
};
in
pkgs.lixresults in the always fun error: infinite recursion encountered, so I don't think we can determine whether this is a libcxx or Darwin issue just yet
I'm inclined to say we should change this to stdenv.cc.libcxx != null for now, as that would cover Darwin and the possibly broken (and currently un-evaluatable) libcxx Linux build -- which can always be revisited later
Also let me know if there's anything I can help with to get this merged. Would love to see a fix after all this time :)
There was a problem hiding this comment.
i believe there is a libcxxStdenvPackages somewhere that is a nixpkgs staged from libcxxStdenv but yeah you can't really mix libc++ and libstdc++ in one binary at least if you're linking c++ symbols between them.
if this pr is acceptable i can just like, merge it. i have commit rights. i think it's more important that lix works than that it has LTO in all configurations possible (including weird ones) given that iirc LTO doesn't make that huge a difference anyway.
There was a problem hiding this comment.
i think it's more important that lix works than that it has LTO in all configurations possible
Agreed.
I'm inclined to say we should change this to
stdenv.cc.libcxx != nullfor now, as that would cover Darwin and the possibly broken (and currently un-evaluatable) libcxx Linux build -- which can always be revisited later
Also agreed.
There was a problem hiding this comment.
if this pr is acceptable
I'd say it 100% is. My suggestions were by no means a blocker, just a way to hopefully get things going
i believe there is a libcxxStdenvPackages somewhere that is a nixpkgs staged from libcxxStdenv
TIL that is pkgsLLVM...I thought that used clangStdenv. Time to build :)
|
@ofborg build lix |
JohnRTitor
left a comment
JohnRTitor
left a comment
There was a problem hiding this comment.
Builds on both x86_64-darwin and aarch64-darwin on Nix communtiy builder.
ofborg
bot
added
10.rebuild-darwin: 1-10
getchoo
left a comment
getchoo
left a comment
There was a problem hiding this comment.
nixpkgs-review result
Generated using nixpkgs-review.
Command: nixpkgs-review pr 353576
x86_64-darwin
✅ 10 packages built:
- lix (lixVersions.latest ,lixVersions.lix_2_91 ,lixVersions.stable)
- lix.dev (lixVersions.latest.dev ,lixVersions.lix_2_91.dev ,lixVersions.stable.dev)
- lix.devdoc (lixVersions.latest.devdoc ,lixVersions.lix_2_91.devdoc ,lixVersions.stable.devdoc)
- lix.doc (lixVersions.latest.doc ,lixVersions.lix_2_91.doc ,lixVersions.stable.doc)
- lix.man (lixVersions.latest.man ,lixVersions.lix_2_91.man ,lixVersions.stable.man)
- lixVersions.lix_2_90
- lixVersions.lix_2_90.dev
- lixVersions.lix_2_90.devdoc
- lixVersions.lix_2_90.doc
- lixVersions.lix_2_90.man
aarch64-darwin
✅ 10 packages built:
- lix (lixVersions.latest ,lixVersions.lix_2_91 ,lixVersions.stable)
- lix.dev (lixVersions.latest.dev ,lixVersions.lix_2_91.dev ,lixVersions.stable.dev)
- lix.devdoc (lixVersions.latest.devdoc ,lixVersions.lix_2_91.devdoc ,lixVersions.stable.devdoc)
- lix.doc (lixVersions.latest.doc ,lixVersions.lix_2_91.doc ,lixVersions.stable.doc)
- lix.man (lixVersions.latest.man ,lixVersions.lix_2_91.man ,lixVersions.stable.man)
- lixVersions.lix_2_90
- lixVersions.lix_2_90.dev
- lixVersions.lix_2_90.devdoc
- lixVersions.lix_2_90.doc
- lixVersions.lix_2_90.man
getchoo
added
12.approvals: 1
|
Successfully created backport PR for |
Per `seccomp(2)`, this header defines the `SYS_*` constants used Found in NixOS/nixpkgs#353576 Change-Id: I64ebc5513c53500b7a9012eaa664c2e59f49ceac
8 participants
Enabling LTO causes test failures when compiling for x86_64-darwin (at least under Rosetta 2). The Nix package only enables LTO when using GCC, so we'll copy that for now. I don't feel safe leaving it on for aarch64-darwin unless we can understand why it's failing on x86_64-darwin in case this is an indication of UB.
Fixes #337036.
I am making my contributions to this project solely in my personal capacity and am not conveying any rights to any intellectual property of any third parties.
Things done
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.