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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
