[PATCH] D28234: Support for custom install dirs in CMake build

[Milan Bouchet-Valat via Phabricator via llvm-commits]

nalimilan added a comment.

Thanks for the review!



================
Comment at: CMakeLists.txt:299
+    CACHE STRING "Define suffix of library directory name (32/64)" )
+set(CMAKE_INSTALL_BINDIR bin
+    CACHE STRING "Path for binary directory relative to CMAKE_INSTALL_PREFIX (defaults to 'bin')" )
----------------
mgorny wrote:
> beanz wrote:
> > In keeping with our conventions these variables probably should start with `LLVM` instead of `CMAKE`.
> And anyway, why were you reinventing GNUInstallDirs? If we want the CMAKE* vars, I don't see a reason not to use the module completely. 
I'm not that familiar with CMake, so I don't really know how modules work.I thought I would propose the less invasive patch for a start.  But I can use GNUInstallDirs if you think that's the best approach.


================
Comment at: CMakeLists.txt:301
+    CACHE STRING "Path for binary directory relative to CMAKE_INSTALL_PREFIX (defaults to 'bin')" )
+set(CMAKE_INSTALL_LIBDIR lib${LLVM_LIBDIR_SUFFIX}
+    CACHE STRING "Path for library directory relative to CMAKE_INSTALL_PREFIX (defaults to 'lib${LLVM_LIBDIR_SUFFIX}'" )
----------------
beanz wrote:
> Generally speaking having a CMake cache variable that is derived from the value of another cache variable is really bad. If you update `LLVM_LIBDIR_SUFFIX` in this case `CMAKE_INSTALL_LIBDIR` will not be updated.
I see, but what should I do here? Drop `LLVM_LIBDIR_SUFFIX`? Stop caching it? Same for the other uses, I'm not sure what's the best approach.


================
Comment at: CMakeLists.txt:305
+    CACHE STRING "Path for include directory relative to CMAKE_INSTALL_PREFIX (defaults to 'include'" )
+set(CMAKE_INSTALL_DOCDIR share/doc/${project}
+    CACHE STRING "Path for documentation directory relative to CMAKE_INSTALL_PREFIX (defaults to 'share/doc/${project}')" )
----------------
beanz wrote:
> This is also bad for similar reasons as above.
Is there a reason to use `${project}` here instead of just `llvm`?


Repository:
  rL LLVM

https://reviews.llvm.org/D28234