[songgaoye]

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

CMake may select one Protobuf installation while a generic include prefix
(for example /usr/local/include) is searched first. If that prefix contains
headers from a different Protobuf version, brpc can compile against headers
that do not match the selected Protobuf libraries.

This PR makes the selected Protobuf include directories take precedence for
the module-owned build targets.

What is changed and the side effects?

Changed:

• Add Protobuf_INCLUDE_DIRS to butil_build_config before generic dependency
  include directories.
• Preserve the generated in-tree brpc headers as the highest-priority include
  directory.
• Add a CMake regression test that verifies selected Protobuf include
  directories precede generic include directories.

Side effects:

• Builds that expose conflicting Protobuf headers through generic prefixes now
  consistently use the Protobuf installation selected by find_package.
• This does not make arbitrary mixed-version dependency prefixes safe; those
  still need to be managed by the build environment.

Breaking backward compatibility:

None.

---

[Copilot]

🟢 Approval recommended

The focused include-order change addresses the reported header mismatch without an identified issue.

0 open findings

What changed in this PR

This PR adjusts CMake header search order so builds use headers from the selected protobuf installation.

Changes:

• Places generated headers first, followed by the selected protobuf headers and then general dependency headers.

File	Description
CMakeLists.txt	Updates brpc_common_config include order.

🧠 Review effort: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[darion-yaphet]

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.

[songgaoye]

Thank you! @darion-yaphet

[Copilot]

🟡 Changes recommended

Correct the test include ordering to ensure freshly generated headers take precedence.

1 open issue

• Test include order can select stale in-source generated headers · New

_{Review effort: Lite}

[Copilot]

🟢 Approval recommended

No unresolved issues were identified, and all assessments support approval.

No open issues 🎉

1 resolved since last review

• Test include order can select stale in-source generated headers

_{Review effort: Lite}

[wasphin]

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.

[wwbmmm]

[replied by brpc-oncall robot] The butil-level ordering change is reasonable, but it only protects butil's own config so the brpc module and test targets can still pick up Protobuf headers from a generic prefix, the added regression check never runs in CI, and one unrelated test source was added.

[wasphin]

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.

[darion-yaphet]

Thanks for the update.

The two points from my earlier comment look addressed for this PR's scope.

On 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 nit 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.

[wasphin]

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.

[wasphin]

LGTM