We held off on this before as LLVM_LIBDIR_SUFFIX conflicted with it.
Now we return this.
LLVM_LIBDIR_SUFFIX is kept as a deprecated way to set
CMAKE_INSTALL_LIBDIR. The other *_LIBDIR_SUFFIX are just removed
entirely.
I imagine this is too potentially-breaking to make LLVM 15. That's fine.
I have a more minimal version of this in the disto (NixOS) patches for
LLVM 15 (like previous versions). This more expansive version I will
test harder after the release is cut.
Forgot to define CLANG_INSTALL_LIBDIR_BASENAME
Just to check if your intention aligns with my understanding, removing the suffix here is fine because the destination is in the binary dir and not the final install destination?
nit: The indentation is wrong
This is an install directory, so should this be something like
extend_path(install_dir "${CMAKE_INSTALL_PREFIX}" "${CMAKE_INSTALL_LIBDIR}")
instead?
This is an install path, so should it use ${CMAKE_INSTALL_LIBDIR}/clang/${CLANG_VERSION} instead?
What is the result we expect here?
In case that CMAKE_INSTALL_LIBDIR is defined as lib64, this path will change.
In some cases it would have been lib64/clang/14.0.0/lib,
but with this patch it would be lib/clang/14.0.0/lib64 if I understand correctly.
It looks to me like this path is used for installation, so should it have the (potential) suffix?
In AddLLVM.cmake, _install_rpath uses ${CMAKE_INSTALL_LIBDIR}.
Same here as above, the rpath should probably use ${CMAKE_INSTALL_LIBDIR}?
Thanks so much @sebastian-ne for the thorough review!
Unlike the other patches rather than being cleaned up code that needs more testing, this is closer to the original patch we've been using for a while (tested but not cleaned up); sorry there were so many things to catch!
Yes exactly.
I really should write up the "rules" that I've (a) discovered from reading (b) invented somewhere. Any idea where?
Oops, thanks.
Indeed, thanks for catching!
${CMAKE_INSTALL_FULL_LIBDIR} also does this.
Yes. Thanks!
Oh good point, yeah this double lib is very tricky I will fix and add comment.
Yes. I'll do an extend_path to handle the absolute path case too.
Agreed, and same extra comment on the fix as above too,
Fix review comments. Thanks again!
Add deprecation notice
Actually, it will do the CMAKE_INSTALL_PREFIX part by default (and the other branch is already a potentially relative path) so just ${CMAKE_INSTALL_LIBDIR} will suffice.
Does this not potentially break downstreams?
I think the commit message is a good place.
There’s also message(DEPRECATION …) :)
At the top of this file it says
/* This generated file is for internal use. Do not include it from headers. */
I'm happy with this from the libc++/libc++abi side of things. You can ignore the failing CI job, it's been addressed.
WARNING -> DEPRECATION in message call
Rebase so libcxx and friends are hopefully fixed
Mmm, my philosophy is that commit messages should document relationships between the code, but the code in isolation should be documented in-tree.
I wouldn't want one to have to git log to find this information which should be a "living reference" of the current CMake idioms as currently practiced, make sense?
Oh nice!
Looks good to me, thanks!
This comment is outdated
Rebase, fix outdated comment
Anyone have any idea what this Debian test failure is about?
In D130586#3731021, @Ericson2314 wrote:
Anyone have any idea what this Debian test failure is about?
I haven’t seen a passing debian pre-merge check for months now, so I usually ignore it (the failures seems to be related to debuginfod currently).
The bazel failure seems to be a bug or flake in the build system as well.
OK I was worried the debuginfod one was unique to this PR; glad to hear it isn't. I'll give it a shot then, thanks!
I can no longer do a two stage build on Fedora after this change.
$ cmake \
-B build/stage1 \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_C_COMPILER=$(command -v clang) \
-DCMAKE_CXX_COMPILER=$(command -v clang++) \
-DLLVM_ENABLE_PROJECTS="clang;lld" \
-DLLVM_ENABLE_TERMINFO=OFF \
-DLLVM_TARGETS_TO_BUILD=host \
-DLLVM_USE_LINKER=$(command -v ld.lld) \
-G Ninja \
-S llvm
...
$ ninja -C build/stage1
...
$ cmake \
-B build/stage2 \
-DCLANG_TABLEGEN=$PWD/build/stage1/bin/clang-tblgen \
-DCMAKE_AR=$PWD/build/stage1/bin/llvm-ar \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_C_COMPILER=$PWD/build/stage1/bin/clang \
-DCMAKE_CXX_COMPILER=$PWD/build/stage1/bin/clang++ \
-DCMAKE_RANLIB=$PWD/build/stage1/bin/llvm-ranlib \
-DLLVM_ENABLE_PROJECTS="clang;lld" \
-DLLVM_ENABLE_TERMINFO=OFF \
-DLLVM_TABLEGEN=$PWD/build/stage1/bin/llvm-tblgen \
-DLLVM_TARGETS_TO_BUILD=host \
-DLLVM_USE_LINKER=$PWD/build/stage1/bin/ld.lld \
-G Ninja \
-S llvm
...
-- Performing Test LLVM_LIBSTDCXX_MIN
-- Performing Test LLVM_LIBSTDCXX_MIN - Failed
CMake Error at cmake/modules/CheckCompilerVersion.cmake:88 (message):
libstdc++ version must be at least 7.1.
Call Stack (most recent call first):
cmake/config-ix.cmake:15 (include)
CMakeLists.txt:810 (include)
...
$ cat build/stage2/CMakeFiles/CMakeError.log
Checking whether the ASM compiler is GNU using "--version" did not match "(GNU assembler)|(GCC)|(Free Software Foundation)":
clang version 16.0.0 ( f7a33090a91015836497c75f173775392ab0304d)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /home/nathan/cbl/src/llvm-project/build/stage1/bin
Performing C++ SOURCE FILE Test LLVM_LIBSTDCXX_MIN failed with the following output:
Change Dir: /home/nathan/cbl/src/llvm-project/build/stage2/CMakeFiles/CMakeTmp
Run Build Command(s):/usr/bin/ninja-build cmTC_caa1d && [1/2] Building CXX object CMakeFiles/cmTC_caa1d.dir/src.cxx.o
FAILED: CMakeFiles/cmTC_caa1d.dir/src.cxx.o
/home/nathan/cbl/src/llvm-project/build/stage1/bin/clang++ -DLLVM_LIBSTDCXX_MIN -std=c++0x -std=c++17 -MD -MT CMakeFiles/cmTC_caa1d.dir/src.cxx.o -MF CMakeFiles/cmTC_caa1d.dir/src.cxx.o.d -o CMakeFiles/cmTC_caa1d.dir/src.cxx.o -c /home/nathan/cbl/src/llvm-project/build/stage2/CMakeFiles/CMakeTmp/src.cxx
In file included from /home/nathan/cbl/src/llvm-project/build/stage2/CMakeFiles/CMakeTmp/src.cxx:2:
In file included from /usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/iosfwd:40:
In file included from /usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/postypes.h:40:
In file included from /usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/cwchar:44:
/usr/include/wchar.h:35:10: fatal error: 'stddef.h' file not found
#include <stddef.h>
^~~~~~~~~~
1 error generated.
ninja: build stopped: subcommand failed.
Source file was:
#include <iosfwd>
#if defined(__GLIBCXX__)
#if !defined(_GLIBCXX_RELEASE) || _GLIBCXX_RELEASE < 7
#error Unsupported libstdc++ version
#endif
#endif
int main() { return 0; }
$ fd stddef.h build/stage1
build/stage1/lib/clang/16.0.0/include/stddef.h
The parent commit is fine. If there is any further information I can provide or patches I can test, please let me know!
I seem to be unable to pass check-clang after this. A lot of tests fail because they can't find headers they need.
This is breaking our build setup. It causes e.g. the Clang resource headers to be installed to lib64/clang instead of lib/clang. Given this and the other breakages, can we revert for now?
Sorry. I have reverted this. I see why the lib and lib64 mixup could be caused by this, but I am baffled how the missing headers could be. Headers search paths should be unaffected?
When this relands and it's not too much trouble, it'd be nice if you could reland d3a1dbc4907b59690f9013cdb6221573ca4233f1 in the commit that relands this.
In D130586#3734012, @Ericson2314 wrote:
Sorry. I have reverted this. I see why the lib and lib64 mixup could be caused by this, but I am baffled how the missing headers could be. Headers search paths should be unaffected?
I think there may be code somewhere that computes header paths as relative to the lib paths.
Rebase; Include d3a1dbc4907b59690f9013cdb6221573ca4233f1 as requested
Thanks @thakis for letting me know.
Rebase, fix some issues (by the looks of it)