You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "songgaoye (via GitHub)" <gi...@apache.org> on 2026/10/15 05:57:23 UTC
[PR] Fix CMake protobuf include order (brpc)
songgaoye opened a new pull request, #3612:
URL: https://github.com/apache/brpc/pull/3612
### What problem does this PR solve?
Issue Number: resolve
Problem Summary:
On macOS with Homebrew, CMake may find a versioned protobuf installation while
a generic dependency include path, such as `/opt/homebrew/include`, appears
before the protobuf include directory.
If that prefix contains headers from another protobuf installation, generated
sources are compiled against headers that do not match the selected `protoc`
and protobuf library. This causes errors such as:
This file was generated by a newer version of protoc ...
### What is changed and the side effects?
Changed:
- Put `${PROTOBUF_INCLUDE_DIRS}` ahead of generic dependency include paths in
`brpc_common_config`.
- Keep generated protobuf headers first, followed by the selected protobuf
installation and then other project/dependency headers.
Side effects:
* Performance effects:
None. This only changes CMake header search order.
* Breaking backward compatibility:
None.
* * *
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix CMake protobuf include order (brpc)
Posted by "darion-yaphet (via GitHub)" <gi...@apache.org>.
darion-yaphet commented on PR #3612:
URL: https://github.com/apache/brpc/pull/3612#issuecomment-6186482606
Thanks, moving protobuf ahead of the generic dependency prefixes looks right. Two small things before merging:
1. `src/` now comes after protobuf. If protobuf sits in a shared prefix that also has an installed brpc, the installed `butil/`/`brpc/` headers will shadow the in-tree ones.
2. The test targets aren't covered yet. `test/CMakeLists.txt:29` adds `GPERFTOOLS_INCLUDE_DIR` directly to `brpc_test_config`, so `/opt/homebrew/include` is still searched before protobuf when building unit tests. macOS CI doesn't build the tests, so it won't catch this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix CMake protobuf include order (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #3612:
URL: https://github.com/apache/brpc/pull/3612#discussion_r4349006682
##########
test/CMakeLists.txt:
##########
@@ -253,6 +254,17 @@ target_link_libraries(test_butil PRIVATE
add_test(NAME test_butil COMMAND test_butil)
+string(REPLACE ";" "|" protobuf_include_dirs_for_test
+ "${Protobuf_INCLUDE_DIRS}")
+string(REPLACE ";" "|" gflags_include_dirs_for_test
+ "${GFLAGS_INCLUDE_PATH}")
+add_test(NAME cmake_protobuf_include_order
Review Comment:
[replied by brpc-oncall robot] This regression check will never run in CI: no workflow configures with `BUILD_UNIT_TESTS=ON`, and the test jobs execute the test binaries directly through `test/run_tests.sh` (globs like `test_butil`/`brpc*unittest`); `ctest` is not invoked anywhere. As a result the `check_protobuf_include_order.cmake` script and the `file(GENERATE ...butil_build_config_include_dirs.txt)` dump added in src/butil/CMakeLists.txt provide no protection. Prefer a configure-time verification (e.g. validate the ordering right after `butil_build_config` is defined and fail with `message(FATAL_ERROR)`), or hook the check into a target CI already builds such as `cmake_link_checks`. Note also that the check reads only `butil_build_config`'s own `INTERFACE_INCLUDE_DIRECTORIES`, so it would not catch the gaps mentioned in the src/butil/CMakeLists.txt comment.
##########
src/butil/CMakeLists.txt:
##########
@@ -85,9 +87,18 @@ add_library(butil_build_config INTERFACE)
target_link_libraries(butil_build_config INTERFACE brpc_common_config)
target_compile_definitions(butil_build_config INTERFACE ${BUTIL_DEFINITIONS})
target_include_directories(butil_build_config BEFORE INTERFACE
- ${butil_generated_include_dir})
+ ${butil_generated_include_dir}
+ ${Protobuf_INCLUDE_DIRS})
Review Comment:
[replied by brpc-oncall robot] The selected Protobuf directory is only prepended for `butil_build_config`, but a target's own include directories are ordered before the ones inherited from the libraries it links, so the fix does not cover the rest of the tree:
- `brpc_build_config` (src/brpc/CMakeLists.txt) adds `BRPC_INCLUDE_DIRS`, which contains `${ZLIB_INCLUDE_DIRS}`, the Thrift path and the RDMA/URMA paths, ahead of anything inherited from `butil_build_config`. On Homebrew `${ZLIB_INCLUDE_DIRS}` resolves to `/opt/homebrew/include`, i.e. exactly the generic prefix that can carry a different `google/protobuf`, so brpc's own sources can still be compiled against headers that do not match the selected `protoc`.
- `brpc_test_config` (test/CMakeLists.txt) still adds `${GPERFTOOLS_INCLUDE_DIR}` directly, which is likewise searched before the Protobuf directory inherited from `butil_build_config` for test compilations.
Please apply the same ordering to these configs (e.g. prepend `${Protobuf_INCLUDE_DIRS}` in `brpc_build_config`/`brpc_test_config` BEFORE lists, or ensure the generic prefixes are appended after it), otherwise the reported failure can still occur.
##########
test/CMakeLists.txt:
##########
@@ -128,6 +128,7 @@ SET(TEST_BUTIL_SOURCES
${CMAKE_CURRENT_SOURCE_DIR}/non_thread_safe_unittest.cc
${CMAKE_CURRENT_SOURCE_DIR}/nullable_string16_unittest.cc
${CMAKE_CURRENT_SOURCE_DIR}/object_pool_unittest.cpp
+ ${CMAKE_CURRENT_SOURCE_DIR}/resource_pool_unittest.cpp
Review Comment:
[replied by brpc-oncall robot] This is unrelated to the Protobuf include-order fix, and `resource_pool_unittest.cpp` is not registered in the Makefile or Bazel test lists either, so it is not built anywhere. Please drop it from this PR (or submit it separately) to keep the change focused.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix CMake protobuf include order (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3612:
URL: https://github.com/apache/brpc/pull/3612#discussion_r4305391674
##########
test/CMakeLists.txt:
##########
@@ -19,7 +19,11 @@ find_package(Gperftools)
add_library(brpc_test_config INTERFACE)
target_link_libraries(brpc_test_config INTERFACE brpc_common_config)
-target_include_directories(brpc_test_config INTERFACE ${CMAKE_CURRENT_BINARY_DIR})
+target_include_directories(brpc_test_config BEFORE INTERFACE
+ ${CMAKE_CURRENT_BINARY_DIR}
+ ${PROJECT_SOURCE_DIR}/src
+ ${PROTOBUF_INCLUDE_DIRS}
Review Comment:
Adding `${PROJECT_SOURCE_DIR}/src` directly to `brpc_test_config` puts it before the transitive include directories from `brpc_common_config`, including the generated `${PROJECT_BINARY_DIR}` directory. That reverses the generated-header protection for test compilations: if an in-source `src/brpc/*.pb.h` is left over, tests will select it before the freshly generated header. Keep only the test binary directory here and let `brpc_common_config` provide the generated/source/protobuf order, or explicitly place `${PROJECT_BINARY_DIR}` before this source directory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix CMake protobuf include order (brpc)
Posted by "wasphin (via GitHub)" <gi...@apache.org>.
wasphin commented on PR #3612:
URL: https://github.com/apache/brpc/pull/3612#issuecomment-6295100344
Prepending the selected Protobuf include directory is a reasonable, scoped fix. It assumes that the directory belongs to a consistent dependency installation. Include ordering alone cannot guarantee correctness when shared prefixes contain conflicting versions of multiple libraries; that requires control over the build environment and dependency graph.
Keeping generated and in-tree brpc headers ahead of installed copies remains worthwhile, but I would treat that as a separate, limited concern rather than require this PR to solve general multi-version coexistence.
Since #3618 has merged, please rebase and adapt the change to the module-owned targets. A regression check with conflicting Protobuf headers in a generic prefix would help verify the intended behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix CMake protobuf include order (brpc)
Posted by "wasphin (via GitHub) via dev" <de...@brpc.apache.org>.
wasphin merged PR #3612:
URL: https://github.com/apache/brpc/pull/3612
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix CMake protobuf include order (brpc)
Posted by "wasphin (via GitHub) via dev" <de...@brpc.apache.org>.
wasphin commented on PR #3612:
URL: https://github.com/apache/brpc/pull/3612#issuecomment-6541789840
Could we drop the dedicated regression test here? This is a straightforward include-order adjustment, and checking the generated compiler flags alongside the existing build CI seems sufficient. The fake-header fixture adds maintenance overhead and can fail when the selected Protobuf headers live in a default system directory such as `/usr/include`, since ordinary `-I` paths are searched first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix CMake protobuf include order (brpc)
Posted by "darion-yaphet (via GitHub)" <gi...@apache.org>.
darion-yaphet commented on PR #3612:
URL: https://github.com/apache/brpc/pull/3612#issuecomment-6437138387
Thanks for the update.
The two points from my earlier comment look addressed for this PR's scope:
1. `brpOn the `src/` vs Protobuf shared-prefix case, I agree with @wasphin that it is a separate, limited concern and does not need to block this change.
One CI nits before merge: License Check fails because `test/protobuf_include_order/generic_include/google/protobuf/stubs/common.h` has no valid license header. The poison `#define` can stay; please add the usual Apache header above it.
With that, LGTM from my side on the CMake ordering fix and the regression test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix CMake protobuf include order (brpc)
Posted by "songgaoye (via GitHub)" <gi...@apache.org>.
songgaoye commented on PR #3612:
URL: https://github.com/apache/brpc/pull/3612#issuecomment-6212403875
Thank you! @darion-yaphet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix CMake protobuf include order (brpc)
Posted by "wasphin (via GitHub)" <gi...@apache.org>.
wasphin commented on PR #3612:
URL: https://github.com/apache/brpc/pull/3612#issuecomment-6363388174
Thanks for the updates. The include-order fix looks reasonable, and the latest revision covers the module and test configuration targets.
One suggestion: `brpc_assert_protobuf_include_order()` feels a bit heavyweight for this change. The include ordering is already explicitly established by `target_include_directories(... BEFORE ...)`, and the additional function mostly validates the same configuration we just constructed.
More importantly, checking `INTERFACE_INCLUDE_DIRECTORIES` does not necessarily reflect the final compiler include search order, which may also be affected by transitive dependencies, system include directories, and other target properties.
I'd prefer keeping the explicit ordering fix and using a focused regression test with conflicting Protobuf installations to verify the actual compiler behavior, rather than introducing a custom assertion function into the regular CMake configuration.
This would keep the implementation simpler while providing stronger regression coverage for the original issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org