This is an archive of the discontinued LLVM Phabricator instance.

Differential D95653

[clang-tidy] Fix linking tests to LLVMTestingSupport
ClosedPublic

Authored by mgorny on Jan 28 2021, 5:18 PM.

Details

Reviewers
njames93
aaron.ballman
alexfh
Commits
rG632545e8ce84: [clang-tidy] Fix linking tests to LLVMTestingSupport
Summary

LLVMTestingSupport is not part of libLLVM, and therefore can not
be linked to via LLVM_LINK_COMPONENTS. Instead, it needs to be
specified explicitly to ensure that it is linked explicitly
even if LLVM_LINK_LLVM_DYLIB is used. This is consistent with handling
in clangd.

Fixes PR#48931

Diff Detail

Event Timeline

mgorny created this revision.Jan 28 2021, 5:18 PM
Herald added subscribers: usaxena95, kadircet, xazax.hun. · View Herald TranscriptJan 28 2021, 5:18 PM
mgorny requested review of this revision.Jan 28 2021, 5:18 PM
Herald added a subscriber: ilya-biryukov. · View Herald TranscriptJan 28 2021, 5:18 PM
njames93 added a comment.Jan 28 2021, 7:15 PM

I think I introduced this failure due to my abysmal knowledge of CMake and LLVM's library structure, So I'm definitely not qualified to say if this is a good fix, but seeing as it fixes the linker error I'll give it a tentative LG.

njames93 retitled this revision from [cte] [clang-tidy] Fix linking tests to LLVMTestingSupport to [clang-tidy] Fix linking tests to LLVMTestingSupport.Jan 28 2021, 7:16 PM
njames93 added a reviewer: alexfh.
njames93 added a project: Restricted Project.
mgorny added a comment.Jan 29 2021, 12:51 AM
In D95653#2529776, @njames93 wrote:

I think I introduced this failure due to my abysmal knowledge of CMake and LLVM's library structure, So I'm definitely not qualified to say if this is a good fix, but seeing as it fixes the linker error I'll give it a tentative LG.

I'm a bit confused now. Are you telling that I should push it or wait for another review?

aaron.ballman accepted this revision.Jan 29 2021, 5:11 AM

LGTM

This revision is now accepted and ready to land.Jan 29 2021, 5:11 AM
njames93 added a comment.EditedJan 29 2021, 6:00 AM
In D95653#2530163, @mgorny wrote:
In D95653#2529776, @njames93 wrote:

I think I introduced this failure due to my abysmal knowledge of CMake and LLVM's library structure, So I'm definitely not qualified to say if this is a good fix, but seeing as it fixes the linker error I'll give it a tentative LG.

I'm a bit confused now. Are you telling that I should push it or wait for another review?

I was just saying wait for another reviewer to approve it. But it's all good now

Closed by commit rG632545e8ce84: [clang-tidy] Fix linking tests to LLVMTestingSupport (authored by mgorny). · Explain WhyJan 29 2021, 12:54 PM
This revision was automatically updated to reflect the committed changes.
mgorny added a commit: rG632545e8ce84: [clang-tidy] Fix linking tests to LLVMTestingSupport.
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2021, 12:54 PM

Revision Contents

PathSize
clang-tools-extra/
unittests/
clang-tidy/
2 lines

Diff 320193

clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

set(LLVM_LINK_COMPONENTS set(LLVM_LINK_COMPONENTS
FrontendOpenMP FrontendOpenMP
Support Support
TestingSupport
) )
get_filename_component(CLANG_LINT_SOURCE_DIR get_filename_component(CLANG_LINT_SOURCE_DIR
${CMAKE_CURRENT_SOURCE_DIR}/../../clang-tidy REALPATH) ${CMAKE_CURRENT_SOURCE_DIR}/../../clang-tidy REALPATH)
include_directories(${CLANG_LINT_SOURCE_DIR}) include_directories(${CLANG_LINT_SOURCE_DIR})
add_extra_unittest(ClangTidyTests add_extra_unittest(ClangTidyTests
AddConstTest.cpp AddConstTest.cpp
Show All 28 Linestarget_link_libraries(ClangTidyTests
PRIVATE PRIVATE
clangTidy clangTidy
clangTidyAndroidModule clangTidyAndroidModule
clangTidyGoogleModule clangTidyGoogleModule
clangTidyLLVMModule clangTidyLLVMModule
clangTidyObjCModule clangTidyObjCModule
clangTidyReadabilityModule clangTidyReadabilityModule
clangTidyUtils clangTidyUtils
LLVMTestingSupport
) )