Replace LLVM_LIBDIR_SUFFIX by CMAKE_INSTALL_LIBDIR

[serge-sans-paille]

Summary

There are a few advantages of using CMAKE_INSTALL_LIBDIR in place of
LLVM_LIBDIR_SUFFIX:

1. it's a standard flag, no need to support an extra cmake configuration
2. it has sane defaults, and picks lib64 instead of lib on some systems (including Fedora)

[sylvestre.ledru]

Maybe add it to the release notes?

[serge-sans-paille]

+ Release Note

[mgorny]

bolt/lib/RuntimeLibs/RuntimeLibrary.cpp

32

Well, one thing I immediately notice is that technically CMAKE_INSTALL_LIBDIR can be an absolute path, while in many locations you are assuming it will always be relative. Not saying that's a blocker but I think you should add checks for that into CMakeLists.txt.

clang/tools/libclang/CMakeLists.txt

234

You seem to be changing indentation here, and below.

On unrelated note, hardcoding site-packages path sounds like a bad idea but that's a problem for another patch.

[tianshilei1992]

Can you verify if projects enabled by LLVM_ENABLE_RUNTIMES (not LLVM_ENABLE_PROJECTS) will still be installed properly, aka in LLVM's lib directory?

[Ericson2314]

bolt/lib/RuntimeLibs/RuntimeLibrary.cpp

32

Yes I was working on this, and I intend to use CMAKE_INSTALL_LIBDIR with an absolute path.

OK if this isn't supported initially but we should ovoid regressing where possible.

clang/include/clang/Config/config.h.cmake

39

I had called it _BASENAME in anticipation that it would be the basename of CMAKE_INSTALL_LIBDIR eventually when CMAKE_INSTALL_LIBDIR is not a relative path.

[serge-sans-paille]

bolt/lib/RuntimeLibs/RuntimeLibrary.cpp

32

Turns out LLVM_LIBDIR_SUFFIX is used in several situations: (a) for the library install dir or (b) for the location where build artifact as stored in CMAKE_BINARY_DIR. (a) could accept a path but not (b), unless we derive it from (a) but I can see a lot of complication there for the testing step. Would that be ok to choke on CMAKE_INSTALL_LIBDIR being a path and not a directory component?

[compnerd]

bolt/lib/RuntimeLibs/RuntimeLibrary.cpp

32

I think that is absolutely reasonable. There is the CMAKE_INSTALL_FULL_LIBDIR which should be the relatively absolute path (it is relative to DESTDIR). The CMAKE_INSTALL_LIBDIR should be the relative component which is added to CMAKE_INSTALL_PREFIX.

[Ericson2314]

bolt/lib/RuntimeLibs/RuntimeLibrary.cpp

32

Please do not do this. In NixOS we uses absolute paths for these which are *not* within CMAKE_INSTALL_PREFIX and I would very much like that to continue to work, and have put a lot of effort into it.

(I am sorry I have been a bit AWAL on LLVM things in general but hopefully will have more time as we head into the new year.)

[compnerd]

bolt/lib/RuntimeLibs/RuntimeLibrary.cpp

32

Why can NixOS not use CMAKE_INSTALL_FULL_LIBDIR? That is computed to ${CMAKE_INSTALL_PREFIX}${CMAKE_INSTALL_LIBDIR} only if it is not already defined to an absolute path.

[barannikov88]

bolt/include/bolt/RuntimeLibs/RuntimeLibraryVariables.inc.in

17

The prefix must remain LLVM_*
In other components accordingly

[barannikov88]

bolt/include/bolt/RuntimeLibs/RuntimeLibraryVariables.inc.in

17

The prefix must remain LLVM_*
In other components accordingly

To clarify, I mean the C++ macro name and not CMake variable name