Page MenuHomePhabricator

[clang-tools-extra][cmake] Use `GNUInstallDirs` to support custom installation dirs.
ClosedPublic

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
  • Restricted Project

Details

Summary

This is the original patch in my GNUInstallDirs series, now last to merge as the final piece!

It arose as a new draft of D28234. I initially did the unorthodox thing of pushing to that when I wasn't the original author, but since I ended up

  • Using GNUInstallDirs, rather than mimicking it, as the original author was hesitant to do but others requested.
  • Converting all the packages, not just LLVM, effecting many more projects than LLVM itself.

I figured it was time to make a new revision.

I have used this patch series (and many back-ports) as the basis of https://github.com/NixOS/nixpkgs/pull/111487 for my distro (NixOS), which was merged last spring (2021). 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. Variables like COMPILER_RT_INSTALL_PATH have already been dealt with. Variables like LLVM_LIBDIR_SUFFIX however, will require further work, so that we may use CMAKE_INSTALL_LIBDIR.

These remaining items will be addressed in further patches. What is here is now rote and so we should get it out of the way before dealing more intricately with the remainder.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  1. Updating D99484: Use GNUInstallDirs to support custom installation dirs. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

rebase atop other PRs

Rebase on top of fixed polly

Make sure possibly exposed modules have their own include(GNUInstallDirs)s

Ericson2314 retitled this revision from Use `GNUInstallDirs` to support custom installation dirs. to [cmake] Use `GNUInstallDirs` to support custom installation dirs..Jan 8 2022, 4:24 PM

Fix conflicts, one more thing for openmp

Retrigger CI now that deps are fixed

Rebase after landing the polly change

Ericson2314 marked 10 inline comments as done.Jan 14 2022, 4:56 PM

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.

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

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.

clang/cmake/modules/AddClang.cmake
127 ↗(On Diff #334152)

CMAKE_INSTALL_LIBDIR is no longer introduced in this patch; this has been the case since before the approval.

libcxx/CMakeLists.txt
32 ↗(On Diff #334152)

include(GNUInstallDirs) is no longer in funny locations like this.

polly/cmake/CMakeLists.txt
82–96 ↗(On Diff #334576)

Polly was made like the others with D116555. There is no issue here anymore.

Ericson2314 marked 3 inline comments as done.Jan 14 2022, 5:01 PM
Ericson2314 added inline comments.
polly/cmake/CMakeLists.txt
85 ↗(On Diff #333765)

I am not sure how to find the date on this comment but I quite sure it dates back to an earlier change on this line. This just combines the two parts of the path with a / which is what we want and what it did before.

Ericson2314 edited the summary of this revision. (Show Details)Jan 14 2022, 5:06 PM
Ericson2314 added a subscriber: sterni.
This revision was landed with ongoing or failed builds.Jan 14 2022, 5:08 PM
This revision was automatically updated to reflect the committed changes.
Ericson2314 marked an inline comment as done.

Sorry about that.

I think the issue was just one more include(GNUInstallDirs) for compiler-rt for the standalone build, but I am going to bed so just reverted for now to try again later.

Ericson2314 reopened this revision.Jan 14 2022, 11:48 PM

Found two more include(GNUInstallDirs) I should include which will hopefully fix the build.

This revision is now accepted and ready to land.Jan 14 2022, 11:48 PM

Try again, with more include(GNUInstallDirs)

This revision was landed with ongoing or failed builds.Jan 15 2022, 9:33 PM
This revision was automatically updated to reflect the committed changes.
Ericson2314 reopened this revision.Jan 15 2022, 9:51 PM

Will try to break up the pathc further for better troubleshooting, but keeping this open to track progress.

This revision is now accepted and ready to land.Jan 15 2022, 9:51 PM

Trim down, now that many parts have been landed already

Ericson2314 retitled this revision from [cmake] Use `GNUInstallDirs` to support custom installation dirs. to [clang-tools-extra][cmake] Use `GNUInstallDirs` to support custom installation dirs..Jan 22 2022, 10:12 AM

Alright, this is the last bit (except for D101070 which evidently caused the issues), looking good.