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

Fri Jan 14 16:56:40 PST 2022

Ericson2314 marked 10 inline comments as done.
Ericson2314 added a comment.

The approval on this patch is quite old, but nothing much interesting has happened in it since then --- if anything, it has gotten more trivial as I created patches "under" it and landed them which cleared the way for this.

The build failure in the libc++ "modular build" I also see in other CI runs, e.g. https://buildkite.com/llvm-project/libcxx-ci/builds/7855#b40f8ad6-dc6a-4589-81d7-a459dae8b7e7, so I it appears unrelated.

I double checked and old lingering conversations (pre approval) can be marked resolved.

================
Comment at: clang-tools-extra/clang-doc/tool/CMakeLists.txt:26
 install(FILES ../assets/index.js
-  DESTINATION share/clang
+  DESTINATION "${CMAKE_INSTALL_DATADIR}/clang"
   COMPONENT clang-doc)
----------------
Ericson2314 wrote:
> Ericson2314 wrote:
> > compnerd wrote:
> > > Why are these quoted but other uses not?
> > I confess I have no clue when quoting is required or advisable with CMake. I started out converting things by hand and then did some auto-conversions, this must have been one of the by-hand ones.
> > 
> > Happy to normalize either way, just tell me which one.
> I looked it up, https://stackoverflow.com/questions/35847655/when-should-i-quote-cmake-variables says I'm slightly better off quoting all of these so I did.
Since then I landed D115566, which made the quoting consistent prior to this. This adds quotes defensively around variable expansions, and just needs to where the path expression didn't involve variables before.

================
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:
> > > 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.
`CMAKE_INSTALL_LIBDIR` is no longer introduced in this patch; this has been the case since before the approval.

================
Comment at: libcxx/CMakeLists.txt:32
+
+include(GNUInstallDirs)

----------------
Ericson2314 wrote:
> 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.
`include(GNUInstallDirs)` is no longer in funny locations like this.

================
Comment at: polly/cmake/CMakeLists.txt:82-96
+set(POLLY_INSTALL_PREFIX "")
 set(POLLY_CONFIG_LLVM_CMAKE_DIR "${LLVM_BINARY_DIR}/${LLVM_INSTALL_PACKAGE_DIR}")
-set(POLLY_CONFIG_CMAKE_DIR "${POLLY_INSTALL_PREFIX}/${POLLY_INSTALL_PACKAGE_DIR}")
-set(POLLY_CONFIG_LIBRARY_DIRS "${POLLY_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}")
+set(POLLY_CONFIG_CMAKE_DIR "${POLLY_INSTALL_PREFIX}${CMAKE_INSTALL_PREFIX}/${POLLY_INSTALL_PACKAGE_DIR}")
+set(POLLY_CONFIG_LIBRARY_DIRS "${POLLY_INSTALL_PREFIX}${CMAKE_INSTALL_FULL_LIBDIR}${LLVM_LIBDIR_SUFFIX}")
 if (POLLY_BUNDLED_ISL)
   set(POLLY_CONFIG_INCLUDE_DIRS
+    "${POLLY_INSTALL_PREFIX}${CMAKE_INSTALL_FULL_LIBDIR}"
----------------
Meinersbur wrote:
> Ericson2314 wrote:
> > `POLLY_INSTALL_PREFIX`, like `COMPILER_RT_INSTALL_PATH`, I changed to be empty by default. If the `COMPILER_RT_INSTALL_PATH` can be removed, maybe `POLLY_INSTALL_PREFIX` can also be removed?
> I don't have an overview atm on Polly's different paths, but could look into it if needed. Originally, this was derived from how Clang did it. If it works for COMPILER_RT_INSTALL_PATH, it should work for Polly as well.
Polly was made like the others with D116555. There is no issue here anymore.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484