Skip to content
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.

Sign up for GitHub

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

llvmPackages_13.libcxx: fix darwin build #147289

Merged
merged 3 commits into from
Nov 26, 2021
Merged

llvmPackages_13.libcxx: fix darwin build #147289

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e157b5d
llvmPackages_13.libcxx: fix darwin build
midchildan Nov 22, 2021
d8338cb
fixup! llvmPackages_13.libcxx: fix darwin build
midchildan Nov 25, 2021
7994b1d
fixup! llvmPackages_13.libcxx: fix darwin build
midchildan Nov 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions pkgs/development/compilers/llvm/13/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,18 @@ let
else stdenv;
};

libcxxabi = callPackage ./libcxxabi {
inherit llvm_meta;
stdenv = if stdenv.hostPlatform.useLLVM or false
libcxxabi = let
stdenv_ = if stdenv.hostPlatform.useLLVM or false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what you want to do here is test stdenv.cc.isClang. useLLVM isn't true for Darwin, for example. (Unless that changed recently?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure what the intention behind that conditional expression is, so I left it unchanged in this PR. Does your suggestion apply to other parts of the code too? I see the same conditional expression used everywhere throughout the LLVM packaging code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I guess leave it if you've tested it on Darwin anyway?

then overrideCC stdenv buildLlvmTools.clangNoLibcxx
else stdenv;
cxx-headers = callPackage ./libcxx {
inherit llvm_meta;
stdenv = stdenv_;
isCxxHeaders = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen headersOnly as the argument for triggering a headers only build a couple times, might be nice to align with that convention?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now changed it to headersOnly.

};
in callPackage ./libcxxabi {
stdenv = stdenv_;
inherit llvm_meta cxx-headers;
};

libunwind = callPackage ./libunwind {
Expand Down
37 changes: 28 additions & 9 deletions pkgs/development/compilers/llvm/13/libcxx/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,23 @@
{ lib, stdenv, llvm_meta, src, cmake, python3, fixDarwinDylibNames, version
, libcxxabi
, enableShared ? !stdenv.hostPlatform.isStatic

# If isCxxHeaders is true, the resulting package would only include the headers.
# Use this to break the circular dependency between libcxx and libcxxabi.
#
# Some context:
# https://reviews.llvm.org/rG1687f2bbe2e2aaa092f942d4a97d41fad43eedfb
, isCxxHeaders ? false
}:

stdenv.mkDerivation rec {
pname = "libcxx";
pname = if isCxxHeaders then "cxx-headers" else "libcxx";
inherit version;

inherit src;
sourceRoot = "source/${pname}";
sourceRoot = "source/libcxx";

outputs = [ "out" "dev" ];
outputs = [ "out" ] ++ lib.optional (!isCxxHeaders) "dev";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be simpler if dev would be the single output when only building the headers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, but it failed to evaluate when I did this.

❯ nix-build -A llvmPackages_13.libcxx
these 3 derivations will be built:
  /nix/store/9za4wnq6g4kzls8kda2lma2axhppgis4-cxx-headers-13.0.0.drv
  /nix/store/yhfjvzqrz6d1rs8d7mfifsvm4i6c0rdd-libcxxabi-13.0.0.drv
  /nix/store/2c18dsqr57z2sl6bw0y85ziz44349jn7-libcxx-13.0.0.drv
building '/nix/store/9za4wnq6g4kzls8kda2lma2axhppgis4-cxx-headers-13.0.0.drv'...
Error: _assignFirst found no valid variant!


patches = [
./gnu-install-dirs.patch
Expand All @@ -24,15 +32,29 @@ stdenv.mkDerivation rec {
nativeBuildInputs = [ cmake python3 ]
++ lib.optional stdenv.isDarwin fixDarwinDylibNames;

cmakeFlags = [
] ++ lib.optional (stdenv.hostPlatform.isMusl || stdenv.hostPlatform.isWasi) "-DLIBCXX_HAS_MUSL_LIBC=1"
buildInputs = lib.optional (!isCxxHeaders) [ libcxxabi ];
midchildan marked this conversation as resolved.
Show resolved Hide resolved

cmakeFlags = [ "-DLIBCXX_CXX_ABI=libcxxabi" ]
++ lib.optional (stdenv.hostPlatform.isMusl || stdenv.hostPlatform.isWasi) "-DLIBCXX_HAS_MUSL_LIBC=1"
++ lib.optional (stdenv.hostPlatform.useLLVM or false) "-DLIBCXX_USE_COMPILER_RT=ON"
++ lib.optional stdenv.hostPlatform.isWasm [
++ lib.optionals stdenv.hostPlatform.isWasm [
"-DLIBCXX_ENABLE_THREADS=OFF"
"-DLIBCXX_ENABLE_FILESYSTEM=OFF"
"-DLIBCXX_ENABLE_EXCEPTIONS=OFF"
] ++ lib.optional (!enableShared) "-DLIBCXX_ENABLE_SHARED=OFF";

buildFlags = lib.optional isCxxHeaders "generate-cxx-headers";
installTargets = lib.optional isCxxHeaders "install-cxx-headers";

# At this point, cxxabi headers would be installed in the dev output, which
# prevents moveToOutput from doing its job later in the build process.
postInstall = lib.optionalString (!isCxxHeaders) ''
mv "$dev/include/c++/v1/"* "$out/include/c++/v1/"
pushd "$dev"
rmdir -p include/c++/v1
popd
Comment on lines +53 to +55
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pushd "$dev"
rmdir -p include/c++/v1
popd
rmdir -p $dev/include/c++/v1

Copy link
Member Author

@midchildan midchildan Nov 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would result in different behavior. I want the directories v1, c++, and include to be deleted in order while making sure that each of those directories are completely empty before deletion. The suggested change would also attempt to delete each path component in $dev too.

'';

passthru = {
isLLVM = true;
};
Expand All @@ -45,9 +67,6 @@ stdenv.mkDerivation rec {
C++14 and above.
'';

# https://github.com/NixOS/nixpkgs/pull/133217#issuecomment-895742807
broken = stdenv.isDarwin;

# "All of the code in libc++ is dual licensed under the MIT license and the
# UIUC License (a BSD-like license)":
license = with lib.licenses; [ mit ncsa ];
Expand Down
4 changes: 2 additions & 2 deletions pkgs/development/compilers/llvm/13/libcxxabi/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{ lib, stdenv, llvm_meta, cmake, python3, src, libunwind, libcxx, version
{ lib, stdenv, llvm_meta, cmake, python3, src, cxx-headers, libunwind, version
, enableShared ? !stdenv.hostPlatform.isStatic
, standalone ? stdenv.hostPlatform.useLLVM or false
, withLibunwind ? !stdenv.isDarwin && !stdenv.isFreeBSD && !stdenv.hostPlatform.isWasm
Expand Down Expand Up @@ -27,7 +27,7 @@ stdenv.mkDerivation rec {
buildInputs = lib.optional withLibunwind libunwind;

cmakeFlags = [
"-DLIBCXXABI_LIBCXX_INCLUDES=${libcxx.dev}/include/c++/v1"
"-DLIBCXXABI_LIBCXX_INCLUDES=${cxx-headers}/include/c++/v1"
] ++ lib.optionals standalone [
"-DLLVM_ENABLE_LIBCXX=ON"
] ++ lib.optionals (standalone && withLibunwind) [
Expand Down
2 changes: 2 additions & 0 deletions pkgs/top-level/all-packages.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12578,6 +12578,8 @@ with pkgs;
targetLlvmLibraries = targetPackages.llvmPackages_13.libraries;
} // lib.optionalAttrs (stdenv.hostPlatform.isi686 && buildPackages.stdenv.cc.isGNU) {
stdenv = gcc7Stdenv;
} // lib.optionalAttrs stdenv.hostPlatform.isDarwin {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the LLVM bump to 11 #126411 was just merged to staging so I don't think it's necessary to explicitly pick the LLVM version here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this change in the latest commit.

stdenv = llvmPackages_11.stdenv;
}));

llvmPackages_latest = llvmPackages_13;
Expand Down