[CMake] Fix FindGRPC cmake module to allow different layering

[steven_wu]

Summary

Take the library target out of generate_protos function so the caller
can decide where to layer the library using the source generated from
the function.

Fixes: https://github.com/llvm/llvm-project/issues/58075

[sammccall]

Hmm, as mentioned on the bug, I think wrapping the deps and generated sources into a library is an important part of what the build rule does.

If we need to split it up like this, I think a generate_clang_protos wrapper in FindGRPC.cmake might be the right place to encapsulate it.

[steven_wu]

Address review feedback

[steven_wu]

Was missing the latest feedback. Bump FindGRPC module to top layer.

[sammccall]

thanks!

cmake/Modules/FindGRPC.cmake

111

changing from function to macro means all the local variables here are done in the parent scope, which is hard to reason about.

I think the only reason you're doing that here is to return the filename in the ProtoSource variable, in which case set(${GeneratedSource} ${GeneratedProtoSource} PARENT_SCOPE) should do the job from a function.

111

nit: the plural is in the wrong place: rather generate_proto_sources?
this generates the multiple sources for a single proto, rather than the other way around.

(I'm not sure why the original was plural, but this makes it somehow more confusing)

[steven_wu]

cmake/Modules/FindGRPC.cmake

111

The reason why it is a macro is I don't know any elegant way to pass all the DEPEND down to add_*_library.

[steven_wu]

Minor rename

[sammccall]

cmake/Modules/FindGRPC.cmake

111

The reason why it is a macro is I don't know any elegant way to pass all the DEPEND down to add_*_library.

... oh, yikes, we're passing through by pretending they're ${ProtoFile}, I didn't see that :-\

I guess the easiest option is just to change that name to something like ProtoArgs in the wrapping function, and add a bit of documentation.
I think using CMAKE_PARSE_ARGUMENTS in the wrapping function is an option though, and then pass the unparsed arguments through?

[steven_wu]

cmake/Modules/FindGRPC.cmake

111

Ah, I just learnt the unparsed_argument option. Then it is not too bad. Fixing.

[steven_wu]

Turn generate_proto_sources back to function and forward argument more elegantly.

[sammccall]

Thanks, LG!

clang-tools-extra/clangd/CMakeLists.txt

 if (LLVM_INCLUDE_BENCHMARKS)
   add_subdirectory(benchmarks)
 endif()
 if ( CLANGD_BUILD_XPC )
   add_subdirectory(xpc)
 endif ()
 
 if (CLANGD_ENABLE_REMOTE)
-  include(FindGRPC)
+  include(AddGRPC)
 endif()
 
 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/index/remote/CMakeLists.txt

 if (CLANGD_ENABLE_REMOTE)
-  generate_protos(clangdRemoteIndexProto "Index.proto")
-  generate_protos(clangdMonitoringServiceProto "MonitoringService.proto"
+  generate_clang_protos_library(clangdRemoteIndexProto "Index.proto")
+  generate_clang_protos_library(clangdMonitoringServiceProto "MonitoringService.proto"
     GRPC)
-  generate_protos(clangdRemoteIndexServiceProto "Service.proto"
+  generate_clang_protos_library(clangdRemoteIndexServiceProto "Service.proto"
     DEPENDS "Index.proto"
     GRPC)
   # FIXME: Move this into generate_protos. Currently we only mention proto
   # filename as a dependency, but linking requires target name.
   target_link_libraries(clangdRemoteIndexServiceProto
     PRIVATE
     clangdRemoteIndexProto
     clangdMonitoringServiceProto

clang/cmake/modules/AddGRPC.cmake

+include(FindGRPC)
+
+function(generate_clang_protos_library LibraryName ProtoFile)
+  # Take the first two args and forward the remaining to generate_proto_sources.
+  cmake_parse_arguments(PARSE_ARGV 2 PROTO "" "" "")
+  generate_proto_sources(ProtoSource ${ProtoFile} ${PROTO_UNPARSED_ARGUMENTS})
+
+  add_clang_library(${LibraryName} ${ProtoSource}
+    PARTIAL_SOURCES_INTENDED
+    LINK_LIBS PUBLIC grpc++ protobuf)
+endfunction()

cmake/Modules/FindGRPC.cmake

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)
+function(generate_proto_sources GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath "${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
   get_filename_component(Basename ${ProtoSourceAbsolutePath} NAME_WLE)
 
   set(GeneratedProtoSource "${CMAKE_CURRENT_BINARY_DIR}/${Basename}.pb.cc")
   set(GeneratedProtoHeader "${CMAKE_CURRENT_BINARY_DIR}/${Basename}.pb.h")
   set(Flags
     --cpp_out="${CMAKE_CURRENT_BINARY_DIR}"
     --proto_path="${ProtoSourcePath}")
   if (PROTO_GRPC)
     list(APPEND Flags
       --grpc_out="${CMAKE_CURRENT_BINARY_DIR}"
       --plugin=protoc-gen-grpc="${GRPC_CPP_PLUGIN}")
     list(APPEND GeneratedProtoSource "${CMAKE_CURRENT_BINARY_DIR}/${Basename}.grpc.pb.cc")
     list(APPEND GeneratedProtoHeader "${CMAKE_CURRENT_BINARY_DIR}/${Basename}.grpc.pb.h")
   endif()
   add_custom_command(
         OUTPUT ${GeneratedProtoSource} ${GeneratedProtoHeader}
         COMMAND ${PROTOC}
         ARGS ${Flags} "${ProtoSourceAbsolutePath}"
         DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-    PARTIAL_SOURCES_INTENDED
-    LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource} PARENT_SCOPE)
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
   # the source dir, the generated headers we need are in the binary dir.
   foreach(ImportedProto IN LISTS PROTO_DEPENDS)
     # Foo.proto -> Foo.pb.h
     STRING(REGEX REPLACE "\\.proto$" ".pb.h" ImportedHeader "${ImportedProto}")
     # Foo.pb.h -> ${CMAKE_CURRENT_BINARY_DIR}/Foo.pb.h