Page MenuHomePhabricator

Use `GNUInstallDirs` to support custom installation dirs.
AcceptedPublic

Authored by Ericson2314 on Mar 28 2021, 9:49 PM.
Tags
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project

Details

Reviewers
bollu
ldionne
sscalpone
awarzynski
jdoerfert
compnerd
beanz
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rT253115e2acbe: Added 'host' device for various *_RUN_PLACEHOLDER because
Summary

This is a new draft of D28234. I previously did the unorthodox thing of pushing to it when I wasn't the original author, but since this version

  • Uses GNUInstallDirs, rather than mimics it, as the original author was hesitant to do but others requested.
  • Is much broader, effecting many more projects than LLVM itself.

I figured it was time to make a new revision.

I am using this patch (and many back-ports) as the basis of https://github.com/NixOS/nixpkgs/pull/111487 for my distro (NixOS). It looked like people were generally on board in D28234, but I make note of this here in case extra motivation is useful.


As pointed out in the original issue, a central tension is that LLVM already has some partial support for these sorts of things. For example LLVM_LIBDIR_SUFFIX, or COMPILER_RT_INSTALL_PATH. Because it's not quite clear yet what to do about those, we are holding off on changing libdirs and compiler-rt. for this initial PR.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
compnerd added inline comments.Mar 31 2021, 8:07 AM
clang/cmake/modules/AddClang.cmake
129

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.

compiler-rt/cmake/base-config-ix.cmake
72 ↗(On Diff #334152)

Sure, but the *descriptions* of the options are needed to changed for changing the value.

That said, I'm not convinced that this change is okay. It changes the way that compiler-rt can be built and used when building a toolchain image. The handling of the install prefix in compiler-rt is significantly different from the other parts of LLVM, and so, I think that it may need to be done as a separate larger effort.

libcxx/CMakeLists.txt
32

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.

libcxx/cmake/Modules/HandleLibCXXABI.cmake
66

I don't think that bringing up other build systems is particularly helpful.

I do expect it to be modified, and I suspect that this is used specifically for builds that @ldionne supports.

ldionne added a subscriber: phosek.Mar 31 2021, 8:29 AM

I am generally OK with the libcxx and libcxxabi changes.

compiler-rt/cmake/Modules/CompilerRTUtils.cmake
389 ↗(On Diff #333765)

Yeah, I don't understand why this isn't just CMAKE_INSTALL_LIBDIR like elsewhere.

libcxx/cmake/Modules/HandleLibCXXABI.cmake
66

Actually, I've never used it myself, but @phosek seems to be using it for the Runtimes build to output one set of headers for each target, as mentioned above.

It seems to me that tweaking CMAKE_INSTALL_PREFIX globally when driving the libc++ build from the runtimes build would be more in-line with the CMake way of doing things (one configuration == one build), but I'm curious to hear what @phosek has to say about that.

Ericson2314 added inline comments.Mar 31 2021, 10:38 AM
clang/cmake/modules/AddClang.cmake
129

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.

compiler-rt/cmake/Modules/CompilerRTUtils.cmake
389 ↗(On Diff #333765)

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.

compiler-rt/cmake/base-config-ix.cmake
72 ↗(On Diff #334152)

So just skip everything compile-rt related for now?

libcxx/CMakeLists.txt
32

Huh. That one always had the unconditional include(GNUInstalDirs), so not sure why the CI build was failing before.

libcxx/cmake/Modules/HandleLibCXXABI.cmake
66

It seems to me that tweaking CMAKE_INSTALL_PREFIX globally when driving the libc++ build from the runtimes build would be more in-line with the CMake way of doing things (one configuration == one buid)

You mean trying to mutate it during the libc++ CMake eval and then set it back after? That would keep the intended meaning of CMAKE_INSTALL_PREFIX being the actual prefix, but that feels awfully fragile to me. Or do you mean something else?

ldionne added inline comments.Mar 31 2021, 2:33 PM
libcxx/cmake/Modules/HandleLibCXXABI.cmake
66

I keep forgetting that the runtimes build uses add_subdirectory to include each sub-project instead of driving a proper CMake build for each of those.

Basically, what I'm saying is that whatever place we decide to build for N multiple targets, we should perform N different CMake builds setting the appropriate CMAKE_INSTALL_PREFIX, one for each target. But that discussion should happen elsewhere, not on this review.

As far as this review is concerned, I do believe you want instead:

${CMAKE_INSTALL_INCLUDEDIR}${LIBCXX_INSTALL_HEADER_PREFIX}

(reversed the order of variables)

We should have @phosek confirm.

phosek added inline comments.Mar 31 2021, 5:17 PM
libcxx/cmake/Modules/HandleLibCXXABI.cmake
66

@ldionne CMAKE_INSTALL_PREFIX cannot be used for this purpose. When using the multiarch layout with the runtimes build, we want to place build artifacts in ${BUILD_DIR}/{include,lib}/<target>/... and install them to ${CMAKE_INSTALL_PREFIX}/{include,lib}/<target>/.... There are two issues:

  1. CMAKE_INSTALL_PREFIX only comes into play during install step, but we want the build layout to match the installation layout so we need a different variable to control the output directory.
  2. We already use CMAKE_INSTALL_PREFIX for the toolchain itself and there's no way to use it hierarchically, that is you cannot do something like ${SUPER_CMAKE_INSTALL_PREFIX}/{include,lib}/${CMAKE_INSTALL_PREFIX}/... which is why we need a separate variable to control the installation directory.

Regarding LIBCXX_INSTALL_HEADER_PREFIX, I haven't found any current uses of that variable. I introduced it in D59168 which was an earlier attempt at having per-target headers, but D89013 is strictly better for the reasons I described in https://reviews.llvm.org/D89013#2662578 (no duplicate headers) so I think we can just remove this variable.

phosek added inline comments.Mar 31 2021, 5:45 PM
libcxx/cmake/Modules/HandleLibCXXABI.cmake
66

I have sent D99697 for review which removes these variables.

phosek added inline comments.Mar 31 2021, 5:58 PM
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
389 ↗(On Diff #333765)

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.

Ericson2314 marked an inline comment as not done.Apr 1 2021, 8:20 AM
Ericson2314 added inline comments.
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
389 ↗(On Diff #333765)

With D99697 now all approved (thanks again!) it looks like this is the next step? Would be very nice if this could be removed!

polly/cmake/CMakeLists.txt
82–97

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?

compnerd added inline comments.Apr 1 2021, 8:45 AM
clang/cmake/modules/AddClang.cmake
129

Yes.

Rebase on newer version of previous diff

Rebase now that previous diff landed

Ericson2314 added inline comments.Apr 1 2021, 11:24 AM
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
389 ↗(On Diff #333765)

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.

Meinersbur added inline comments.
polly/cmake/CMakeLists.txt
82–97

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.

Don't use CMAKE_INTALL_LIBDIR or convert compiler-rt for now

These steps were suggested by @compnerd, IIUC. The compiler-rt stuff is in flux
because I am now skeptical whether COMPILER_RT_INSTALL_PATH can now be so
easily removed (see D99755).

Additionally, I have cleaned up the polly code so that I need not change it
so much, and it should still work with absolute or relative paths quite
flexibly. If COMPILER_RT_INSTALL_PATH or equivalent functionality is in fact
needed (i.e D99755 must be abandoned), I think the same method I am now using
for polly could also work there.

Additionally, I have cleaned up the polly code so that I need not change it
so much, and it should still work with absolute or relative paths quite
flexibly.

Looks fine to me.

ldionne accepted this revision as: Restricted Project, Restricted Project.Tue, Apr 6, 12:16 PM

LGTM for libcxx and libcxxabi.

Fix error, remove more CMAKE_INSTALL_LIBDIR

I think with this last revision this diff might finally be ready!

Ericson2314 edited the summary of this revision. (Show Details)Sat, Apr 10, 7:23 PM

I simplified the description at the top in light of feedback (some harder issues we are just punting on for now). @compnerd does this now look good to you then? Does anyone want to approve the libunwind said of things? Anything else this is waiting on?

Ericson2314 updated this revision to Diff 338697.EditedMon, Apr 19, 8:28 PM

Rebase. On the advice of @lebedev.ri, I am splitting this up a bit per
subproject to allow it to be more easily reviewed. See D100810 which is just for LLVM proper.

Recombind revisions, need to convert everything at once

Resplit on @LebedevRI's advice that it's OK if not all patches build alone

compnerd accepted this revision.Tue, Apr 27, 3:41 PM

This is a pretty straightforward cleanup now, which adds additional functionality by deferring work to CMake. There are a couple of minor points about inconsistent quoting but this seems good otherwise.

clang-tools-extra/clang-doc/tool/CMakeLists.txt
26

Why are these quoted but other uses not?

flang/CMakeLists.txt
442

Why is this quoted but other uses not?

This revision is now accepted and ready to land.Tue, Apr 27, 3:41 PM
Ericson2314 added inline comments.Tue, Apr 27, 3:46 PM
clang-tools-extra/clang-doc/tool/CMakeLists.txt
26

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.

Quote variable usages

Ericson2314 added inline comments.Tue, Apr 27, 10:09 PM
clang-tools-extra/clang-doc/tool/CMakeLists.txt
26

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.

Fix stray ':' typo

Fix bug makeing polly path full, and rebase fixing conflicts

Resubmit now prior diff is rebased, so CI doesn't trip up

Fix bug makeing polly path full, and rebase fixing conflicts

What was the problem?

Ericson2314 added a comment.EditedWed, Apr 28, 5:39 PM

Fix bug makeing polly path full, and rebase fixing conflicts

What was the problem?

I effectively replaced ${CMAKE_INSTALL_PREFIX}/lib with ${CMAKE_INSTALL_LIBDIR}, which evaluates (by default) merely to lib. The fix was ${CMAKE_INSTALL_FULL_LIBDIR} which will always return an absolute path.

I effectively replaced ${CMAKE_INSTALL_PREFIX}/lib with ${CMAKE_INSTALL_LIBDIR}, which evaluates (by default) merely to lib. The fix was ${CMAKE_INSTALL_FULL_LIBDIR} which will always return an absolute path.

AFAIU, install is always relative the the install prefix, so there should be no difference?

Ericson2314 added inline comments.Thu, Apr 29, 7:58 AM
polly/cmake/CMakeLists.txt
104–110

@Meinersbur the change was here. I did indeed get an error because this stuff prior to installing did in fact need an absolute path.

Ericson2314 added a comment.EditedFri, Apr 30, 8:38 AM

This is now approved and passing CI, and I have also normalized the quoting @compnerd asked about (in I hope a satisfactory way). Should I be pinging someone to land it?

*edit* oh I suppose https://reviews.llvm.org/D100810 also needs approval.

Ericson2314 added inline comments.Fri, Apr 30, 8:42 AM
compiler-rt/cmake/base-config-ix.cmake
72 ↗(On Diff #334152)

compile-rt is skipped in the latest version, and I have an independent https://reviews.llvm.org/D101497 laying the groundwork for a future GNUInstallDirs-adding patch to do the regular thing.