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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:158
+if (LLVM_USE_GRPC)
+  set(protobuf_MODULE_COMPATIBLE TRUE)
+  find_package(Protobuf CONFIG REQUIRED HINTS ${GRPC_INSTALL_PATH})
----------------
can we add a comment to the top of this section describing the setup that it expects? (GRPC built from source and installed into ${GRPC_INSTALL_PATH})


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:164
+
+  set_target_properties(protobuf::libprotobuf PROPERTIES IMPORTED_GLOBAL TRUE)
+  add_library(protobuf ALIAS protobuf::libprotobuf)
----------------
Comment: "grpc cmakelists gives the libraries slightly odd names, make them match the conventional system-installed names"


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:170
+
+  # NOTE: Callers of the function should also add CMake source directory to
+  # include path.
----------------
Add primary function comment before the note.

this comment is a bit confusing:
 - it says callers of the function, when it means users of the proto
 - it says source directory, I think it means binary directory
 - it doesn't say why
 
I'd suggest:
  Proto headers are generated in ${CMAKE_CURRENT_BINARY_DIR}.
  Libraries that use these headers should adjust the include path.


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:178
+    set(GeneratedProtoHeader "${CMAKE_CURRENT_BINARY_DIR}/Index.pb.h")
+    set(GeneratedGRPCSource "${CMAKE_CURRENT_BINARY_DIR}/Index.grpc.pb.cc")
+    set(GeneratedGRPCHeader "${CMAKE_CURRENT_BINARY_DIR}/Index.grpc.pb.h")
----------------
this should be optional, with a parameter to generate_protos (along with the plugin and grpc_out args) - AIUI it's required for protos that define services only.

Conventional name for this param is HasServices


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:182
+          OUTPUT "${GeneratedProtoSource}" "${GeneratedProtoHeader}" "${GeneratedGRPCSource}" "${GeneratedGRPCHeader}"
+          COMMAND ${GRPC_INSTALL_PATH}/bin/protoc
+          ARGS --grpc_out="${CMAKE_CURRENT_BINARY_DIR}"
----------------
I believe this should be Protobuf_DIR or so (find_package output) to avoid getting confused if find_package found it somewhere else


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:191
+    add_library(${LibraryName} ${GeneratedProtoSource} ${GeneratedProtoHeader} ${GeneratedGRPCSource} ${GeneratedGRPCHeader})
+    target_link_libraries(${LibraryName} gRPC::grpc++ protobuf::libprotobuf)
+  endfunction()
----------------
nit: grpc++ protobuf


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:194
+
+  add_definitions(-DGOOGLE_PROTOBUF_NO_RTTI=1)
+  include_directories(${Protobuf_INCLUDE_DIRS})
----------------
these global side-effects are pretty scary, can we limit them somehow?


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:157
+# FIXME(kirillbobyrev): Move the gRPC setup logic to llvm/CMakeLists.txt
+if (LLVM_USE_GRPC)
+  set(protobuf_MODULE_COMPATIBLE TRUE)
----------------
Having thought about this more - I think this flag should (at least for now) be called something like `CLANGD_ENABLE_REMOTE` and documented as requiring grpc.


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:158
+if (LLVM_USE_GRPC)
+  set(protobuf_MODULE_COMPATIBLE TRUE)
+  find_package(Protobuf CONFIG REQUIRED HINTS ${GRPC_INSTALL_PATH})
----------------
I think this code should be moved to `llvm/cmake/modules/FindGRPC.cmake` and included here via `include(FindGRPC)`


================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:11
+
+package remote;
+
----------------
package clang.clangd.remote, to get namespaces compatible with the c++ code


================
Comment at: clang-tools-extra/clangd/index/remote/server/CMakeLists.txt:12
+
+  ${PROTOBUF_LIBRARY}
+  ${GRPC_LIBRARY}
----------------
protobuf and grpc++?


================
Comment at: llvm/CMakeLists.txt:1112
+# FIXME(kirillbobyrev): Document this in the LLVM docs.
+option(LLVM_USE_GRPC "Use gRPC library to enable remote index support for Clangd" OFF)
+# FIXME(kirillbobyrev): Check if it works with optimized tablegen CMake option.
----------------
Sorry my thought before was incomplete - this should probably be CLANGD_USE_GRPC for now with a fixme to move up to LLVM. Especially since all the cmake code for this is in clangd for now.


================
Comment at: llvm/CMakeLists.txt:1114
+# FIXME(kirillbobyrev): Check if it works with optimized tablegen CMake option.
+set(GRPC_INSTALL_PATH "/usr" CACHE PATH "Path to gRPC library installation.")
----------------
This doesn't make sense - it's not a universally-usable directory, and there's almost no chance that the cmake files are in /usr/lib/cmake/... since the packages we've found don't include them.

I'd suggest for now we always require this to be set if using an installed-grpc-with-cmakelists setup, and work out how to relax later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77794