[CMake] Support inter-proto dependencies in generate_protos.

[kadircet]

llvm/cmake/modules/FindGRPC.cmake

117

nit: I would move the fixme near to the set_source_files_properties

[sammccall]

Move FIXME to more specific place

[kbobyrev]

llvm/cmake/modules/FindGRPC.cmake

118

On line 89 the argument is called DEPENDS (also in the comments) but here it is called PROTO_DEPENDS, I think this is a typo?

124

This relies on the protos being generated from the same directory which is probably a very restrictive requirement. I don't see a better solution other than specifying dirs/full paths to the files in arguments but that does not seem nice, too. Maybe add a FIXME?

[sammccall]

llvm/cmake/modules/FindGRPC.cmake

118

the variables extracted from arguments have names derived from the argument labels (DEPENDS) by prepending a prefix (in this case PROTO, the third argument to cmake_parse_arguments). So DEPENDS is the arg, PROTO_DEPENDS is the extracted variable.

(I forgot to mention I did test this, checking that the ninja graph still shows a dependency from Service.pb.cc.o onto Index.pb.h)

124

This relies on the protos being generated from the same directory

Why? The intention was that the DEPENDS args can be relative paths (Foo.proto, dir/Bar.proto, ../Baz.proto) or absolute ones (${CMAKE_CURRENT_BINARY_DIR}/../SomeGenerated.proto).

Thus resolving the extension-substituted path against ${CMAKE_CURRENT_BINARY_DIR}, rather than simply appending it.

[kbobyrev]

LGTM, thanks!

llvm/cmake/modules/FindGRPC.cmake

118

Ahh, interesting. That's not immediately obvious :D Thanks for the explanation!

124

Ah, you're right, my bad.

[sammccall]

llvm/cmake/modules/FindGRPC.cmake

118

Agree it's not obvious - I could paste the manual for cmake_parse_arguments here but it doesn't seem appropriate :-)

124

Added some extra comments to spell this out.

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

 if (CLANGD_ENABLE_REMOTE)
-  generate_protos(RemoteIndexServiceProto "Service.proto" GRPC)
   generate_protos(RemoteIndexProto "Index.proto")
-  # Ensure dependency headers are generated before dependent protos are built.
-  # FIXME: this should be encapsulated in generate_protos.
-  # FIXME: CMake docs say OBJECT_DEPENDS isn't needed, but I can't get the
-  #        recommended add_dependencies() approach to work.
-  set_source_files_properties(
-    ${CMAKE_CURRENT_BINARY_DIR}/Service.pb.cc
-    PROPERTIES OBJECT_DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/Index.pb.h)
+  generate_protos(RemoteIndexServiceProto "Service.proto"
+    DEPENDS "Index.proto"
+    GRPC)
   include_directories(${CMAKE_CURRENT_BINARY_DIR})
   include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../)
 
   # FIXME(kirillbobyrev): target_compile_definitions is not working with
   # add_clang_library for some reason. Is there any way to make this
   # target-local?
   add_definitions(-DGOOGLE_PROTOBUF_NO_RTTI=1)
 

llvm/cmake/modules/FindGRPC.cmake

if (HOMEBREW)
       endif()
     endif()
   endif()
 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.
 function(generate_protos LibraryName ProtoFile)
-  cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "")
+  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}"
add_custom_command(
         OUTPUT ${GeneratedProtoSource} ${GeneratedProtoHeader}
         COMMAND ${PROTOC}
         ARGS ${Flags} "${ProtoSourceAbsolutePath}"
         DEPENDS "${ProtoSourceAbsolutePath}")
 
   add_clang_library(${LibraryName} ${GeneratedProtoSource}
     PARTIAL_SOURCES_INTENDED
     LINK_LIBS grpc++ protobuf)
+
+  # Ensure dependency headers are generated before dependent protos are built.
+  # DEPENDS arg is a list of "Foo.proto".
+  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
+    get_filename_component(ImportedHeader "${ImportedHeader}"
+      ABSOLUTE
+      BASE_DIR "${CMAKE_CURRENT_BINARY_DIR}")
+    # Compilation of each generated source depends on ${BINARY}/Foo.pb.h.
+    foreach(Generated IN LISTS GeneratedProtoSource)
+      # FIXME: CMake docs suggest OBJECT_DEPENDS isn't needed, but I can't get
+      #        the recommended add_dependencies() approach to work.
+      set_source_files_properties("${Generated}"
+        PROPERTIES OBJECT_DEPENDS "${ImportedHeader}")
+    endforeach(Generated)
+  endforeach(ImportedProto)
 endfunction()