zchuango commented on PR #3475:
URL: https://github.com/apache/brpc/pull/3475#issuecomment-5412504859
> @zchuango Adding find_package(ZLIB REQUIRED) and linking against
ZLIB::ZLIB is reasonable because zlib is a direct dependency of bRPC.
>
> My concern is specifically about changing SOURCES_LIB include ordering to
work around include directories leaked from the parent project. bRPC cannot
guarantee that its dependency headers will never conflict with unrelated
headers exposed by a parent project, and prioritizing the zlib include
directory only addresses this particular collision rather than the underlying
dependency-scope issue.
>
> The parent project should scope each module dependency with target-level
CMake commands, such as target_include_directories(), instead of exposing
unrelated include directories through directory-level settings. This prevents
unnecessary dependencies from leaking into bRPC compilation scope. Therefore, I
do not think bRPC should work around the parent project dependency leakage by
changing its internal include order.
I agree that bRPC should not adjust its internal include ordering to
compensate for include directories leaked by a parent project.
I have removed the target_include_directories(SOURCES_LIB BEFORE ...)
workaround. The PR now only discovers zlib explicitly with find_package(ZLIB
REQUIRED) and links it through ZLIB::ZLIB, since zlib is a direct dependency of
bRPC.
I will also update the PR description to clarify that parent-project include
leakage is not addressed by this change.
--
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]
