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

Thu Apr 1 08:50:48 PDT 2021

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:
> > 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.
Yes.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484