[Lldb-commits] [PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.


================
Comment at: clang/cmake/modules/AddClang.cmake:127
+          LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+          ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+          RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})
----------------
For the initial change, Id leave this off.  `CMAKE_INSTALL_LIBDIR` sometimes already deals with the bit suffix, so you can end up with two instances of `32` or `64`.  I think that cleaning that up separately, while focusing on the details of cleaning up how to handle `LLVM_LIBDIR_SUFFIX` is the right thing to do.  The same applies to the rest of the patch.


================
Comment at: compiler-rt/cmake/base-config-ix.cmake:72
+  set(COMPILER_RT_INSTALL_PATH "" CACHE PATH
+    "Prefix where built compiler-rt artifacts should be installed, comes before CMAKE_INSTALL_PREFIX.")
   option(COMPILER_RT_INCLUDE_TESTS "Generate and build compiler-rt unit tests." OFF)
----------------
Please don't change the descriptions of the options as part of the `GNUInstallDirs` handling.  The change to `COMPILER_RT_INSTALL_PATH` looks incorrect.  Can you explain this change please?


================
Comment at: libcxx/CMakeLists.txt:32
+
+include(GNUInstallDirs)
 
----------------
Does this need to come here?  Why not push this to after the if block completes?  The same applies through out the rest of the patch.


================
Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:66
           install(FILES "${LIBCXX_BINARY_INCLUDE_DIR}/${fpath}"
-            DESTINATION ${LIBCXX_INSTALL_HEADER_PREFIX}include/c++/v1/${dstdir}
+            DESTINATION ${LIBCXX_INSTALL_HEADER_PREFIX}${CMAKE_INSTALL_INCLUDEDIR}/c++/v1/${dstdir}
             COMPONENT cxx-headers
----------------
@ldionne - how is the `LIBCXX_INSTALL_HEADER_PREFIX` used?  Can altering the value of `CMAKE_INSTALL_INCLUDEDIR` serve the purpose?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484