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

Chris Bieneman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 10 11:16:32 PST 2017


beanz added inline comments.


================
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')" )
----------------
nalimilan wrote:
> 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.
If you use the GNUInstallDirs module, then it makes sense to keep the `CMAKE_*` prefixes, however GNUInstallDirs may not play well with `LLVM_LIBDIR_SUFFIX`.


================
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}'" )
----------------
nalimilan wrote:
> 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.
I would not include `LLVM_LIBDIR_SUFFIX` in the cached variable and instead include it at the use sites. CMake has no strategy for cache invalidation, so once a variable is cached it is very difficult to make it not cached.


================
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}')" )
----------------
mgorny wrote:
> nalimilan wrote:
> > beanz wrote:
> > > This is also bad for similar reasons as above.
> > Is there a reason to use `${project}` here instead of just `llvm`?
> Yes. Those paths were used in e.g. clang, and there project evaluated to clang. At least for stand-alone builds, that is. 
I would drop `${project}` from the cached variable and have the docs directory add project at the end in each sub-project.


Repository:
  rL LLVM

https://reviews.llvm.org/D28234





More information about the llvm-commits mailing list