Copilot commented on code in PR #3475:
URL: https://github.com/apache/brpc/pull/3475#discussion_r3835737153
##########
CMakeLists.txt:
##########
@@ -351,7 +352,7 @@ set(DYNAMIC_LIB
${CMAKE_THREAD_LIBS_INIT}
${THRIFT_LIB}
dl
- z)
+ ZLIB::ZLIB)
Review Comment:
Switching the link dependency from the bare library name `z` to `ZLIB::ZLIB`
can make the actual zlib link flag/library name platform- or toolchain-specific
(e.g. static variants). The generated pkg-config metadata still hard-codes
`-lz` via BRPC_PRIVATE_LIBS, which may no longer match what CMake uses and can
break consumers that rely on `brpc.pc` for static linking.
##########
src/CMakeLists.txt:
##########
@@ -21,6 +21,10 @@ add_dependencies(SOURCES_LIB PROTO_LIB)
target_link_libraries(BUTIL_LIB PRIVATE brpc_common_config)
target_link_libraries(SOURCES_LIB PRIVATE brpc_common_config)
+# protobuf/io/gzip_stream.h includes <zlib.h>. Prioritize the discovered
+# zlib headers over include directories inherited from parent projects.
+target_include_directories(SOURCES_LIB BEFORE PRIVATE ${ZLIB_INCLUDE_DIRS})
Review Comment:
Using ${ZLIB_INCLUDE_DIRS} duplicates FindZLIB state and can diverge from
the imported target you now link (ZLIB::ZLIB). Prefer sourcing the include dirs
from ZLIB::ZLIB so there’s a single authoritative discovery result (and it
stays correct if ZLIB_INCLUDE_DIRS is not populated as expected).
--
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]
