[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
   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
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
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
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.

  rG LLVM Github Monorepo



More information about the libc-commits mailing list