Page MenuHomePhabricator

[cmake] Don't export `LLVM_TOOLS_INSTALL_DIR` anymore
ClosedPublic

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 ↗(On Diff #402289)

Is this variable used anywhere?

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

I moved the openmp definitions so they should be used. Don't need the other variables yet I don't think?

openmp/tools/CMakeLists.txt
1–3 ↗(On Diff #402289)

llvm_add_tool computes a usage.

Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 10:56 PM
Herald added a subscriber: bzcheeseman. · View Herald Transcript

@nikic @mizvekov inspired by the recent activity in D126308, I rebased this. Would you mind giving it a review or trying out a local build?

mizvekov added inline comments.Jun 9 2022, 1:10 PM
bolt/tools/CMakeLists.txt
12–14

Instead of each project having to define a different macro for this, why not just do this based on the current project in scope?
I think this would be a less invasive change.

FWIW my review can only help in these small matters of coding, I can't really review this from the perspective of the intended effect, I am just a clang developer and I never have to package or install LLVM I built myself, I just test stuff from the build dir :)

Ericson2314 added inline comments.Jun 9 2022, 7:42 PM
bolt/tools/CMakeLists.txt
12–14

project is not redefined in the unified / monorepo build, so we cannot rely on it.

12–14

I am just a clang developer and I never have to package or install LLVM I built myself, I just test stuff from the build dir :)

You mean you build clang against a prebuilt LLVM? I am confused.

nikic accepted this revision.Jun 10 2022, 6:38 AM

Looks reasonable to me.

This revision is now accepted and ready to land.Jun 10 2022, 6:38 AM
This revision was landed with ongoing or failed builds.Jun 10 2022, 7:35 AM
This revision was automatically updated to reflect the committed changes.
sberg added a subscriber: sberg.Jun 10 2022, 10:07 AM

There might be something fishy with this commit. My fresh from-scratch cmake --build . && cmake --build . --target install monorepo build now lacked e.g. the bin/clang++ symlink in the installation dir, and locally reverting this commit fixed that.

dyung added a subscriber: dyung.Jun 10 2022, 7:41 PM

Just another data point, but we noticed the same issue of clang++ missing after install on our internal Windows builds, and I bisected it to this commit.

We saw the same thing at Intel FWIW. When I investigated it, all of the symlinks were being created as name} instead of whatever you'd expect the symlink to be named.

nikic added inline comments.Jun 11 2022, 6:27 AM
clang/cmake/modules/AddClang.cmake
183–190

Yeah, there's a typo on this line. Should be ${name}.

Ericson2314 reopened this revision.Jun 11 2022, 10:02 AM

Thanks for finding this and sorry for the disruption. Hope the revert didn't come to slowly.

This revision is now accepted and ready to land.Jun 11 2022, 10:02 AM

Fix typo

Thank you @aaron.ballman and @nikic for finding the culprit!

Ericson2314 marked an inline comment as done.Jun 11 2022, 10:03 AM
Ericson2314 marked an inline comment as done.
Ericson2314 added inline comments.
openmp/tools/CMakeLists.txt
1–3 ↗(On Diff #402289)

Moved this stuff make sure it is used properly.

Hmm, are all those errors from running out of file system space, or might there be another issue?

Rebase again, to try to figure out if last round of errors were spurious

https://reviews.llvm.org/D130254 oh yay I learned from this the openmp failure is in fact spurious!

nikic accepted this revision.Thu, Jul 21, 11:32 AM

Still LGTM

This revision was landed with ongoing or failed builds.Thu, Jul 21, 12:04 PM
This revision was automatically updated to reflect the committed changes.

I think this commit broke our standalone builds:

CMake Error at utils/hmaptool/CMakeLists.txt:1 (install):

install PROGRAMS given no DESTINATION!

Was the LLVM_TOOLS_INSTALL_DIR in this file supposed to be changed to CLANG_TOOLS_INSTALL_DIR ?

https://lab.llvm.org/buildbot#builders/74/builds/12262 here is a failure with only me in the blamelist, but I cannot see how those failures have anything to do with this?

@tstellar Thank you! That is an error I do understand, and yes your example is exactly right.

Sorry, I thought I had checked every rebase that no new usages cropped up, but I guess I missed one.

Ericson2314 added a comment.EditedFri, Jul 22, 7:25 AM

Will make patch for it.

*edit* it's https://reviews.llvm.org/D130362

This change broke the LLVMConfig generation and now when including llvm through LLVM_DIR in another project such as onnx-mlir, various tools no longer have the correct paths. For example, before this change:

set(LLVM_TOOLS_BINARY_DIR "${LLVM_INSTALL_PREFIX}/bin")
set(LLVM_DEFAULT_EXTERNAL_LIT "${LLVM_INSTALL_PREFIX}/bin/llvm-lit")

After this change:

set(LLVM_DEFAULT_EXTERNAL_LIT "/__w/1/b/llvm/Release/./bin/llvm-lit")
set(LLVM_TOOLS_BINARY_DIR "/__w/1/b/llvm/Release/./bin")

Note that the new paths are both _absolute_ paths (which is not great for an install scenario) as well as _wrong_ because they point to the build directory even in an install scenario.

The offending line is actually one that was removed from here: /llvm-project/llvm/cmake/modules/CMakeLists.txt:

extend_path(LLVM_CONFIG_TOOLS_BINARY_DIR "\${LLVM_INSTALL_PREFIX}" "${LLVM_TOOLS_INSTALL_DIR}")

Previously (before this line was removed), LLVM_CONFIG_TOOLS_BINARY_DIR was set to the join of LLVM_TOOLS_INSTALL_DIR and LLVM_INSTALL_PREFIX, but now that this is gone, it is simply set to:

set(LLVM_CONFIG_TOOLS_BINARY_DIR "${LLVM_TOOLS_BINARY_DIR}")

where LLVM_TOOLS_BINARY_DIR is an absolute path to the binary directory (which is fine for LLVMConfig in the binary tree) and LLVM_CONFIG_TOOLS_BINARY_DIR is never updated to point to an install location for the install tree.

This change broke the LLVMConfig generation and now when including llvm through LLVM_DIR in another project such as onnx-mlir, various tools no longer have the correct paths. For example, before this change:

set(LLVM_TOOLS_BINARY_DIR "${LLVM_INSTALL_PREFIX}/bin")
set(LLVM_DEFAULT_EXTERNAL_LIT "${LLVM_INSTALL_PREFIX}/bin/llvm-lit")

After this change:

set(LLVM_DEFAULT_EXTERNAL_LIT "/__w/1/b/llvm/Release/./bin/llvm-lit")
set(LLVM_TOOLS_BINARY_DIR "/__w/1/b/llvm/Release/./bin")

Note that the new paths are both _absolute_ paths (which is not great for an install scenario) as well as _wrong_ because they point to the build directory even in an install scenario.

The offending line is actually one that was removed from here: /llvm-project/llvm/cmake/modules/CMakeLists.txt:

extend_path(LLVM_CONFIG_TOOLS_BINARY_DIR "\${LLVM_INSTALL_PREFIX}" "${LLVM_TOOLS_INSTALL_DIR}")

Previously (before this line was removed), LLVM_CONFIG_TOOLS_BINARY_DIR was set to the join of LLVM_TOOLS_INSTALL_DIR and LLVM_INSTALL_PREFIX, but now that this is gone, it is simply set to:

set(LLVM_CONFIG_TOOLS_BINARY_DIR "${LLVM_TOOLS_BINARY_DIR}")

where LLVM_TOOLS_BINARY_DIR is an absolute path to the binary directory (which is fine for LLVMConfig in the binary tree) and LLVM_CONFIG_TOOLS_BINARY_DIR is never updated to point to an install location for the install tree.

I am going to try https://reviews.llvm.org/D130545 which was committed today to see if it has resolved the issue. Linking here for reference.

Thanks @stella.stamenova for looking into this. I agree the extend_path should go back so it properly becomes the install dir. I can't think why I removed it in the first place! The point was to not export the other var, and indeed having that one properly alternate between the build dir vs install dir is why we can get rid of exporting the other one.