This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Fix `FindGRPC.cmake` for setting up gRPC related libraries for macOS+homebrew context
ClosedPublic

Authored by akyrtzi on Jun 15 2022, 12:12 PM.

Details

Summary
  • Switch $GRPC_OPTS references to ${GRPC_OPTS}
  • Also find and include the search path for abseil headers
  • Only setup the gRPC related targets once, so that include(FindGRPC) can be called from multiple tools

Diff Detail

Event Timeline

akyrtzi created this revision.Jun 15 2022, 12:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2022, 12:12 PM
Herald added a subscriber: mgorny. · View Herald Transcript
akyrtzi requested review of this revision.Jun 15 2022, 12:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2022, 12:12 PM
compnerd added inline comments.Jun 15 2022, 12:59 PM
llvm/cmake/modules/FindGRPC.cmake
71

I realize that this is something that exists prior to your change, but it seems better to actually do this as target_include_directories(grpc++ PUBLIC ${GRPC_HOMEBREW_PATH}/include) which avoids the extra includes from being put into all targets.

79

Why is the header search path for all targets being modified to add abseil? Is that a dependency for some other target that is being imported here? Perhaps we should add it to that particular library and let CMake propagate it properly?

akyrtzi added inline comments.Jun 15 2022, 1:05 PM
llvm/cmake/modules/FindGRPC.cmake
71

I can give it a try, will this make the search path propagate to all the targets that depend on grpc++?

79

The gRPC headers include abseil headers.

akyrtzi updated this revision to Diff 437315.Jun 15 2022, 1:24 PM

Use target_include_directories() to add include search paths only for the targets the depend on gRPC

akyrtzi marked 2 inline comments as done.Jun 15 2022, 1:26 PM
akyrtzi added inline comments.
llvm/cmake/modules/FindGRPC.cmake
71

Thanks for the suggestion! Using target_include_directories() seems like a much better choice.

compnerd accepted this revision.Jun 15 2022, 1:40 PM

LGTM!

This revision is now accepted and ready to land.Jun 15 2022, 1:40 PM
This revision was landed with ongoing or failed builds.Jun 15 2022, 4:11 PM
This revision was automatically updated to reflect the committed changes.