[clangd] Remove extra dependancies for clangd

[saghir]

Summary

This patch removes dependancies for clangd that were duplicated in
915659bfa1e9fe2e2c748ac84d33881e248f9ad5.
The original patch was added to fix build failure with BUILD_SHARED_LIBS=ON.

[mstorsjo]

Thanks for looking at my suggestion! At this point, I'd defer the judgement to the code owner, @sammccall - whether it's nicer to have a tighter set of dependencies, or to reduce the risk of churn and accidentally breaking the shared libs build again.

[sammccall]

I don't have a strong opinion here (or solid understanding of dynamic linking!)

I suppose the idea is that clangdMain has dependencies from the source files in this directory, and the clangd binary has dependencies from the tweaks that we link in as object files.
This seems reasonable but also fragile - I honestly don't see a good workflow to understand exactly which deps are necessary and when this changes other than watching for failure reports on various platforms. (The static build is much simpler in this regard)

So I can see an argument (though not really the practical benefit) for minimizing the set of deps now to be more correct, and also an argument for having all targets depend on everything to minimize churn. Up to you.

[saghir]

Thank you for the review and your comments @sammccall and @mstorsjo. As I understand there is no strong opinion here either way. I think we can leave things as they are for now. I am going to abandon this patch. If someone else feels strongly about removing them, they can put up a patch for this.

[MaskRay]

llvm/cmake/modules/HandleLLVMOptions.cmake passes -Wl,-z,defs. -DBUILD_SHARED_LIBS=on builds get checking from the linker option (https://maskray.me/blog/2021-06-13-dependency-related-linker-options#z-defs). This is similar Bazel's layering_check.

For a dependency, say, clangTooling, if a source file within the clangd target (executable) uses (directly or transitively) clangTooling, the dependency should be kept, otherwise it should be removed.
If the source files are completely IWYU clean, it should be straightforward to tell whether a dependency can be removed by inspecting the #include lines.

[sammccall]

In D155540#4524219, @MaskRay wrote:

llvm/cmake/modules/HandleLLVMOptions.cmake passes -Wl,-z,defs. -DBUILD_SHARED_LIBS=on builds get checking from the linker option (https://maskray.me/blog/2021-06-13-dependency-related-linker-options#z-defs). This is similar Bazel's layering_check.

Thanks! This is good to know - it seems like this means when things break I can repro locally and should get the same results, if I anticipate a breakage I can test for it. I can't easily tell if I have *too many* deps, but that's probably not an urgent problem.

For a dependency, say, clangTooling, if a source file within the clangd target (executable) uses (directly or transitively) clangTooling, the dependency should be kept, otherwise it should be removed.
If the source files are completely IWYU clean, it should be straightforward to tell whether a dependency can be removed by inspecting the #include lines.

I'm not sure this is the case:

// clangdMain.cpp
#include "clangDaemon.h"
int result = clangDaemonThings();

// clangDaemon.h
#include "Tooling.h"
inline int clangDaemonThings() { tooling::doStuff(); }

IIUC if the call to clangDaemonThings gets inlined, now clangdMain has to be linked against Tooling, despite being IWYU-clean and not including any of its headers.

(This would be unlike bazel, which doesn't require cc_library(name="clangdMain", deps=["clangTooling"]) in that case, maybe I'm misunderstanding though...)

[MaskRay]

In D155540#4524326, @sammccall wrote:

In D155540#4524219, @MaskRay wrote:

llvm/cmake/modules/HandleLLVMOptions.cmake passes -Wl,-z,defs. -DBUILD_SHARED_LIBS=on builds get checking from the linker option (https://maskray.me/blog/2021-06-13-dependency-related-linker-options#z-defs). This is similar Bazel's layering_check.

Thanks! This is good to know - it seems like this means when things break I can repro locally and should get the same results, if I anticipate a breakage I can test for it. I can't easily tell if I have *too many* deps, but that's probably not an urgent problem.

For a dependency, say, clangTooling, if a source file within the clangd target (executable) uses (directly or transitively) clangTooling, the dependency should be kept, otherwise it should be removed.
If the source files are completely IWYU clean, it should be straightforward to tell whether a dependency can be removed by inspecting the #include lines.

I'm not sure this is the case:

// clangdMain.cpp
#include "clangDaemon.h"
int result = clangDaemonThings();

// clangDaemon.h
#include "Tooling.h"
inline int clangDaemonThings() { tooling::doStuff(); }

IIUC if the call to clangDaemonThings gets inlined, now clangdMain has to be linked against Tooling, despite being IWYU-clean and not including any of its headers.

(This would be unlike bazel, which doesn't require cc_library(name="clangdMain", deps=["clangTooling"]) in that case, maybe I'm misunderstanding though...)

You are right! This is unlike Bazel...

[lei]

I was thinking to commit this patch to minimize the dependencies if there are no objections. @MaskRay @nemanjai any preferences?

[MaskRay]

In D155540#4574613, @lei wrote:

I was thinking to commit this patch to minimize the dependencies if there are no objections. @MaskRay @nemanjai any preferences?

LGTM. Please commit it.

[lei]

Rebase to ToT

clang-tools-extra/clangd/tool/CMakeLists.txt

 endif()
 
 clang_target_link_libraries(clangdMain
   PRIVATE
   clangAST
   clangBasic
   clangFormat
   clangFrontend
-  clangLex
-  clangSema
   clangTooling
-  clangToolingCore
-  clangToolingRefactoring
   clangToolingSyntax
   )
 
 target_link_libraries(clangdMain
   PRIVATE
   clangTidy
 
   clangDaemon
   clangdRemoteIndex
   clangdSupport
   ${CLANGD_XPC_LIBS}
   )
 
 clang_target_link_libraries(clangd
   PRIVATE
   clangAST
   clangBasic
-  clangFormat
-  clangFrontend
   clangLex
   clangSema
-  clangTooling
   clangToolingCore
   clangToolingRefactoring
   clangToolingSyntax
   )
 
 target_link_libraries(clangd
   PRIVATE
   clangdMain
   clangDaemon
   clangdSupport
   )