[PATCH] D90215: [CMake] Support inter-proto dependencies in generate_protos.

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Oct 29 02:03:53 PDT 2020


sammccall added inline comments.


================
Comment at: llvm/cmake/modules/FindGRPC.cmake:118
+  # DEPENDS arg is a list of "Foo.proto".
+  foreach(ImportedProto IN LISTS PROTO_DEPENDS)
+    # Foo.proto -> Foo.pb.h
----------------
kbobyrev wrote:
> sammccall wrote:
> > kbobyrev wrote:
> > > 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?
> > 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)
> Ahh, interesting. That's not immediately obvious :D Thanks for the explanation!
Agree it's not obvious - I could paste the manual for cmake_parse_arguments here but it doesn't seem appropriate :-)


================
Comment at: llvm/cmake/modules/FindGRPC.cmake:124
+      ABSOLUTE
+      BASE_DIR "${CMAKE_CURRENT_BINARY_DIR}")
+    # Compilation of each generated source depends on ${BINARY}/Foo.pb.h.
----------------
kbobyrev wrote:
> sammccall wrote:
> > kbobyrev wrote:
> > > 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?
> > > 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.
> Ah, you're right, my bad.
Added some extra comments to spell this out.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90215/new/

https://reviews.llvm.org/D90215



More information about the cfe-commits mailing list