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

[John Ericson via Phabricator via llvm-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:
> 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.
So you are saying II should back all of these out into `lib${LLVM_LIBDIR_SUFFIX}` as they were before, for now?

Yes I don't want to make this bigger either, and would rather be on the hook for follow-up work than have this one be too massive to get over the finish line.


================
Comment at: compiler-rt/cmake/Modules/CompilerRTUtils.cmake:389
     get_compiler_rt_target(${arch} target)
-    set(${install_dir} ${COMPILER_RT_INSTALL_PATH}/lib/${target} PARENT_SCOPE)
+    set(${install_dir} ${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_FULL_LIBDIR}/${target} PARENT_SCOPE)
   else()
----------------
ldionne wrote:
> lebedev.ri wrote:
> > This looks suspect
> Yeah, I don't understand why this isn't just `CMAKE_INSTALL_LIBDIR` like elsewhere.
See the non-line comment I responded to @lebidev.ri with. In sort if

```
${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_LIBDIR}/${target}
```

is a relative path, then we end up with

```
${CMAKE_INSTALL_PREFIX}/${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_LIBDIR}/${target}
```

instead of 

```
${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}/${target}
```

as we do with the other per-package prefixes. Also if `CMAKE_INSTALL_LIBDIR` is already an absolute path, then
```
${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_FULL_LIBDIR}/${target}
```
is the same thing, and closer to the second than the first.


================
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:
> 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.
So just skip everything compile-rt related for now?


================
Comment at: libcxx/CMakeLists.txt:32
+
+include(GNUInstallDirs)
 
----------------
compnerd wrote:
> 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.
Huh. That one always had the unconditional `include(GNUInstalDirs)`, so not sure why the CI build was failing before.


================
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
----------------
ldionne wrote:
> compnerd wrote:
> > 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.
> Actually, I've never used it myself, but @phosek seems to be using it for the Runtimes build to output one set of headers for each target, as mentioned above.
> 
> It seems to me that tweaking `CMAKE_INSTALL_PREFIX` globally when driving the libc++ build from the runtimes build would be more in-line with the CMake way of doing things (one configuration == one build), but I'm curious to hear what @phosek has to say about that.
> It seems to me that tweaking CMAKE_INSTALL_PREFIX globally when driving the libc++ build from the runtimes build would be more in-line with the CMake way of doing things (one configuration == one buid)

You mean trying to mutate it during the libc++ CMake eval and then set it back after? That would keep the intended meaning of  `CMAKE_INSTALL_PREFIX` being the actual prefix, but that feels awfully fragile to me. Or do you mean something else?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484