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

Fri Apr 30 08:48:34 PDT 2021

Ericson2314 added inline comments.

================
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:
> > 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?
compile-rt is skipped in the latest version, and I have an independent https://reviews.llvm.org/D101497 laying the groundwork for a future `GNUInstallDirs`-adding patch to do the regular thing.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484