[libcxx-commits] [PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[Sebastian Neubauer via Phabricator via libcxx-commits]

sebastian-ne added inline comments.


================
Comment at: clang/CMakeLists.txt:100
   set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin)
-  set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
+  set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib)
   if(WIN32 OR CYGWIN)
----------------
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?


================
Comment at: clang/runtime/CMakeLists.txt:90-92
+			   -DCMAKE_INSTALL_BINDIR="${CMAKE_INSTALL_BINDIR}"
+			   -DCMAKE_INSTALL_LIBDIR="${CMAKE_INSTALL_LIBDIR}"
+			   -DCMAKE_INSTALL_INCLUDEDIR="${CMAKE_INSTALL_INCLUDEDIR}"
----------------
nit: The indentation is wrong


================
Comment at: compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake:65
       set(output_dir "${LLVM_LIBRARY_OUTPUT_INTDIR}")
-      set(install_dir "${CMAKE_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}")
+      set(install_dir "${CMAKE_INSTALL_PREFIX}/lib")
     else()
----------------
This is an install directory, so should this be something like
```
extend_path(install_dir "${CMAKE_INSTALL_PREFIX}" "${CMAKE_INSTALL_LIBDIR}")
```
instead?


================
Comment at: compiler-rt/cmake/base-config-ix.cmake:48
   set(COMPILER_RT_EXEC_OUTPUT_DIR ${LLVM_RUNTIME_OUTPUT_INTDIR})
-  set(COMPILER_RT_INSTALL_PATH lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION})
+  set(COMPILER_RT_INSTALL_PATH lib/clang/${CLANG_VERSION})
   option(COMPILER_RT_INCLUDE_TESTS "Generate and build compiler-rt unit tests."
----------------
This is an install path, so should it use `${CMAKE_INSTALL_LIBDIR}/clang/${CLANG_VERSION}` instead?


================
Comment at: compiler-rt/cmake/base-config-ix.cmake:103
     ${COMPILER_RT_OUTPUT_DIR}/lib)
-  extend_path(default_install_path "${COMPILER_RT_INSTALL_PATH}" lib)
+  extend_path(default_install_path "${COMPILER_RT_INSTALL_PATH}" "${CMAKE_INSTALL_LIBDIR}")
   set(COMPILER_RT_INSTALL_LIBRARY_DIR "${default_install_path}" CACHE PATH
----------------
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.


================
Comment at: lldb/source/API/CMakeLists.txt:116
 if(LLDB_ENABLE_PYTHON AND (BUILD_SHARED_LIBS OR LLVM_LINK_LLVM_DYLIB) AND UNIX AND NOT APPLE)
-  set_property(TARGET liblldb APPEND PROPERTY INSTALL_RPATH "\$ORIGIN/../../../../lib${LLVM_LIBDIR_SUFFIX}")
+  set_property(TARGET liblldb APPEND PROPERTY INSTALL_RPATH "\$ORIGIN/../../../../lib")
 endif()
----------------
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}`.


================
Comment at: mlir/cmake/modules/AddMLIRPython.cmake:412
     set_property(TARGET ${target} APPEND PROPERTY
-      INSTALL_RPATH "${_origin_prefix}/${ARG_RELATIVE_INSTALL_ROOT}/lib${LLVM_LIBDIR_SUFFIX}")
+      INSTALL_RPATH "${_origin_prefix}/${ARG_RELATIVE_INSTALL_ROOT}/lib")
   endif()
----------------
Same here as above, the rpath should probably use `${CMAKE_INSTALL_LIBDIR}`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586