[libc-commits] [PATCH] D99484: [cmake] Use `GNUInstallDirs` to support custom installation dirs.
John Ericson via Phabricator via libc-commits
libc-commits at lists.llvm.org
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
More information about the libc-commits
mailing list