This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Support inter-proto dependencies in generate_protos.
ClosedPublic

Authored by sammccall on Oct 27 2020, 2:58 AM.

Diff Detail

Event Timeline

sammccall created this revision.Oct 27 2020, 2:58 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 27 2020, 2:58 AM
sammccall requested review of this revision.Oct 27 2020, 2:58 AM
kadircet added inline comments.Oct 27 2020, 3:04 AM
llvm/cmake/modules/FindGRPC.cmake
117

nit: I would move the fixme near to the set_source_files_properties

sammccall updated this revision to Diff 300930.Oct 27 2020, 3:05 AM
sammccall marked an inline comment as done.

Move FIXME to more specific place

kbobyrev added inline comments.Oct 27 2020, 5:28 AM
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 added inline comments.Oct 27 2020, 7:03 AM
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.

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.

kbobyrev accepted this revision.Oct 29 2020, 12:23 AM
This revision is now accepted and ready to land.Oct 29 2020, 12:23 AM
sammccall added inline comments.Oct 29 2020, 2:03 AM
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.

This revision was landed with ongoing or failed builds.Oct 29 2020, 2:04 AM
This revision was automatically updated to reflect the committed changes.