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

[John Ericson via Phabricator via Openmp-commits]

Ericson2314 added inline comments.


================
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()
----------------
Ericson2314 wrote:
> phosek wrote:
> > Ericson2314 wrote:
> > > 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.
> > I'm not sure if that's desirable. I'm not sure if we still need `COMPILER_RT_INSTALL_PATH`. That variable is only used by `clang/runtime/CMakeLists.txt` which predates `runtimes/CMakeLists.txt` and was AFAIK only ever used by Apple. I think we should consider removing `COMPILER_RT_INSTALL_PATH`. We'll need to check if `clang/runtime/CMakeLists.txt` is still being used or not.
> With D99697 now all approved (thanks again!) it looks like this is the next step? Would be very nice if this could be removed!
I went ahead and opened https://reviews.llvm.org/D99755 with @phosek's tentative proposal; I figured it would be easier for others to discuss in a dedicated patch than there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484