[Support] Restore LLVMTestingSupport as an llvm component library

[nextsilicon-itay-bookstein]

Summary

The LLVMTestingSupport library is intended to be a component,
as indicated by the lookup for "testingsupport" performed in
compiler-rt standalone builds. Not tagging it as such emits
a warning in compiler-rt/cmake/Modules/CompilerRTUtils.cmake
and misses a few tests.

[nextsilicon-itay-bookstein]

ping :)

[dblaikie]

Can't say I know much about the CMake config or the difference between a component and a non-component here. Chandler's not around much here these days, so might need to find someone else more familiar with the cmake configuration.

[nextsilicon-itay-bookstein]

Added Tom (https://reviews.llvm.org/D70179, creation of add_llvm_component_library), and Michał (https://reviews.llvm.org/D55891, includes introduction of warning I'm trying to tackle)

[mgorny]

Honestly, I have no clue what the difference between a 'component library' and a regular library is. What I do know is that LLVMTestingSupport is not supposed to be installed, so if it has anything to do with that, then it's not what we're supposed to do.

[nextsilicon-itay-bookstein]

I think that an llvm 'component library' is a logical library that llvm exposes outwards.
It becomes query-able from llvm-config, and it declares other components it depends on for internal dependency management, which is then exposed outwards also.
For example, when I invoke llvm-config --ldflags --libs testingsupport, after the fix, I get this output:

-L/path/to/build/dir/lib
-lLLVMTestingSupport -lLLVMSupport -lLLVMDemangle

Without the fix, I get the warning "testingsupport library not installed, some tests will be skipped" warning, because then llvm-config cannot find it in the llvm-config --ldflags --libs testingsupport invocation in CompilerRTUtils.cmake.
As far as I can tell, the add_llvm_component_library function passes the BUILDTREE_ONLY argument through to add_llvm_library, which then controls whether it will get installed or not.

[mgorny]

In that case, I think this change is wrong. TestingSupport library is not supposed to be installed but built directly from LLVM source tree.

[nextsilicon-itay-bookstein]

That's already what happens, both before and after this change, because both invocations (add_llvm_library before the change, and add_llvm_component_library after the change) pass the BUILDTREE_ONLY flag.
Therefore the library doesn't get installed either way.

[tstellar]

A component library means is one that gets included into libLLVM.so see https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/AddLLVM.cmake#L431

Since this is more of an internal LLVM helper library, I don't think it should be included in libLLVM.so.

To solve your problem I think you just need to drop BUILDTREE_ONLY to install the library. At least for Fedora we manually install the library, because it's required by clang to run the tests.

[mgorny]

…or extract LLVM sources alongside compiler-rt and pass -DLLVM_MAIN_SRC_DIR=.... Compiler-RT will detect LLVM sources and build LLVMTestingSupport.

[nextsilicon-itay-bookstein]

My only lead for this change was that I'd assumed that at some point this code did not emit the aforementioned warning: https://github.com/llvm/llvm-project/blob/811b1736d91b301f59fbaa148ff55c32f90a3947/compiler-rt/cmake/Modules/CompilerRTUtils.cmake#L346-L352

Investigating why the command failed, I saw that executing it manually I'd get "llvm-config: unknown component name: testingsupport".
In the llvm-config code, this happens when the string supplied (in this case, "testingsupport"), does not appear as a key in the component map: https://github.com/llvm/llvm-project/blob/933e2469a2a8ad2b264f9e64c86a2f6c6e6128af/llvm/tools/llvm-config/llvm-config.cpp#L185

This led me to believe that it has to be a component, or the code in the compiler-rt standalone build has to find the library in some other way (not llvm-config).
This is because only items in LLVM_COMPONENT_LIBS are added to the AvailableComponents array which generates the dictionary.

I don't see any existing add_llvm_component_library with BUILDTREE_ONLY though, so the code in CompilerRTUtils.cmake might all be a bit of a contradiction...
Then again, I see that the struct AvailableComponent has a bool IsInstalled field, but it's always true: https://github.com/llvm/llvm-project/blob/e6e29831ddf8bb8ec03472fe69eea91dcc942124/llvm/cmake/modules/LLVM-Build.cmake#L69

Perhaps that line should set IsInstalled = !BUILDTREE_ONLY?

In D110318#3039943, @tstellar wrote:

A component library means is one that gets included into libLLVM.so see https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/AddLLVM.cmake#L431

Since this is more of an internal LLVM helper library, I don't think it should be included in libLLVM.so.

To solve your problem I think you just need to drop BUILDTREE_ONLY to install the library. At least for Fedora we manually install the library, because it's required by clang to run the tests.

Note that the comment there says "candidate for inclusion", I wonder if in the past there were conditions under which it didn't get included into libLLVM.so.

Also, I tried what you suggested; If I keep it as a non-component, and remove the BUILDTREE_ONLY flag, I get this:

CMake Error: install(EXPORT "LLVMExports" ...) includes target "LLVMTestingSupport" which requires target "gtest" that is not in any export set.

I removed the BUILDTREE_ONLY from gtest as well, but then I get:

CMake Error in utils/unittest/CMakeLists.txt:
  Target "gtest" INTERFACE_INCLUDE_DIRECTORIES property contains path:

    "/local/users/itay/Projects/next-llvm-project/llvm/utils/unittest/googletest/include"

  which is prefixed in the source directory.

llvm/lib/Testing/Support/CMakeLists.txt

-add_llvm_library(LLVMTestingSupport
+add_llvm_component_library(LLVMTestingSupport
   Annotations.cpp
   Error.cpp
   SupportHelpers.cpp
 
   BUILDTREE_ONLY
 
   ADDITIONAL_HEADER_DIRS
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/Testing/Support
 
   LINK_COMPONENTS
   Support
   )
 
 target_link_libraries(LLVMTestingSupport PRIVATE gtest)