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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
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:
> > 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().
Do you mean moving gRPC library setup logic to `llvm/CMakeLists.txt`?


================
Comment at: clang-tools-extra/clangd/index/shared/CMakeLists.txt:8
+set(SharedIndex_grpc_hdrs "${CMAKE_CURRENT_BINARY_DIR}/SharedIndex.grpc.pb.h")
+add_custom_command(
+      OUTPUT "${SharedIndex_proto_srcs}" "${SharedIndex_proto_hdrs}" "${SharedIndex_grpc_srcs}" "${SharedIndex_grpc_hdrs}"
----------------
sammccall wrote:
> can we wrap this in a function?
Are you proposing making a function (e.g. `generate_protos`) that would emit required source files? I'd have to do some manual dependency management in this case, that's why I did `add_custom_command` which seems to be the idiomatic way of generating targets with the right dependencies and properties (being generated files).

Could you please explain the benefit of wrapping this into a function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77794