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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
