Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Build] Removed gRPC_PROTOBUF_PACKAGE_TYPE, supporting config only #32988

Merged
merged 3 commits into from May 4, 2023

Conversation

veblush
Copy link
Contributor

@veblush veblush commented May 2, 2023

FindProtobuf isn't working as Protobuf began to use Abseil so gRPC is now using CONFIG mode for protobuf module
(Context: https://gitlab.kitware.com/cmake/cmake/-/issues/24321)

@veblush veblush added lang/core area/build release notes: yes Indicates if PR needs to be in release notes labels May 2, 2023
@veblush veblush requested a review from yashykt May 2, 2023 20:44
@veblush veblush changed the title Removed gRPC_PROTOBUF_PACKAGE_TYPE, supporting config only [Build] Removed gRPC_PROTOBUF_PACKAGE_TYPE, supporting config only May 2, 2023
@veblush
Copy link
Contributor Author

veblush commented May 3, 2023

CC: @coryan

@@ -90,6 +86,6 @@ elseif(gRPC_PROTOBUF_PROVIDER STREQUAL "package")
set(_gRPC_PROTOBUF_PROTOC_EXECUTABLE ${PROTOBUF_PROTOC_EXECUTABLE})
endif()
endif()
set(_gRPC_FIND_PROTOBUF "if(NOT Protobuf_FOUND AND NOT PROTOBUF_FOUND)\n find_package(Protobuf ${gRPC_PROTOBUF_PACKAGE_TYPE})\nendif()")
set(_gRPC_FIND_PROTOBUF "if(NOT Protobuf_FOUND AND NOT PROTOBUF_FOUND)\n find_package(Protobuf CONFIG)\nendif()")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but this is going into gRPCConfig.cmake, you should use find_dependency() and not find_package() in those:

https://cmake.org/cmake/help/latest/module/CMakeFindDependencyMacro.html
https://stackoverflow.com/questions/50263280/difference-between-find-dependency-and-find-package-in-cmake

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's a good point. Thanks! Updated this and will update others too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to include(CMakeFindDependencyMacro) to use find_dependency. #33361

@veblush veblush enabled auto-merge (squash) May 4, 2023 16:01
@veblush veblush disabled auto-merge May 4, 2023 20:55
@veblush veblush merged commit 303e568 into grpc:master May 4, 2023
61 of 63 checks passed
veblush added a commit to veblush/grpc that referenced this pull request May 4, 2023
…rpc#32988)

`FindProtobuf` isn't working as Protobuf began to use Abseil so gRPC is
now using `CONFIG` mode for protobuf module
(Context: https://gitlab.kitware.com/cmake/cmake/-/issues/24321)
veblush added a commit that referenced this pull request May 4, 2023
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label May 5, 2023
nanahpang pushed a commit to nanahpang/grpc that referenced this pull request May 8, 2023
…rpc#32988)

`FindProtobuf` isn't working as Protobuf began to use Abseil so gRPC is
now using `CONFIG` mode for protobuf module
(Context: https://gitlab.kitware.com/cmake/cmake/-/issues/24321)
wanlin31 pushed a commit that referenced this pull request May 18, 2023
…32988)

`FindProtobuf` isn't working as Protobuf began to use Abseil so gRPC is
now using `CONFIG` mode for protobuf module
(Context: https://gitlab.kitware.com/cmake/cmake/-/issues/24321)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build bloat/none imported Specifies if the PR has been imported to the internal repository lang/c++ lang/core per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants