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: [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]
