[PATCH] D81967: [clang-tools-extra] Prevent linking to duplicate .a libs and dylib

Wed Jun 17 09:08:43 PDT 2020

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks! I understand the clangd stuff better now, and scanning through the other changes they seem to be the same pattern. LGTM

================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:105
+
+clang_target_link_libraries(clangDaemon
+  PRIVATE
----------------
mgorny wrote:
> sammccall wrote:
> > This has split our link dependencies in two, and I'm not really sure why (and so am likely to put future dependencies in the wrong place).
> > 
> > Can *all* the link dependencies be moved to the clang_target_link_libraries section?
> > ((Even if this addresses a problem only seen with clang libs, I'd rather have everything in one place)
> No. `clang_target_link_libraries` is only for libraries that are included in `libclang-cpp.so`, so when the latter is built, they are replaced with it entirely. I'm all for improving this (e.g. to automatically replace only these libs that are included in clang-cpp) but I don't really have time to work on this beyond fixing immediate failures.
ah, thanks. Sounds like it should be `target_link_clang_libraries` then :-)

================
Comment at: clang-tools-extra/clangd/unittests/CMakeLists.txt:123
   clangTidy
-  LLVMSupport
   LLVMTestingSupport
----------------
mgorny wrote:
> sammccall wrote:
> > why this change? We do depend directly on LLVMSupport, and I'd prefer that to remain explicit rather than pick it up transitively.
> It's already listed in `LLVM_LINK_COMPONENTS` which handles using libLLVM correctly. Listing it here results in another copy of LLVMSupport being linked in which caused the test to crash immediately on duplicate command-line options.
Thanks, sounds like we should move TestingSupport there too (but will do that separately).

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81967/new/

https://reviews.llvm.org/D81967