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

[Ericson2314]

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.

[Meinersbur]

In D99484#2668225, @Ericson2314 wrote:

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]

LGTM for libcxx and libcxxabi.

[Ericson2314]

Fix error, remove more CMAKE_INSTALL_LIBDIR

[Ericson2314]

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

[Ericson2314]

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]

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.

[Ericson2314]

Recombind revisions, need to convert everything at once

[Ericson2314]

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

[compnerd]

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?

[Ericson2314]

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.

[Ericson2314]

Quote variable usages

[Ericson2314]

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.

[Ericson2314]

Fix stray ':' typo

[Ericson2314]

Fix bug makeing polly path full, and rebase fixing conflicts

[Ericson2314]

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

[Meinersbur]

In D99484#2724055, @Ericson2314 wrote:

Fix bug makeing polly path full, and rebase fixing conflicts

What was the problem?

[Ericson2314]

In D99484#2724343, @Meinersbur wrote:

In D99484#2724055, @Ericson2314 wrote:

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.

[Meinersbur]

In D99484#2724435, @Ericson2314 wrote:

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]

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]

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]

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.

[Ericson2314]

rebased, fixed some new DESTINATION

[Ericson2314]

Rebase, push again after previous PR changed

[Ericson2314]

rebase

[Ericson2314]

Rebase again because previous diff changed

[Ericson2314]

Rebase, and catch two more DESTINATION bin

[Ericson2314]

Simple rebase

[Ericson2314]

Rebase after landing parent diff again, take 2

[Ericson2314]

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.

[Ericson2314]

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

[Ericson2314]

Rebase on top of fixed polly

[Ericson2314]

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

[Ericson2314]

Fix conflicts, one more thing for openmp

[Ericson2314]

rebase

[Ericson2314]

Retrigger CI now that deps are fixed

[Ericson2314]

Rebase after landing the polly change

[Ericson2314]

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]

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.

[vitalybuka]

It breaks multiple bots.
https://lab.llvm.org/buildbot/#/builders/37/builds/9960
https://lab.llvm.org/buildbot/#/builders/77/builds/13245
https://lab.llvm.org/buildbot/#/builders/169/builds/5409
https://lab.llvm.org/buildbot/#/builders/105/builds/19941

Can you please fix or revert it?

[Ericson2314]

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]

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

[Ericson2314]

Try again, with more include(GNUInstallDirs)

[Ericson2314]

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

[Ericson2314]

OK I split out D117417, D117418, D117419, and D117420.

[Ericson2314]

Trim down, now that many parts have been landed already

[Ericson2314]

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

clang-tools-extra/CMakeLists.txt

 include(CMakeDependentOption)
+include(GNUInstallDirs)
 
 option(CLANG_TIDY_ENABLE_STATIC_ANALYZER
   "Include static analyzer checks in clang-tidy" ON)
 
 add_subdirectory(clang-apply-replacements)
 add_subdirectory(clang-reorder-fields)
 add_subdirectory(modularize)
 add_subdirectory(clang-tidy)

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

clang_target_link_libraries(clang-doc
   clangToolingCore
   )
 target_link_libraries(clang-doc
   PRIVATE
   clangDoc
   )
 
 install(FILES ../assets/clang-doc-default-stylesheet.css
-  DESTINATION share/clang
+  DESTINATION "${CMAKE_INSTALL_DATADIR}/clang"
   COMPONENT clang-doc)
 
 install(FILES ../assets/index.js
-  DESTINATION share/clang
+  DESTINATION "${CMAKE_INSTALL_DATADIR}/clang"
   COMPONENT clang-doc)

clang-tools-extra/clang-include-fixer/find-all-symbols/tool/CMakeLists.txt

clang_target_link_libraries(find-all-symbols
   clangTooling
   )
 target_link_libraries(find-all-symbols
   PRIVATE
   findAllSymbols
   )
 
 install(PROGRAMS run-find-all-symbols.py
-  DESTINATION share/clang
+  DESTINATION "${CMAKE_INSTALL_DATADIR}/clang"
   COMPONENT find-all-symbols)

clang-tools-extra/clang-include-fixer/tool/CMakeLists.txt

clang_target_link_libraries(clang-include-fixer
   )
 target_link_libraries(clang-include-fixer
   PRIVATE
   clangIncludeFixer
   findAllSymbols
   )
 
 install(PROGRAMS clang-include-fixer.el
-  DESTINATION share/clang
+  DESTINATION "${CMAKE_INSTALL_DATADIR}/clang"
   COMPONENT clang-include-fixer)
 install(PROGRAMS clang-include-fixer.py
-  DESTINATION share/clang
+  DESTINATION "${CMAKE_INSTALL_DATADIR}/clang"
   COMPONENT clang-include-fixer)

clang-tools-extra/clang-tidy/CMakeLists.txt

 # Other subtargets. These may reference ALL_CLANG_TIDY_CHECKS
 # and must be below its definition.
 add_subdirectory(plugin)
 add_subdirectory(tool)
 add_subdirectory(utils)
 
 if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
   install(DIRECTORY .
-    DESTINATION include/clang-tidy
+    DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/clang-tidy"
     COMPONENT clang-tidy-headers
     FILES_MATCHING
     PATTERN "*.h"
     )
   add_custom_target(clang-tidy-headers)
   set_target_properties(clang-tidy-headers PROPERTIES FOLDER "Misc")
   if(NOT LLVM_ENABLE_IDE)
     add_llvm_install_targets(install-clang-tidy-headers
                              DEPENDS clang-tidy-headers
                              COMPONENT clang-tidy-headers)
   endif()
 endif()

clang-tools-extra/clang-tidy/tool/CMakeLists.txt

target_link_libraries(clang-tidy
   PRIVATE
   clangTidy
   clangTidyMain
   ${ALL_CLANG_TIDY_CHECKS}
   )
 
 
 install(PROGRAMS clang-tidy-diff.py
-  DESTINATION share/clang
+  DESTINATION "${CMAKE_INSTALL_DATADIR}/clang"
   COMPONENT clang-tidy)
 install(PROGRAMS run-clang-tidy.py
-  DESTINATION bin
+  DESTINATION "${CMAKE_INSTALL_BINDIR}"
   COMPONENT clang-tidy
   RENAME run-clang-tidy)

clang-tools-extra/modularize/CMakeLists.txt

clang_target_link_libraries(modularize
   clangDriver
   clangFrontend
   clangLex
   clangSerialization
   clangTooling
   )
 
 install(TARGETS modularize
-        RUNTIME DESTINATION bin
+        RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}"
         COMPONENT clang-extras)