Page MenuHomePhabricator

[cmake] Don't export `LLVM_TOOLS_INSTALL_DIR` anymore
Needs ReviewPublic

Authored by Ericson2314 on Jan 22 2022, 10:10 PM.

Details

Summary

First of all, LLVM_TOOLS_INSTALL_DIR put there breaks our NixOS
builds, because LLVM_TOOLS_INSTALL_DIR defined the same as
CMAKE_INSTALL_BINDIR becomes an *absolute* path, and then when
downstream projects try to install there too this breaks because our
builds always install to fresh directories for isolation's sake.

Second of all, note that LLVM_TOOLS_INSTALL_DIR stands out against the
other specially crafted LLVM_CONFIG_* variables substituted in
llvm/cmake/modules/LLVMConfig.cmake.in.

@beanz added it in d0e1c2a550ef348aae036d0fe78cab6f038c420c to fix a
dangling reference in AddLLVM, but I am suspicious of how this
variable doesn't follow the pattern.

Those other ones are carefully made to be build-time vs install-time
variables depending on which LLVMConfig.cmake is being generated, are
carefully made relative as appropriate, etc. etc. For my NixOS use-case
they are also fine because they are never used as downstream install
variables, only for reading not writing.

To avoid the problems I face, and restore symmetry, I deleted the
exported and arranged to have many ${project}_TOOLS_INSTALL_DIRs.
AddLLVM now instead expects each project to define its own, and they
do so based on CMAKE_INSTALL_BINDIR. LLVMConfig still exports
LLVM_TOOLS_BINARY_DIR which is the location for the tools defined in
the usual way, matching the other remaining exported variables.

For the AddLLVM changes, I tried to copy the existing pattern of
internal vs non-internal or for LLVM vs for downstream function/macro
names, but it would good to confirm I did that correctly.

Diff Detail

Event Timeline

Ericson2314 created this revision.Jan 22 2022, 10:10 PM
Herald added a reviewer: rafauler. · View Herald Transcript
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Ericson2314 requested review of this revision.Jan 22 2022, 10:10 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Ericson2314 retitled this revision from WIP: [cmake] Don't export `LLVM_TOOLS_INSTALL_DIR` anymore to [cmake] Don't export `LLVM_TOOLS_INSTALL_DIR` anymore.Jan 23 2022, 2:20 PM

Could one of you review this?

I think in openmp/tools currently only libraries and header files get installed, but no binaries.

I suggest to define OPENMP_TOOLS_INSTALL_BINDIR, OPENMP_TOOLS_INSTALL_LIBDIR, and OPENMP_TOOLS_INSTALL_INCLUDEDIR and probably base these variables on OPENMP_TOOLS_INSTALL_DIR, if defined (e.g., for distro builds), or OPENMP_INSTALL_{BIN|LIB|INCLUDE}DIR as fallback.

openmp/tools/*/CMakeLists.txt should then use these variables instead of the various variables they use currently.

openmp/tools/CMakeLists.txt
1–3

Is this variable used anywhere?

yota9 removed a subscriber: yota9.Feb 21 2022, 5:58 AM