-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
haskell-modules/generic-builder.nix: work around libc++abi issue #266172
Conversation
I haven't been following these clang changes. Could you be more specific about where would be the right place to make these fixes? For instance, if you don't fix
Is there an issue/PR you could link to in the code that made it easier to track when this happens? (Maybe it is #166205?) Also, if these commits are meant to just all be reverted soon, it might make it easier for us if all of the changes in this PR were included in a single commit. |
The right place is cc-wrapper. This problem affects any program that invokes My primary concern with modifying
Depending on which modules are used, potentially. Should I go ahead with modifying
I pinged the Nixpkgs/NixOS Contributions room on Matrix to see if anyone is working on a fix. I thought someone was, but I don’t recall who offhand and don’t think there is an issue or draft PR tracking that work.
Depending on whether the preference is to do a localized fix here or fix it Haskell-wide at the cost of rebuilds, I can squash or push that alternative instead. |
06d4bc6
to
b0fac4d
Compare
Pinging @toonn for awareness since he’s working on the cc-wrapper changes. @cdepillabout I went ahead and squashed the commits as requested. I skimmed the list of problematic dependencies in the Nix Tools report for the latest staging-next evaluation. The other Haskell issues don’t appear to be related to this one. It doesn’t seem not worth the mass rebuilds to use a more general fix. |
The thing about the Haskell stuff in Nixpkgs is that it is commonly used by end users, to build their own packages not included in Nixpkgs. If there is a way we can fix it generally, so that individual packages/users don't have to worry about it, I'd say we should do it. My personal opinion is that it is more important to not break end-users' code, rather than trying to minimalize rebuilds on Hydra. |
Sure, I can do that. Thank you for clarifying. I’ve been erring on the side of avoiding rebuilds if possible, but I can put the fix in (I assume that pushing the fix to staging instead is undesirable because there would be a risk of end-user code breaking before the next staging makes it way to master.) I’ve tried to avoid rebuilds on Linux with my change. It’s narrowly targeted at clang stdenvs and only those using libc++, which Linux does not use by default. Once I confirm Cachix builds, I’ll update this PR. |
Work around clang’s not linking libc++abi when invoked as `clang` or `cc` even when it’s needed to link a C++ library or C++ code. See NixOS#166205.
b0fac4d
to
8164b19
Compare
I pushed the new patch and updated the PR’s title to reflect the change. Cachix successfully built on both aarch64-darwin and x86_64-darwin with just this patch. |
@reckenrode Thanks! This seems reasonable. Maybe @sternenseemann could do a final check, and we could merge in if he signs off on this. |
|
||
# Ensure libc++abi is linked even when clang is invoked as just `clang` or `cc`. | ||
# Works around https://github.com/NixOS/nixpkgs/issues/166205. | ||
# This can be dropped once a fix has been committed to cc-wrapper. |
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.
Has a PR with the proposed fix to cc-wrapper
been opened?
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.
@toonn is working on the fix to cc-wrapper. I don’t believe he’s opened a PR for it yet.
The Hydra log from the top of this issue has been deleted so I can't see the failure. I suspect the root cause is that Darwin's nixpkgs/pkgs/build-support/cc-wrapper/default.nix Lines 528 to 533 in ab5d876
|
As I understand it, Darwin is not
Darwin’s For example, given (and pardon my C++98):
Compiling it with Xcode’s clang:
Which is because it’s ignoring the contents of
If I add |
FYI, binaries are being prepared already, as it seemed likely that this would go ahead soon: |
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.
Seems okay if it stays temporary. It's kind of funny how often something relating to C++ FFI breaks in Haskell exclusively on darwin…
# Works around https://github.com/NixOS/nixpkgs/issues/166205. | ||
# This can be dropped once a fix has been committed to cc-wrapper. | ||
// lib.optionalAttrs (stdenv.cc.isClang && stdenv.cc.libcxx != null) { | ||
env.NIX_LDFLAGS = "-l${stdenv.cc.libcxx.cxxabi.libName}"; |
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.
This completely overwrites env
if it is set. Is probably better to just set NIX_LDFLAGS
top level to prevent confusion if this sticks around for longer.
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. I pushed an update that preserves env
while also avoiding any further rebuilds (since the staging-next job on Hydra is currently pointed to a branch with this commit cherry-picked).
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 opened #266878 to preserve env
since this got merged before I could push the update.
OK, merged like this right now. I suspect this hack will stay in 23.11, but I don't know if that would make it worth to improve this workaround during some other rebuild. |
This is a follow-up PR to NixOS#266172 to address the feedback left by @sternenseemann regarding `env`. Rebuilds on staging-next NixOS#263535 should be limited if there are any.
If we don't check `hasCC`, we run into trouble on pkgsCross.ghcjs, one of the few platforms where `cc` will throw (so `stdenv.cc.isClang or false would` not be enough). Problem introduced in 8164b19 / NixOS#266172.
Description of changes
Fixes failing build on staging-next #263535 plus whatever else was broken in nixpkgs-review.
This is a workaround until cc-wrapper is fixed to link libc++abi even when invoked as just
clang
. Other approaches were investigated (modifyinghaskell-modules/generic-builder.nix
), but they would result in mass rebuilds.Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)