Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
600

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
3–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
73

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

162

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?

389

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
73

Doing in D116510 instead.

162

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

292

Yes, will do later.

389

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
0–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.