kbobyrev added a comment.
LGTM, thanks!
================
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
----------------
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!
================
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.
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90215/new/
https://reviews.llvm.org/D90215
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
