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

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.


================
Comment at: llvm/CMakeLists.txt:589
     CACHE STRING "Doxygen-generated HTML documentation install directory")
-set(LLVM_INSTALL_OCAMLDOC_HTML_DIR "share/doc/llvm/ocaml-html"
+set(LLVM_INSTALL_OCAMLDOC_HTML_DIR "${CMAKE_INSTALL_DOCDIR}/${project}/ocaml-html"
     CACHE STRING "OCamldoc-generated HTML documentation install directory")
----------------
Why the change from `llvm` -> `${project}`?  (not that it really makes a difference)


================
Comment at: llvm/cmake/modules/AddSphinxTarget.cmake:77
         if (CMAKE_INSTALL_MANDIR)
-          set(INSTALL_MANDIR ${CMAKE_INSTALL_MANDIR}/)
+          set(INSTALL_MANDIR "${CMAKE_INSTALL_MANDIR}/")
         else()
----------------
Nit: trailing slash is unnecessary, `CMAKE_INSTALL_MANDIR` should be defined, and if not, you do not want installation into `/` anyway.


================
Comment at: llvm/cmake/modules/CMakeLists.txt:3
 
-set(LLVM_INSTALL_PACKAGE_DIR lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm)
+set(LLVM_INSTALL_PACKAGE_DIR lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm CACHE STRING "Path for CMake subdirectory (defaults to 'cmake/llvm')")
 set(llvm_cmake_builddir "${LLVM_BINARY_DIR}/${LLVM_INSTALL_PACKAGE_DIR}")
----------------
Why is this variable being put into the cache now?


================
Comment at: llvm/cmake/modules/LLVMInstallSymlink.cmake:7
   set(DESTDIR $ENV{DESTDIR})
-  set(bindir "${DESTDIR}${CMAKE_INSTALL_PREFIX}/${outdir}/")
+  set(bindir "${DESTDIR}${outdir}/")
 
----------------
Nit: trailing slash shouldn't be there.


================
Comment at: llvm/tools/llvm-config/llvm-config.cpp:361
+    {
+      SmallString<256> Path(StringRef(LLVM_INSTALL_INCLUDEDIR));
+      sys::fs::make_absolute(ActivePrefix, Path);
----------------
Why the temporary `StringRef`?  Can you not just initialize `Path` with the literal?


================
Comment at: llvm/tools/llvm-config/llvm-config.cpp:366
+    {
+      SmallString<256> Path(StringRef(LLVM_INSTALL_BINDIR));
+      sys::fs::make_absolute(ActivePrefix, Path);
----------------
Similar


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100810