[libcxx-commits] [PATCH] D89041: [libc++] Include <__config_site> from <__config>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.



In D89041#2345765 <https://reviews.llvm.org/D89041#2345765>, @vitalybuka wrote:

> Removing the dir does not help for my build:
>
> cmake -GNinja ../../llvm-project/llvm -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;compiler-rt;lld;libcxx;libcxxabi" -DLLVM_ENABLE_LLD=ON -DCMAKE_C_COMPILER=/usr/local/google/home/vitalybuka/src/clang/bin/clang -DCMAKE_CXX_COMPILER=/usr/local/google/home/vitalybuka/src/clang/bin/clang++  -DLLVM_BINUTILS_INCDIR=/usr/include -DCMAKE_BUILD_TYPE=Release
> ninja check-all
>
>   [...]
>
> it's on cd4a4ae97a7ccfa381e06936cd0981cb7d978ec1 <https://reviews.llvm.org/rGcd4a4ae97a7ccfa381e06936cd0981cb7d978ec1>

I think it's because of

  set(MSAN_UNITTEST_COMMON_CFLAGS
    -nostdinc++
    -isystem ${COMPILER_RT_LIBCXX_PATH}/include

in `compiler-rt/lib/msan/tests/CMakeLists.txt`. Other compiler-rt tests don't pass any custom libc++ path, and they rely on the fact that they can find it alongside Clang in the build directory (which, funnily enough, is exactly what 69c2087283cf7b17ca75f69daebf4ffc158b754a <https://reviews.llvm.org/rG69c2087283cf7b17ca75f69daebf4ffc158b754a> fixed. Possible fix in https://reviews.llvm.org/D89915, I pinged you there.

In D89041#2345632 <https://reviews.llvm.org/D89041#2345632>, @mstorsjo wrote:

> This seems to have broken my builds of libcxxabi+libcxx. I'm building them standalone (i.e. configuring each of libunwind, libcxxabi and libcxx individually with cmake), and I've used to set `LIBCXXABI_LIBCXX_INCLUDES` to point at the libcxx include dir, but that cmake option is removed now. I presume the `cxx-headers` dependency is supposed to take care of that, but that only works when building them all with one cmake invocation (part of full llvm build, or part of runtimes build)?
>
> Could we keep `LIBCXXABI_LIBCXX_INCLUDES` (in order not to break existing scripts), or what's the best way forward here?

I've started looking at this one -- I'll have to resume tomorrow.

This change really showcases everything worse about libc++'s build system today:

1. There are many users of libc++, and there's unfortunately no single standardized way of taking a dependency on libc++ at the CMake level. As a result, some projects expect it to be magically placed in some location, other projects expect to be able to include the libc++ headers in the source tree (which used to work, but has always been incorrect cause you were skipping the `<__config_site>`, which could have up to ABI breaking implications), others manually add an include path to a location where the libc++ headers have been installed properly, and others link against the `cxx-headers` target in CMake. Only the two last ones are intended to be supported.
2. There's too many ways of building libc++. There's the Standalone build, the runtimes build and the monorepo build. Not all of them are tested automatically. We ordinarily only test the standard monorepo build in our test bots, hence the break discovered by @mstorsjo
3. Even within the libc++ and libc++abi build itself, there are violations of that. Also, we don't run all targets all the time, so for example this change broke the `cxx-benchmarks` target cause it didn't run by default when I did the tests. Running it all the time is too slow, so it's excluded by default.

I'll pick this up tomorrow morning first thing, but I'll be looking to standardize a single way of building libc++ that doesn't involve building all the rest of LLVM (so it should satisfy the standalone build users requirements).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89041/new/

https://reviews.llvm.org/D89041