This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Ericson2314 on Mar 28 2021, 9:49 PM.

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

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.Apr 6 2021, 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)Apr 10 2021, 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.EditedApr 19 2021, 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.Apr 27 2021, 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 ↗(On Diff #339639)

Why is this quoted but other uses not?

This revision is now accepted and ready to land.Apr 27 2021, 3:41 PM
Ericson2314 added inline comments.Apr 27 2021, 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.Apr 27 2021, 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.EditedApr 28 2021, 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.Apr 29 2021, 7:58 AM
polly/cmake/CMakeLists.txt
104–110 ↗(On Diff #341339)

@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.EditedApr 30 2021, 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.Apr 30 2021, 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.

primeos added a subscriber: primeos.Jun 5 2021, 5:25 AM

rebased, fixed some new DESTINATION

Rebase, push again after previous PR changed

Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2021, 6:21 PM
MTC added a subscriber: MTC.Oct 10 2021, 8:41 PM

Rebase again because previous diff changed

Ericson2314 marked 6 inline comments as done.Oct 30 2021, 9:16 AM

Rebase, and catch two more DESTINATION bin

Simple rebase

Rebase after landing parent diff again, take 2

I split out D115568 D115569 D115570 from this. I think it is better to start from the outside and work back inward, so the LLVM-only commit that previously came before this would actually be the last step.

  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.