[PATCH] D91859: [clangd] Fix shared-lib builds

This revision was automatically updated to reflect the committed changes. Closed by commit rGf726101b6240: [clangd] Fix shared-lib builds (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91859/new/

[PATCH] D91859: [clangd] Fix shared-lib builds

kadircet updated this revision to Diff 307281. kadircet added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. - Link protobuf and grpc++ publicly to generated targets Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D91859: [clangd] Fix shared-lib builds

kbobyrev added a comment. In D91859#2410252 , @nridge wrote: > In D91859#2410002 , @kbobyrev wrote: > >> Ah, right. I think `clangd-remote-index` needs `protobuf` dependency. >> @nridge, I believe this should fix

[PATCH] D91859: [clangd] Fix shared-lib builds

nridge added a comment. In D91859#2410002 , @kbobyrev wrote: > Ah, right. I think `clangd-remote-index` needs `protobuf` dependency. > @nridge, I believe this should fix the problem you're seeing. Not sure if this is what you meant, but I fixed it by

[PATCH] D91859: [clangd] Fix shared-lib builds

nridge added a comment. In D91859#2410166 , @kadircet wrote: > In D91859#2409743 , @nridge wrote: > >> I applied the patch locally and it fixes most of the linker errors but I'm >> still seeing one: >> >> [...] >

[PATCH] D91859: [clangd] Fix shared-lib builds

kbobyrev added a comment. In D91859#2410166 , @kadircet wrote: > In D91859#2409743 , @nridge wrote: > >> I applied the patch locally and it fixes most of the linker errors but I'm >> still seeing one: >> >>

[PATCH] D91859: [clangd] Fix shared-lib builds

kadircet added a comment. In D91859#2409743 , @nridge wrote: > I applied the patch locally and it fixes most of the linker errors but I'm > still seeing one: > > [6/393] Linking CXX executable bin/clangd-index-server > FAILED: bin/clangd-index-server

[PATCH] D91859: [clangd] Fix shared-lib builds

kbobyrev accepted this revision. kbobyrev added a comment. This revision is now accepted and ready to land. Ah, right. I think `clangd-remote-index` needs `protobuf` dependency. @nridge, I believe this should fix the problem you're seeing. I'm somewhat confused as to why I'm not seeing the same

[PATCH] D91859: [clangd] Fix shared-lib builds

nridge added a comment. I applied the patch locally and it fixes most of the linker errors but I'm still seeing one: [6/393] Linking CXX executable bin/clangd-index-server FAILED: bin/clangd-index-server : && /usr/bin/clang++-10 -fPIC -fvisibility-inlines-hidden -Werror=date-time

[PATCH] D91859: [clangd] Fix shared-lib builds

kadircet updated this revision to Diff 306830. kadircet marked an inline comment as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91859/new/ https://reviews.llvm.org/D91859 Files:

[PATCH] D91859: [clangd] Fix shared-lib builds

kadircet added a subscriber: nridge. kadircet added a comment. > I don't think that's how CMake works, the whole CMakeLists tree is parsed > before anything is compiled, so it shouldn't race like that. That was also my mental model, but it looks like `include_directories` statement within the

[PATCH] D91859: [clangd] Fix shared-lib builds

kbobyrev added a comment. (also please add "Fixes https://github.com/clangd/clangd/issues/598; to the patch description to close the related issue on GH) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91859/new/ https://reviews.llvm.org/D91859

[PATCH] D91859: [clangd] Fix shared-lib builds

kbobyrev added a comment. In D91859#2408064 , @sammccall wrote: > I'm not totally following the discussion on the more complete fix (LMK if you > want me to dig into that), but if the build is broken in some configurations > we should likely prioritize

[PATCH] D91859: [clangd] Fix shared-lib builds

sammccall added a comment. I'm not totally following the discussion on the more complete fix (LMK if you want me to dig into that), but if the build is broken in some configurations we should likely prioritize fixing that over ensuring the fix is conceptually complete. > Otherwise by the time

[PATCH] D91859: [clangd] Fix shared-lib builds

kbobyrev added a comment. Actually, it would be safer if `FindGRPC` was also higher than `add_subdirectory(tool)`: right now it only includes `remote/index/Client.h` and `Client.h` does not depend on any gRPC/Protobuf headers for now but that might change at some point and break builds

[PATCH] D91859: [clangd] Fix shared-lib builds

kbobyrev requested changes to this revision. kbobyrev added a comment. This revision now requires changes to proceed. Thank you for taking care of thiis! The complete fix, however, should include two more things: 1. Moving this if (CLANGD_ENABLE_REMOTE) include(FindGRPC) endif()

[PATCH] D91859: [clangd] Fix shared-lib builds

kadircet created this revision. kadircet added reviewers: sammccall, kbobyrev. Herald added subscribers: cfe-commits, usaxena95, arphaman, mgorny. Herald added a project: clang. kadircet requested review of this revision. Herald added subscribers: MaskRay, ilya-biryukov. Repository: rG LLVM