This is an archive of the discontinued LLVM Phabricator instance.

[llvm][cmake] Make `install_symlink` workflow work with absolute install dirs
ClosedPublic

Authored by Ericson2314 on Apr 22 2021, 8:09 AM.

Details

Summary

If CMAKE_INSTALL_BINDIR is a different absolute path per project, as
it is with NixOS when we install every package to its own prefix, the
old way fails when the absolute path gets prepended with CMAKE_INSTALL_PREFIX.

The extend_path function does what we want, but it is currently internal-only. So easier to just inline the one small case of it we need.

Also fix one stray bin -> CMAKE_INSTALL_BINDIR

Diff Detail

Event Timeline

Ericson2314 created this revision.Apr 22 2021, 8:09 AM
Ericson2314 requested review of this revision.Apr 22 2021, 8:09 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 22 2021, 8:09 AM
  1. Updating D101070: Make llvm_install_symlink robust with respect to absolute 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

Herald added a project: Restricted Project. · View Herald TranscriptDec 27 2021, 1:50 PM

Much simpler now

Code taken from D99484 and then to D117419 and now to here.

Ericson2314 edited the summary of this revision. (Show Details)Jan 20 2022, 2:25 PM
Ericson2314 retitled this revision from Make `llvm_install_symlink` robust with respect to absolute dirs. to [llvm][cmake] Make `llvm_install_symlink` robust to absolute dirs..Jan 20 2022, 6:08 PM

Continuing my bisect of D99484 (since this change was in there at the time it was approved, but failing) I will land this one next.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 20 2022, 6:11 PM
This revision was automatically updated to reflect the committed changes.
Ericson2314 reopened this revision.Jan 20 2022, 6:51 PM

This seems to be the part of D99484 that wasn't working.

Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2022, 2:22 AM

Rebase, hopefully fix, and also fix stray GNUInstallDirs violation

Ericson2314 edited the summary of this revision. (Show Details)Jul 23 2022, 9:09 PM
Ericson2314 retitled this revision from [llvm][cmake] Make `llvm_install_symlink` robust to absolute dirs. to [llvm][cmake] Make `install_symlink` robust to absolute dirs..
Ericson2314 added reviewers: mstorsjo, beanz, phosek, sterni.
Ericson2314 updated this revision to Diff 447108.EditedJul 23 2022, 9:13 PM

Rebase to retrigger CI

I think it tried to cherry-pick an already-landed patch marked as a parent revision.

Hi, not sure if you saw D130256, but I needed to extend CMAKE_MODULE_PATH, otherwise the ExtendPath module was not found when running LLVMInstallSymlink.cmake.

Hi, not sure if you saw D130256, but I needed to extend CMAKE_MODULE_PATH, otherwise the ExtendPath module was not found when running LLVMInstallSymlink.cmake.

Just checked, if I run ninja install with this patch, it fails with

CMake Error at /llvm-project/llvm/cmake/modules/LLVMInstallSymlink.cmake:5 (include):
  include could not find requested file:

    ExtendPath
Call Stack (most recent call first):
  tools/llvm-ar/cmake_install.cmake:56 (include)
  tools/cmake_install.cmake:49 (include)
  cmake_install.cmake:78 (include)


FAILED: CMakeFiles/install.util

@sebastian-ne Thanks yes I was worried that might happen. I think I will just move the extend_path to the caller then.

Rebase, make caller responsible for making paths absolute

This avoids module resolution issues.

Ericson2314 retitled this revision from [llvm][cmake] Make `install_symlink` robust to absolute dirs. to [llvm][cmake] Make `install_symlink` workflow work with absolute install dirs.Jul 25 2022, 8:41 AM
Ericson2314 edited the summary of this revision. (Show Details)

I get a configure error with this version:

CMake Error at cmake/modules/AddLLVM.cmake:2052 (extend_path):
  Unknown CMake command "extend_path".
Call Stack (most recent call first):
  cmake/modules/AddLLVM.cmake:2148 (llvm_install_symlink)
  cmake/modules/AddLLVM.cmake:2154 (llvm_add_tool_symlink)
  tools/llvm-ar/CMakeLists.txt:22 (add_llvm_tool_symlink)
Ericson2314 updated this revision to Diff 447391.EditedJul 25 2022, 10:06 AM

Oops! ExtendPath was not already imported. Thanks, @sebastian-ne

Also sorry, I had missed D130256 all along!

sebastian-ne accepted this revision.Jul 25 2022, 12:54 PM

No problem, thanks for the fixes!
LGTM

This revision is now accepted and ready to land.Jul 25 2022, 12:54 PM
This revision was landed with ongoing or failed builds.Jul 25 2022, 2:04 PM
This revision was automatically updated to reflect the committed changes.
Ericson2314 reopened this revision.Jul 25 2022, 2:24 PM
This revision is now accepted and ready to land.Jul 25 2022, 2:24 PM
Ericson2314 updated this revision to Diff 447465.EditedJul 25 2022, 2:25 PM

Avoid extend_path

Ericson2314 edited the summary of this revision. (Show Details)Jul 25 2022, 2:26 PM
Ericson2314 edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Jul 25 2022, 8:14 PM
This revision was automatically updated to reflect the committed changes.