[clangd] Optionally add reflection for clangd-index-server

[kbobyrev]

Summary

This was originally landed without the optional part and reverted later:

https://github.com/llvm/llvm-project/commit/8080ea4c4b8c456c72c617587cc32f174b3105c1

[kadircet]

clang-tools-extra/clangd/index/remote/server/CMakeLists.txt

12

can we move this piece into FindGRPC.cmake instead?

llvm/cmake/modules/FindGRPC.cmake

21

nit: is this change needed?

[kbobyrev]

Address review comments.

clang-tools-extra/clangd/index/remote/server/CMakeLists.txt

12

Yes, but I think only the set(REFLECTION_LIBRARY grpc++_reflection) part? Having the -DENABLE_GRPC_REFLECTION definition globally is probably not very nice and as with the other target under clangd/index/remote, add_target_definition doesn't work because of add_clan_executable :(

[kadircet]

clang-tools-extra/clangd/index/remote/server/CMakeLists.txt

12

ah i forgot about that one, can we have this in clang-tools-extra/clangd/Features.inc.in then, with rest of our definitions?

you might also want to canonicalize the option via llvm_canonicalize_cmake_booleans

[kbobyrev]

Add the variable to Features.inc.in and canonicalize.

[kadircet]

clang-tools-extra/clangd/index/remote/server/CMakeLists.txt

12

this is not needed anymore right ?

clang-tools-extra/clangd/index/remote/server/Server.cpp

38

this should be #if rather than an #ifdef, same below.

[kbobyrev]

Resolve review comments.

[kbobyrev]

Include Features.inc in Server.cpp.

[kbobyrev]

Add explicit CMake option.

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/CMakeLists.txt

190

i think this option belongs to FindGRPC.cmake, as it configures the grpc linking behavior in general, not something specific to clangd.

[kbobyrev]

Resolve post-LGTM comments.

clang-tools-extra/clangd/CMakeLists.txt

if (NOT DEFINED CLANGD_BUILD_XPC)
   unset(CLANGD_BUILD_XPC_DEFAULT)
 endif ()
 
 option(CLANGD_MALLOC_TRIM "Call malloc_trim(3) periodically in Clangd. (only takes effect when using glibc)" ON)
 
 llvm_canonicalize_cmake_booleans(
   CLANGD_BUILD_XPC
   CLANGD_ENABLE_REMOTE
+  ENABLE_GRPC_REFLECTION
   CLANGD_MALLOC_TRIM
   LLVM_ENABLE_ZLIB
 )
 
 configure_file(
   ${CMAKE_CURRENT_SOURCE_DIR}/Features.inc.in
   ${CMAKE_CURRENT_BINARY_DIR}/Features.inc
 )

 
 if(CLANG_INCLUDE_TESTS)
   add_subdirectory(test)
   add_subdirectory(unittests)
 endif()
 
 # FIXME(kirillbobyrev): Document this in the LLVM docs once remote index is stable.
 option(CLANGD_ENABLE_REMOTE "Use gRPC library to enable remote index support for Clangd" OFF)
 set(GRPC_INSTALL_PATH "" CACHE PATH "Path to gRPC library manual installation.")
 
 add_subdirectory(index/remote)
 add_subdirectory(index/dex/dexp)

clang-tools-extra/clangd/Features.inc.in

 #define CLANGD_BUILD_XPC @CLANGD_BUILD_XPC@
 #define CLANGD_ENABLE_REMOTE @CLANGD_ENABLE_REMOTE@
+#define ENABLE_GRPC_REFLECTION @ENABLE_GRPC_REFLECTION@
 #define CLANGD_MALLOC_TRIM @CLANGD_MALLOC_TRIM@

clang-tools-extra/clangd/index/remote/server/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 add_clang_executable(clangd-index-server
   Server.cpp
 
   DEPENDS
   RemoteIndexProto
   RemoteIndexServiceProto
   )
 
+if (ENABLE_GRPC_REFLECTION)
+  add_definitions(-DENABLE_GRPC_REFLECTION)
+endif()
+
 target_link_libraries(clangd-index-server
   PRIVATE
   clangDaemon
   clangdSupport
 
   RemoteIndexProto
   RemoteIndexServiceProto
   clangdRemoteMarshalling
+
+  ${REFLECTION_LIBRARY}
   )

clang-tools-extra/clangd/index/remote/server/Server.cpp

 //===--- Server.cpp - gRPC-based Remote Index Server  ---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Index.pb.h"
 #include "Service.grpc.pb.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
 #include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
 #include "support/Context.h"
 #include "support/Logger.h"
 #include "support/Shutdown.h"

 #include "llvm/Support/VirtualFileSystem.h"
 
 #include <chrono>
 #include <grpc++/grpc++.h>
 #include <grpc++/health_check_service_interface.h>
 #include <memory>
 #include <thread>
 
+#ifdef ENABLE_GRPC_REFLECTION
+#include <grpc++/ext/proto_server_reflection_plugin.h>
+#endif
+
 namespace clang {
 namespace clangd {
 namespace remote {
 namespace {
 
 static constexpr char Overview[] = R"(
 This is an experimental remote index implementation. The server opens Dex and
 awaits gRPC lookup requests from the client.
log("New index version loaded. Last modification time: {0}, size: {1} bytes.",
       Status->getLastModificationTime(), Status->getSize());
 }
 
 void runServerAndWait(clangd::SymbolIndex &Index, llvm::StringRef ServerAddress,
                       llvm::StringRef IndexPath) {
   RemoteIndexServer Service(Index, IndexRoot);
 
   grpc::EnableDefaultHealthCheckService(true);
+#ifdef ENABLE_GRPC_REFLECTION
+  grpc::reflection::InitProtoReflectionServerBuilderPlugin();
+#endif
   grpc::ServerBuilder Builder;
   Builder.AddListeningPort(ServerAddress.str(),
                            grpc::InsecureServerCredentials());
   Builder.AddChannelArgument(GRPC_ARG_MAX_CONNECTION_IDLE_MS,
                              IdleTimeoutSeconds * 1000);
   Builder.RegisterService(&Service);
   std::unique_ptr<grpc::Server> Server(Builder.BuildAndStart());
   log("Server listening on {0}", ServerAddress);

llvm/cmake/modules/FindGRPC.cmake

if (GRPC_INSTALL_PATH)
   message(STATUS "Using protobuf ${protobuf_VERSION}")
   find_package(gRPC CONFIG REQUIRED HINTS ${GRPC_INSTALL_PATH})
   message(STATUS "Using gRPC ${gRPC_VERSION}")
 
   include_directories(${Protobuf_INCLUDE_DIRS})
 
   # gRPC CMake CONFIG gives the libraries slightly odd names, make them match
   # the conventional system-installed names.
   set_target_properties(protobuf::libprotobuf PROPERTIES IMPORTED_GLOBAL TRUE)
   add_library(protobuf ALIAS protobuf::libprotobuf)
   set_target_properties(gRPC::grpc++ PROPERTIES IMPORTED_GLOBAL TRUE)
   add_library(grpc++ ALIAS gRPC::grpc++)
+  if (ENABLE_GRPC_REFLECTION)
+    set_target_properties(gRPC::grpc++_reflection PROPERTIES IMPORTED_GLOBAL TRUE)
+    add_library(grpc++_reflection ALIAS gRPC::grpc++_reflection)
+  endif()
 
   set(GRPC_CPP_PLUGIN $<TARGET_FILE:gRPC::grpc_cpp_plugin>)
   set(PROTOC ${Protobuf_PROTOC_EXECUTABLE})
 else()
   # This setup requires system-installed gRPC and Protobuf.
   # We always link dynamically in this mode. While the static libraries are
   # usually installed, the CMake files telling us *which* static libraries to
   # link are not.
if (HOMEBREW)
         list(APPEND PROTOBUF_OPTS PATHS ${PROTOBUF_HOMEBREW_PATH}/lib NO_DEFAULT_PATH)
       endif()
     endif()
   endif()
   find_library(GRPC_LIBRARY grpc++ $GRPC_OPTS REQUIRED)
   add_library(grpc++ UNKNOWN IMPORTED GLOBAL)
   message(STATUS "Using grpc++: " ${GRPC_LIBRARY})
   set_target_properties(grpc++ PROPERTIES IMPORTED_LOCATION ${GRPC_LIBRARY})
+  if (ENABLE_GRPC_REFLECTION)
+    find_library(GRPC_REFLECTION_LIBRARY grpc++_reflection $GRPC_OPTS REQUIRED)
+    add_library(grpc++_reflection UNKNOWN IMPORTED GLOBAL)
+    set_target_properties(grpc++_reflection PROPERTIES IMPORTED_LOCATION ${GRPC_REFLECTION_LIBRARY})
+  endif()
   find_library(PROTOBUF_LIBRARY protobuf $PROTOBUF_OPTS REQUIRED)
   message(STATUS "Using protobuf: " ${PROTOBUF_LIBRARY})
   add_library(protobuf UNKNOWN IMPORTED GLOBAL)
   set_target_properties(protobuf PROPERTIES IMPORTED_LOCATION ${PROTOBUF_LIBRARY})
 endif()
 
+if (ENABLE_GRPC_REFLECTION)
+  set(REFLECTION_LIBRARY grpc++_reflection)
+endif()
+
 # Proto headers are generated in ${CMAKE_CURRENT_BINARY_DIR}.
 # Libraries that use these headers should adjust the include path.
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
 function(generate_protos LibraryName ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath "${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)