LLVM_LIBDIR_SUFFIX is kept as a deprecated way to set
CMAKE_INSTALL_LIBDIR. The other *_LIBDIR_SUFFIX are just removed
entirely.
Details
- Reviewers
sebastian-ne beanz compnerd phosek bollu ldionne sscalpone rafauler Amir maksfb NoQ jdoerfert lebedev.ri - Group Reviewers
Restricted Project Restricted Project Restricted Project - Commits
- rGf7a33090a910: [cmake] Use `CMAKE_INSTALL_LIBDIR` too
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/CMakeLists.txt | ||
---|---|---|
52–53 | 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? | |
clang/runtime/CMakeLists.txt | ||
90–92 | nit: The indentation is wrong | |
compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake | ||
65 | This is an install directory, so should this be something like extend_path(install_dir "${CMAKE_INSTALL_PREFIX}" "${CMAKE_INSTALL_LIBDIR}") instead? | |
compiler-rt/cmake/base-config-ix.cmake | ||
48 | This is an install path, so should it use ${CMAKE_INSTALL_LIBDIR}/clang/${CLANG_VERSION} instead? | |
103–104 | What is the result we expect here? In some cases it would have been lib64/clang/14.0.0/lib, | |
lldb/source/API/CMakeLists.txt | ||
119–120 | It looks to me like this path is used for installation, so should it have the (potential) suffix? | |
mlir/cmake/modules/AddMLIRPython.cmake | ||
411–416 | 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!
clang/CMakeLists.txt | ||
---|---|---|
52–53 | Yes exactly. I really should write up the "rules" that I've (a) discovered from reading (b) invented somewhere. Any idea where? | |
clang/runtime/CMakeLists.txt | ||
90–92 | Oops, thanks. | |
compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake | ||
65 | Indeed, thanks for catching! ${CMAKE_INSTALL_FULL_LIBDIR} also does this. | |
compiler-rt/cmake/base-config-ix.cmake | ||
48 | Yes. Thanks! | |
103–104 | Oh good point, yeah this double lib is very tricky I will fix and add comment. | |
lldb/source/API/CMakeLists.txt | ||
119–120 | Yes. I'll do an extend_path to handle the absolute path case too. | |
mlir/cmake/modules/AddMLIRPython.cmake | ||
411–416 | Agreed, and same extra comment on the fix as above too, |
compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake | ||
---|---|---|
65 | 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. |
clang/include/clang/Config/config.h.cmake | ||
---|---|---|
57 ↗ | (On Diff #448058) | Does this not potentially break downstreams? |
clang/include/clang/Config/config.h.cmake | ||
---|---|---|
57 ↗ | (On Diff #448058) | 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.
clang/CMakeLists.txt | ||
---|---|---|
52–53 | 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? | |
llvm/CMakeLists.txt | ||
66–67 | Oh nice! |
Looks good to me, thanks!
utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h | ||
---|---|---|
70 ↗ | (On Diff #448459) | This comment is outdated |
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 (https://github.com/llvm/llvm-project 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.
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.
We held off on this before as LLVM_LIBDIR_SUFFIX conflicted with it. Now we return this.
I imagine this is too potentially-breaking to make LLVM 15. That's fine. ...
These sentences are no longer relevant and should be removed.
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?