[PATCH] D77794: [clangd] Pull installed gRPC and introduce shared-index-(server|client)

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:135
   )
 
 add_subdirectory(refactor/tweaks)
----------------
can you somewhere (shared/README.md?) write down how to get the build working on at least one platform?

e.g. 
1. install `libgrpc++-dev libprotobuf-dev protobuf-compiler protobuf-compiler-grpc` debian packages
2. set -DGRPC_INSTALL_PATH=/usr/include (seems like at least in that case we shouldn't need to set it explicitly!)

But looking at the find_package line I have a sneaking suspicion that "installed" doesn't mean what I think it does. My grpc++ package didn't come with any cmake files. It has headers and static libraries though...

I was able to get this to build by:
 - installing  the packages above
 - changing $GRPC_INSTALL_PATH/protoc to protoc
 - changing the dependency names to grpc++ and protobuf
 - changing the plugin line to --plugin=protoc-gen-grpc=/usr/bin/grpc_cpp_plugin (this needs detection) 

This seems pretty reasonable (more so than requiring everyone to check out grpc and build from source), we're just missing the detection logic. Unless grpc provides a cmake recipe that discovers the system-installed (i.e. package manager) grpc, maybe we need to write this  detection.


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:156
+
+option(GRPC_INSTALL_PATH "Path to gRPC library installation." OFF)
+if (GRPC_INSTALL_PATH)
----------------
sammccall wrote:
> We'll eventually want to move all this out of clangd I guess? Maybe add FIXMEs for that?
this should be controlled by a top-level LLVM_ENABLE_GRPC or LLVM_USE_GRPC or so flag (not sure what the convention is).
Using GRPC_INSTALL_PATH is tempting now but there are multiple ways to configure it so it won't scale.
And at least in some cases we should be able to autodetect it, so we'll need a dedicated enable flag.


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:156
+
+option(GRPC_INSTALL_PATH "Path to gRPC library installation." OFF)
+if (GRPC_INSTALL_PATH)
----------------
sammccall wrote:
> sammccall wrote:
> > We'll eventually want to move all this out of clangd I guess? Maybe add FIXMEs for that?
> this should be controlled by a top-level LLVM_ENABLE_GRPC or LLVM_USE_GRPC or so flag (not sure what the convention is).
> Using GRPC_INSTALL_PATH is tempting now but there are multiple ways to configure it so it won't scale.
> And at least in some cases we should be able to autodetect it, so we'll need a dedicated enable flag.
GRPC_INSTALL_PATH should be set() instead of option().


================
Comment at: clang-tools-extra/clangd/index/shared/CMakeLists.txt:21
+
+add_clang_executable(shared-index-server
+  SharedIndexServer.cpp
----------------
these targets should  be called clangd-index-server etc, this is a global namespace


================
Comment at: clang-tools-extra/clangd/index/shared/CMakeLists.txt:54
+  gRPC::grpc++
+  clangDaemon
+  )
----------------
why is the client depending on clangd?


================
Comment at: clang-tools-extra/clangd/index/shared/SharedIndexServer.cpp:96
+
+  grpc::EnableDefaultHealthCheckService(true);
+  grpc::ServerBuilder Builder;
----------------
you should include the header for this function. In the version of grpc I installed, it wasn't transitively included.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77794