This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Use `GNUInstallDirs` to support custom installation dirs
ClosedPublic

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

Details

Summary

This is the patch for LLVM proper in my series for adding GNUInstallDirs support in all project.

Additionally:

Create a new CACHE STRING variable, LLVM_EXAMPLES_INSTALL_DIR, to control where the examples are installed on analogy with the other variables.


This patch supersedes D28234, which tried to do the same thing but hand-rolled without GNUInstallDirs.

This patch nearly reverts commit 3 0fc88bf1dc15a72e2d9809d28019d386b7a7cc0, which was a revert of a prior attempt."

(I had to add a space here or else Phabricator detects a reference cycle and won't let me do the form submit.)

Diff Detail

Unit TestsFailed

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
614–615

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

llvm/cmake/modules/AddSphinxTarget.cmake
76–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
4

Why is this variable being put into the cache now?

llvm/cmake/modules/LLVMInstallSymlink.cmake
9

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
614–615

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

llvm/cmake/modules/CMakeLists.txt
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
614–615

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?

Ericson2314 added inline comments.Oct 7 2021, 9:43 AM
llvm/tools/llvm-config/llvm-config.cpp
361

I opened D111322 for this. If that passes CI and lands, I have the next version of this without StringRef ready to go.

Rebase, avoid extra string ref conversions in llvm-config

Ericson2314 marked 2 inline comments as done.Oct 11 2021, 12:18 PM

Removed extraneous StringRef conversion after previous D111322 was landed.

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

Fix issue now.

Skip ${project} change

Ericson2314 marked an inline comment as done.Oct 15 2021, 5:30 AM

Respond to last open thing

llvm/CMakeLists.txt
614–615

Erred on the size of caution and put it back.

Simple rebase

compnerd accepted this revision.Nov 1 2021, 7:37 PM
compnerd added inline comments.
llvm/CMakeLists.txt
289
llvm/docs/CMake.rst
270

I'm kinda torn on this. The variables here are all CMake standard and we should redirect users to CMake. Although, this is following the existing pattern, so seems reasonable.

This revision is now accepted and ready to land.Nov 1 2021, 7:37 PM

Fix last comment

mib added a subscriber: mib.Nov 2 2021, 10:53 AM

Hi @Ericson2314, I think this patch broke our macOS lldb-incremental bot on GreenDragon (https://green.lab.llvm.org/green/job/lldb-cmake/37560/console).

Could you please take a look ? Thanks!

mib added a comment.EditedNov 2 2021, 11:15 AM

I reverted this patch locally and tried building with clang modules (-DLLVM_ENABLE_MODULES=On) which confirmed it was causing the build failure on the bots.

I had to revert your patch upstream to fix our bots. If you need some help to reproduce the build failure, let me know.

Ericson2314 added a comment.EditedNov 3 2021, 9:13 PM

@mib Sorry for not catching this myself. I must admit the error does confuse me. I have an macOS machine I can ssh to, and yes, if if you had some tips on how to reproduce there I would greatly appreciate them.

Thanks.

llvm/docs/CMake.rst
270

Yeah maybe we can redo the section after. "rarely used" is a funny title; shouldn't we be mentioning he most notable CMake variables if any at all?

@mib Can you help me reproduce? I ran

cmake -G Ninja .. -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_INSTALL_PREFIX="$HOME/tmp/out" -DLLDB_TEST_USER_ARGS="--arch;x86_64;--build-dir;$HOME/tmp/out/lldb-test-build.noindex;-t;--env;TERM=vt100" '-DLLDB_ENABLE_PYTHON=On' '-DLLVM_ENABLE_ASSERTIONS:BOOL=TRUE' '-DLLVM_ENABLE_MODULES=On' '-DLLVM_ENABLE_PROJECTS=clang;libcxx;libcxxabi;compiler-rt;lld;lldb;cross-project-tests' "-DLLVM_LIT_ARGS=-v --time-tests --shuffle --xunit-xml-output=$HOME/tmp/test/results.xml" '-DLLVM_VERSION_PATCH=99' '-DCMAKE_C_FLAGS=-Wdocumentation' '-DCMAKE_CXX_FLAGS=-Wdocumentation'

on ssh'd to a macOS machine with perhaps fewer SDKs installed, and where the failure occurred I got:

-- Clang version: 14.0.99
-- Host linker version: 530
# (error was here)
-- Not building amdgpu-arch: hsa-runtime64 not found
-- LLD version: 14.0.99
-- Enable editline support in LLDB: TRUE
-- Enable curses support in LLDB: TRUE

I tried again without lld in the list of enable projects, in case it that made a difference to force it to use ld64 more, but it still worked.

mib added a comment.Dec 10 2021, 11:11 AM

I tried again without lld in the list of enable projects, in case it that made a difference to force it to use ld64 more, but it still worked.

Hi @Ericson2314, I tried reproducing the failure with your patch applied and I couldn't ... May be you can try re-landing this patch and monitor the macOS lldb bots for a failure:

Ericson2314 reopened this revision.Dec 31 2021, 11:07 AM

I guess reopening is the better way to redo after revert?

This revision is now accepted and ready to land.Dec 31 2021, 11:07 AM
Ericson2314 retitled this revision from Use `GNUInstallDirs` to support custom installation dirs. -- LLVM to [llvm] Use `GNUInstallDirs` to support custom installation dirs. -- LLVM.Dec 31 2021, 1:08 PM
Ericson2314 retitled this revision from [llvm] Use `GNUInstallDirs` to support custom installation dirs. -- LLVM to [llvm] Use `GNUInstallDirs` to support custom installation dirs.
Ericson2314 edited the summary of this revision. (Show Details)Dec 31 2021, 1:38 PM
Ericson2314 edited the summary of this revision. (Show Details)Dec 31 2021, 1:53 PM
Ericson2314 edited the summary of this revision. (Show Details)

Big rebase and cleanup

  1. Updating D100810: [llvm] 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

Mark variables as advanced in doc too

  1. Updating D100810: [llvm] 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

Removed one to many ${project}

  1. Updating D100810: [llvm] 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

Fix bugs

  1. Updating D100810: [llvm] 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 on top of D116467

Ericson2314 marked 2 inline comments as done.Jan 1 2022, 10:22 AM

Mark some old threads done.

llvm/cmake/modules/CMakeLists.txt
4

I see I put this back for now, that's good.

  1. Updating D100810: [llvm] 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

Robust

mstorsjo added inline comments.
llvm/CMakeLists.txt
75

Nit: This looks like a spurious unrelated change to whitespace?

164

Could this bit be split out to separate change, to keep this as small as possible - unless it's strictly needed and tied to this one?

292

Nit: Unrelated and can be split out?

406

Unrelated?

Remove off topic bits

Ericson2314 marked 4 inline comments as done.Jan 2 2022, 6:04 PM
Ericson2314 added inline comments.
llvm/CMakeLists.txt
75

Doing in D116510 instead.

164

Doing that, I will post the revision later as it is not that important.

292

Yes, will do later.

406

Yes, see above.

Ericson2314 marked 4 inline comments as done.

Rebase

Ericson2314 edited the summary of this revision. (Show Details)Jan 7 2022, 4:13 PM

OK, at long last, I this is the attempt that's gonna make it!

Ericson2314 added inline comments.Jan 7 2022, 4:47 PM
llvm/cmake/modules/AddSphinxTarget.cmake
1

N.B. this already had it from an earlier revision of mine, so didn't need to add it.

arichardson added inline comments.
llvm/CMakeLists.txt
5

This seems to be causing the following warning for me:

CMake Warning (dev) at /opt/clion-2021.2/bin/cmake/linux/share/cmake-3.20/Modules/GNUInstallDirs.cmake:236 (message):
  Unable to determine default CMAKE_INSTALL_LIBDIR directory because no
  target architecture is known.  Please enable at least one language before
  including GNUInstallDirs.
arichardson added inline comments.Jan 17 2022, 6:57 AM
llvm/CMakeLists.txt
5

Moving it below project(LLVM should fix that, but I'm not sure if there is a reason that it's up here.