Page MenuHomePhabricator

Use `GNUInstallDirs` to support custom installation dirs. -- LLVM
Needs ReviewPublic

Authored by Ericson2314 on Apr 19 2021, 8:27 PM.

Details

Reviewers
jdoerfert
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.


On the advice of @lebedev.ri, I am splitting this up a bit per
subproject, starting with LLVM. To allow it to be more easily reviewed. This and the subsequent patch must be landed together, as this will not build alone. But the rest can be landed on their own.

Diff Detail

Unit TestsFailed

TimeTest
1,550 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::strcmp.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/strcmp.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/strcmp.c.tmp

Event Timeline

Ericson2314 created this revision.Apr 19 2021, 8:27 PM
Ericson2314 requested review of this revision.Apr 19 2021, 8:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2021, 8:27 PM
Ericson2314 abandoned this revision.Apr 22 2021, 7:37 AM

Ah this split won't work, because the changes to AddLLVM and friends mean the downstream packages also need to use GNUInstallDirs.

Ah this split won't work, because the changes to AddLLVM and friends mean the downstream packages also need to use GNUInstallDirs.

So what? The suggestion to split this up was specifically to simplify review.
Nothing is stopping you from committing everything at once in the end.

Reopen, without llvm_install_symlink changes

Ericson2314 edited the summary of this revision. (Show Details)Apr 22 2021, 8:11 AM

Quote variable usages

Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptApr 27 2021, 10:04 PM

Fix stray ':' typo

Rebase, no changes

@compnerd Would you like to review this one too (which the other you approved needs)?

primeos added a subscriber: primeos.Jun 5 2021, 5:26 AM
ormris removed a subscriber: ormris.Jun 7 2021, 4:11 PM

Rebased, fixed unintersting conflicts, organized docs better

@phosek would you mind reviewing this one next?

Rebase, fixing conflicts

rebase, fixing conflicts

compnerd added inline comments.Aug 2 2021, 9:50 AM
llvm/CMakeLists.txt
589

Why the change from llvm -> ${project}? (not that it really makes a difference)

llvm/cmake/modules/AddSphinxTarget.cmake
77

Nit: trailing slash is unnecessary, CMAKE_INSTALL_MANDIR should be defined, and if not, you do not want installation into / anyway.

llvm/cmake/modules/CMakeLists.txt
3–4

Why is this variable being put into the cache now?

llvm/cmake/modules/LLVMInstallSymlink.cmake
7

Nit: trailing slash shouldn't be there.

llvm/tools/llvm-config/llvm-config.cpp
361

Why the temporary StringRef? Can you not just initialize Path with the literal?

366

Similar

Fix things in response to comments

Ericson2314 marked 2 inline comments as done.Aug 3 2021, 11:14 AM
Ericson2314 added inline comments.
llvm/CMakeLists.txt
589

To be honest, I forgot. I might have just been OCD deduping on the fly. Happy to change back,

llvm/cmake/modules/CMakeLists.txt
3–4

I wanted all the installation dirs to be in there for consistency (c.f. Did the same for compiler-rt in D105765).

FWIW I have a yet-unposted commit I want to improve a bit to make it more genuinely user-configurable too (we want the CMake with the headers so it need not be installed runtime)

llvm/tools/llvm-config/llvm-config.cpp
361

I'm not sure. I would think so too, but the old code was also using StringRef so I just followed cargo culted and went with it.

Ericson2314 added inline comments.Aug 10 2021, 1:07 AM
llvm/CMakeLists.txt
589

Which would you like me to do, @compnerd?

llvm/tools/llvm-config/llvm-config.cpp
361

Should I, say, submit a separate patch trying to remove the existing one?