Page MenuHomePhabricator

[cmake] Use `CMAKE_INSTALL_LIBDIR` too
Needs ReviewPublic

Authored by Ericson2314 on Jul 26 2022, 10:25 AM.
Tags
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project

Details

Summary

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 26 2022, 10:25 AM
Ericson2314 requested review of this revision.Jul 26 2022, 10:25 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJul 26 2022, 10:25 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

Forgot to define CLANG_INSTALL_LIBDIR_BASENAME

sebastian-ne added inline comments.Jul 27 2022, 5:38 AM
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 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.

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?
In AddLLVM.cmake, _install_rpath uses ${CMAKE_INSTALL_LIBDIR}.

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,

Ericson2314 marked an inline comment as done.

Fix review comments. Thanks again!

Add deprecation notice

Ericson2314 marked 6 inline comments as done.Jul 27 2022, 8:41 AM
Ericson2314 added inline comments.
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.

compnerd added inline comments.Jul 27 2022, 3:35 PM
clang/include/clang/Config/config.h.cmake
57 ↗(On Diff #448058)

Does this not potentially break downstreams?

sebastian-ne added inline comments.Jul 28 2022, 5:14 AM
clang/CMakeLists.txt
52–53

I think the commit message is a good place.

llvm/CMakeLists.txt
66–67

There’s also message(DEPRECATION …) :)

Ericson2314 added inline comments.Jul 28 2022, 7:41 AM
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. */
ldionne accepted this revision as: Restricted Project, Restricted Project, ldionne.Jul 28 2022, 9:42 AM

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

Ericson2314 marked an inline comment as done.Jul 28 2022, 3:48 PM
Ericson2314 added inline comments.
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!

Ericson2314 marked 2 inline comments as done.Jul 28 2022, 3:49 PM
sebastian-ne accepted this revision.Aug 16 2022, 6:41 AM

Looks good to me, thanks!

utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
70 ↗(On Diff #448459)

This comment is outdated

Ericson2314 marked an inline comment as done.

Rebase, fix outdated comment

Anyone have any idea what this Debian test failure is about?

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!

This revision was not accepted when it landed; it landed in state Needs Review.Aug 18 2022, 12:33 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

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?

thakis added a subscriber: thakis.Aug 19 2022, 4:42 AM

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.

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.

Ericson2314 reopened this revision.Aug 20 2022, 7:25 AM

Rebase; Include d3a1dbc4907b59690f9013cdb6221573ca4233f1 as requested

Thanks @thakis for letting me know.

Rebase, fix some issues (by the looks of it)