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

[John Ericson via Phabricator via libcxx-commits]

Ericson2314 added a comment.

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!



================
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)
----------------
sebastian-ne wrote:
> 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?
Yes exactly.

I really should write up the "rules" that I've (a) discovered from reading (b) invented somewhere. Any idea where?


================
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}"
----------------
sebastian-ne wrote:
> nit: The indentation is wrong
Oops, thanks.


================
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()
----------------
sebastian-ne wrote:
> This is an install directory, so should this be something like
> ```
> extend_path(install_dir "${CMAKE_INSTALL_PREFIX}" "${CMAKE_INSTALL_LIBDIR}")
> ```
> instead?
Indeed, thanks for catching!

`${CMAKE_INSTALL_FULL_LIBDIR}` also does this.


================
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."
----------------
sebastian-ne wrote:
> This is an install path, so should it use `${CMAKE_INSTALL_LIBDIR}/clang/${CLANG_VERSION}` instead?
Yes. Thanks!


================
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
----------------
sebastian-ne wrote:
> 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.
Oh good point, yeah this double `lib` is very tricky I will fix and add comment.


================
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()
----------------
sebastian-ne wrote:
> 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}`.
Yes. I'll do an `extend_path` to handle the absolute path case too.


================
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()
----------------
sebastian-ne wrote:
> Same here as above, the rpath should probably use `${CMAKE_INSTALL_LIBDIR}`?
Agreed, and same extra comment on the fix as above too,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586