This is an archive of the discontinued LLVM Phabricator instance.

[llvm] [cmake] Move LLVM_INSTALL_PACKAGE_DIR top-level to fix llvm-config
ClosedPublic

Authored by mgorny on Aug 15 2022, 1:03 AM.

Details

Summary

Move the LLVM_INSTALL_PACKAGE_DIR declaration from llvm/cmake/modules
directory to the top-level llvm/CMakeLists.txt, in order to fix
the regression in llvm-config --cmakedir output for installed LLVM.
Since the tools directory is processed by CMake prior to
llvm/cmake/modules, the llvm-config executable ended up using
the variable prior to it being defined and incorrectly used an empty
path, resulting in e.g.:

$ llvm-config --cmakedir
/usr/lib/llvm/16/

With this patch, the path is defined (and therefore the default value
is being set) prior to adding the tools subdirectory and llvm-config
starts working correctly:

$ llvm-config --cmakedir
/usr/lib/llvm/16/lib64/cmake/llvm

This fixes a regression introduced by D130539. Thanks to Petr Polezhaev
for reporting the problem @ https://bugs.gentoo.org/865165

Diff Detail

Event Timeline

mgorny created this revision.Aug 15 2022, 1:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2022, 1:03 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
mgorny requested review of this revision.Aug 15 2022, 1:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2022, 1:03 AM

Can we add comment saying why it is this way? This breaks the pattern of how it is everywhere else, which is fine. But I want anyone reading the code to know why the pattern is broken here (llvm-config).

mgorny updated this revision to Diff 452676.Aug 15 2022, 8:12 AM

Add a comment.

Can we add comment saying why it is this way? This breaks the pattern of how it is everywhere else, which is fine. But I want anyone reading the code to know why the pattern is broken here (llvm-config).

Done.

Ericson2314 accepted this revision.Aug 15 2022, 8:17 AM
This revision is now accepted and ready to land.Aug 15 2022, 8:17 AM

BTW this should be backported to the LLVM 15 release branch too.

This revision was landed with ongoing or failed builds.Aug 15 2022, 8:21 AM
This revision was automatically updated to reflect the committed changes.

BTW this should be backported to the LLVM 15 release branch too.

I'm going to open a bug after giving buildbots some time to test it.

Great; sounds good. Happy to review that one too when the time comes.