-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
libcxxabi: don't depend on libunwind #70321
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.
nix-review
passes on NixOS (1 broken on master)
diff LGTM
executables still seem to work across most of the packages
[26 built (1 failed), 83 copied (1550.3 MiB), 492.3 MiB DL]
error: build of '/nix/store/8373j21hf419f6xbjwyxb0v73yhzk9ji-env.drv' failed
https://github.com/NixOS/nixpkgs/pull/70321
6 package are marked as broken and were skipped:
clang-sierraHack clang-sierraHack-stdenv darwin.binutils darwin.binutils-unwrapped darwin.cctools darwin.maloader
1 package failed to build:
irods-icommands
16 package were build:
cquery discord discord-canary discord-ptb gitter irods jucipp libcxx libcxxStdenv libcxxabi llvmPackages.libcxxClang python27Packages.pybindgen python27Packages.pygccxml python37Packages.pygccxml sbt-with-scala-native teamspeak_client
no change in closure size, so probably wasn't being used
|
@GrahamcOfBorg build libcxxabi |
Right, and regarding the one failure of nix-review:
Let's merge! |
Errmm.... :(
What motivated this? (something on Darwin, I guess?)
Is there a reason to change this for LLVM 7 in particular?
(should other versions be changed?)
libc++abi most certainly can make use of LLVM's unwinder,
take a look at the source if you're curious/interested.
Perhaps we were failing to configure the build system properly?
Is this removal useful in any observable way (fixes something?)
or was it suggested as a cleanup after noticing it not having
the intended behavior?
Managing to convince all the LLVM bits to work properly and
with sane configuration is a bit difficult when building each
component individually...
The real question is whether it's desired/useful to use
LLVM's libunwind on each platform, and then make changes
to ensure that we're building things accordingly.
Until then it is unclear if the issue is we shouldn't
be trying to use LLVM's unwinder (as this PR assumes)
or if we /should/ be using LLVM's unwinder but are failing
to do so (for whatever reason).
Anyone have input/experience re:what Nixpkgs's libc++abi unwinder "should" be?
(perhaps platform-specific?)
Also: https://clang.llvm.org/docs/Toolchain.html#unwind-library
One additional reason to prefer LLVM's unwinder: producing toolchain
free of GCC bits, an especially important accomplishment given Nixpkgs'
bug regarding ancient libgcc_s from bootstrap propagating everywhere
(but even if that were fixed, a gcc-free LLVM is a popular goal for
other reasons, and something I've certainly found useful).
…On Fri, 04 Oct 2019 01:17:21 -0700, Bas van Dijk ***@***.***> wrote:
Merged #70321 into master.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#70321 (comment) part: text/html
|
@dtzWill feel free to revert. I fixed libunwind when trying to fix the static builds for libcxx and libcxxabi which depend on it; when I figured out neither actually needed libunwind I decided to save someone else's time who might try to fix libunwind first. In the end I'm not using any of these and use llvm's libunwind directly. |
Motivation for this change
libcxxabi doesn't need libunwind
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)nix-review
fails onidros-icommands
on Linux but that fails on master as well. Otherwise all green.Notify maintainers
cc @dtzWill @basvandijk