[CMake] Support inter-proto dependencies in generate_protos.

[kadircet]

llvm/cmake/modules/FindGRPC.cmake

118

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

119

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?

125

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

119

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)

125

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

119

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

125

Ah, you're right, my bad.

[sammccall]

llvm/cmake/modules/FindGRPC.cmake

119

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

125

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.
+# 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" "" "")
+  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". 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
+    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()