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

[John Ericson via Phabricator via libcxx-commits]

Ericson2314 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})
----------------
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?


================
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)
----------------
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.


================
Comment at: libcxx/CMakeLists.txt:32
+
+include(GNUInstallDirs)
 
----------------
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.


================
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
----------------
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...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484