[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})
----------------
Ericson2314 wrote:
> compnerd wrote:
> > 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.
> https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/GNUInstallDirs.cmake#L257 Hmm I see what you mean. So you are saying `s/${ CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}/${ CMAKE_INSTALL_LIBDIR}/` everywhere?
Yes, that is what I was referring to.  I'm suggesting that you do *not* make that change instead.  That needs a much more involved change to clean up the use of `${LLVM_LIBDIR_SUFFIX}`.  I think that this change is already extremely large as is, and folding more into it is not going to help.


================
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)
----------------
Ericson2314 wrote:
> compnerd wrote:
> > 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?
> I tried explain this https://reviews.llvm.org/D99484#2655885 and the original description about prefixes. The GNU install dir variables are allowed to be absolute paths (and not necessary with the installation prefix) (and I needed that for the NixOS patch), so always prepending `COMPILER_RT_INSTALL_PATH` as is done doesn't work if that is `CMAKE_INSTALL_PREFIX` by default.
> 
> If you do `git grep '_PREFIX ""'` and also `git grep GRPC_INSTALL_PATH` you will see that many similar variables also were already empty by default. I agree this thing is a bit ugly, but by that argument I am not doing a new hack for `GNUInstallDIrs` but rather applying an existing ideom consistently in all packages.
Sure, but the *descriptions* of the options are needed to changed for changing the value.

That said, I'm not convinced that this change is okay.  It changes the way that compiler-rt can be built and used when building a toolchain image.  The handling of the install prefix in compiler-rt is significantly different from the other parts of LLVM, and so, I think that it may need to be done as a separate larger effort.


================
Comment at: libcxx/CMakeLists.txt:32
+
+include(GNUInstallDirs)
 
----------------
Ericson2314 wrote:
> compnerd wrote:
> > 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.
> It might be fine here. But I was worried that in some of these cases code included in those blocks might refer to the `GNUInstallDirs` variables.
> 
> Originally I had `GNUInstallDirs` only included in the conditional block after `project(...)`, but then the combined build test failed because nothing including `GnuInstallDirs` in that case. If there is an "entrypoint" CMakeLists boilerplate that combined builds should always use, I think the best thing would be to change it back and then additionally put `GNUInstallDirs` there.
Unified builds always use `llvm/CMakeLists.txt`.  However, both should continue to work, and so you will need to add this in the subprojects as well.


================
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
----------------
Ericson2314 wrote:
> compnerd wrote:
> > @ldionne - how is the `LIBCXX_INSTALL_HEADER_PREFIX` used?  Can altering the value of `CMAKE_INSTALL_INCLUDEDIR` serve the purpose?
> It is sometimes modified to be per target when multiple targets are being used at once. All things `CMAKE_INSTALL_*` are globally scoped so in general the combination builds are quite awkward.
> 
> (Having worked on Meson, I am really missing https://mesonbuild.com/Subprojects.html which is exactly what's needed to do this without these bespoke idioms that never work well enough . Alas...)
I don't think that bringing up other build systems is particularly helpful.

I do expect it to be modified, and I suspect that this is used specifically for builds that @ldionne supports.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484