[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 ↗

(On Diff #330675)

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/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)