Refactor CMake source and target ownership into module directories by wasphin · Pull Request #3618 · apache/brpc

wasphin · 2026-10-17T07:17:29Z

What problem does this PR solve?

Issue Number: N/A

Problem Summary:

The root CMake project owns source lists, production object targets and
module-specific configuration for all modules. This makes module ownership
hard to follow and expands the review surface of later packaging changes.

This extracts the two prerequisite refactoring commits from #3596. Component
libraries, package exports and the subsequent dependency-resolver rewrite are
not included.

What is changed and the side effects?

Changed:

Please prefer Rebase and merge to preserve the two reviewable steps:

Move production source selection and protocol generation into module
directories, retaining the existing bundle and test source inputs.
Move production object targets, feature/dependency handling and header
installation into the owning modules. Reuse the module objects in the
existing bundles and the source sets in debug unit-test targets.

Generate butil/config.h in the build tree, keep common options in the root,
and scope CRC32C instruction options to the affected source files, including
debug builds. Preserve the existing RISC-V vector architecture options.

Side effects:

Performance effects: No intended runtime change. Production sources are
compiled once and reused for the existing bundle outputs.
Breaking backward compatibility: Existing bundle names are unchanged.
CMake no longer stages headers in output/include. Configure with
-DCMAKE_INSTALL_PREFIX="$PWD/build/output", build, then run
cmake --install build so installed files and pkg-config metadata use the
same prefix. Make and Bazel header generation remain unchanged.

Check List:

Please make sure your changes are compilable.
When providing us with a new feature, it is best to add related tests.
Please follow Contributor Covenant Code of Conduct.

Copilot

🟡 Changes recommended

Unresolved CMake configuration blockers affect clean builds and example dependency discovery.

3 open issues

What changed in this PR

Refactors CMake ownership into module directories while preserving existing bundles and tests.

Changes:

Moves sources, targets, dependencies, generated headers, and installation logic into modules.
Reuses module objects for bundles and debug tests.
Updates tools and examples for build-tree generation and installation-based headers.

File	Summary
`tools/rpc_replay/CMakeLists.txt`	Uses local source discovery.
`tools/rpc_press/CMakeLists.txt`	Uses local source discovery.
`test/CMakeLists.txt`	Reuses module sources for tests.
`src/mcpack2pb/CMakeLists.txt`	Defines mcpack sources and generator; has a critical target-order configuration issue.
`src/json2pb/CMakeLists.txt`	Defines json2pb sources and target.
`src/CMakeLists.txt`	Builds bundles from module objects.
`src/bvar/CMakeLists.txt`	Owns bvar sources and target.
`src/butil/config.h.in`	Provides the generated configuration header template.
`src/butil/CMakeLists.txt`	Owns butil sources and dependencies; has a protobuf include-variable issue.
`src/bthread/CMakeLists.txt`	Owns bthread configuration, sources, and dependencies.
`src/brpc/CMakeLists.txt`	Owns RPC sources, protocols, and dependencies.
`example/ubring_performance/CMakeLists.txt`	Uses local paths; retains stale `output/include` discovery.
`example/partition_echo_c++/CMakeLists.txt`	Uses local resource paths.
`example/multi_threaded_echo_c++/CMakeLists.txt`	Uses local certificate paths.
`example/http_c++/CMakeLists.txt`	Uses local certificate paths.
`example/dynamic_partition_echo_c++/CMakeLists.txt`	Uses local resource paths.
`example/cmake/BrpcExample.cmake`	Uses local example paths; retains stale `output/include` discovery.
`CMakeLists.txt`	Simplifies root configuration and delegates module setup.
`cmake/SetupGtest.cmake`	Uses directory-local script paths.
`cmake/CompileProto.cmake`	Generates protobuf outputs in build directories.
`cmake/bRPCHeaders.cmake`	Provides module-owned header installation helpers.

_{Review effort: Lite}

Keep source selection and bundled artifacts unchanged while moving each module source list and RPC protocol generation out of the root project. Retain the existing test source inputs during this first migration step.

Let each module own its production object library and move the mcpack protoc generator into its module. Aggregate the module objects into the existing brpc bundle without compiling production sources twice. Move butil configuration-header generation, feature-option handling and dependency discovery into the owning modules. Keep RPC-only headers out of lower-library compile settings and assemble legacy bundle dependencies after module setup. Apply CRC32C instruction flags only to the relevant sources, including debug builds. Preserve the RISC-V vector base64 options and the existing AArch64 libcontext workaround. Query module targets for the existing unit-test source sets instead of passing source lists through parent directory scopes. Preserve the test compile definitions and bundled debug library linkage. Install source and generated headers directly from their owning modules. Keep build-tree include paths separate from installed target interfaces instead of copying headers into an output staging tree. Use Protobuf's imported targets to preserve static transitive dependencies and Debug/Release library selection, and handle config-mode import paths.

Stop searching for the removed output/include staging directory and honor the caller-provided CMAKE_PREFIX_PATH in both example entry points. Document the install-before-use workflow in English and Chinese. Link Protobuf through its imported target so non-system installations supply the headers required by generated sources and public RPC headers.

Copilot

🟡 Changes recommended

Resolve the critical CMake 2.8.10 and protobuf::libprotobuf compatibility issue.

1 open issue

Use compatible Protobuf linking for CMake 2.8.10 · New

3 resolved since last review

_{Review effort: Lite}

Require CMake 3.16 and use the same policy range as the root project and other examples. This makes the declared minimum consistent with use of the protobuf::libprotobuf imported target introduced in CMake 3.9.

Copilot

🔵 Needs a closer look

Fix the documented installation workflow’s pkg-config prefix mismatch.

No open issues 🎉

1 resolved since last review

Use compatible Protobuf linking for CMake 2.8.10

_{Review effort: Lite}

Set CMAKE_INSTALL_PREFIX during configuration in both getting-started guides, then build and install without an install-time prefix override. This keeps the generated brpc.pc paths aligned with the installed files.

wwbmmm

[replied by brpc-oncall robot] The module split is structurally sound (source/proto lists match the pre-refactor ones and the install rules reproduce the old staged header set, with the stale in-source butil/config.h correctly excluded), but one include-order regression for the mcpack generator and some refactor leftovers should be cleaned up, and docs/{cn,en}/ubring.md still build example/ubring_performance straight from the in-tree build, which no longer finds brpc after the output/include staging removal.

Inherit build settings through the selected brpc bundle instead of linking butil_build_config first. This keeps the generated IDL header directory ahead of the source tree and prevents stale Make-generated headers from shadowing the current CMake output.

Drop the unread platform-library property and unused URMA parent-scope exports. Remove redundant debug-bundle glog blocks, since module-owned bundle dependencies already propagate the library through DYNAMIC_LIB.

Set the brpc installation prefix at configure time, install the bundle, and pass the prefix explicitly when building the performance example. Update both build sections in the English and Chinese UBRing guides.

Copilot

🔵 Needs a closer look

The broad CMake refactor spans many build paths, and a moderate static-link ordering issue remains unresolved.

No open issues 🎉

_{Review effort: Lite}

chenBright · 2026-10-18T14:12:30Z

-   target_link_libraries(brpc-static PRIVATE ${THRIFT_LIB})
-   check_thrift_version(brpc-static)
-endif()
+target_link_libraries(brpc-static PUBLIC brpc_build_config)


With WITH_THRIFT=ON, the previous implementation explicitly linked ${THRIFT_LIB} to brpc-static. The refactor moves this dependency into brpc_runtime_dependencies, but neither that interface nor brpc_bundle_dependencies is linked to the static target.

Downstream projects that supply the other dependencies and rely on the existing transitive Thrift linkage will now encounter undefined Thrift symbols. In-tree tools can hide this regression because they also link ${DYNAMIC_LIB}.

chenBright · 2026-10-18T14:13:29Z

+    list(APPEND butil_libraries ${GLOG_LIB})
+    string(APPEND butil_private_libs " -lglog")


Appending glog changes the generated brpc.pc ordering from -lglog -lgflags … to -lgflags … -lglog.

With static glog/gflags and GNU ld, a client that only includes butil/logging.h and calls LOG(INFO) may first introduce gflags references when libglog.a is scanned. Since libgflags.a has already been processed, this leaves google::FlagRegisterer unresolved.

Propagate the complete link dependency closure from the static bundle and reuse it in tools rather than masking missing dependencies at each caller. Order module consumers before providers in both CMake and pkg-config, and keep system libraries after their users. Retain LevelDB Snappy dependencies independently of WITH_SNAPPY without exposing unused Snappy headers. Include selected optional runtime libraries in the generated private link metadata. Add bundle-only consumers and dependency-metadata checks to the existing CMake CI jobs without increasing job concurrency.

wasphin requested a lite review from Copilot October 17, 2026 07:19

Copilot started reviewing on behalf of wasphin October 17, 2026 07:19

View session

Copilot AI reviewed Oct 17, 2026

View reviewed changes

Comment thread src/mcpack2pb/CMakeLists.txt Outdated

Comment thread example/cmake/BrpcExample.cmake Outdated

Comment thread example/ubring_performance/CMakeLists.txt Outdated

wasphin added 3 commits October 17, 2026 19:31

Move CMake source definitions into module directories

1e5445b

Keep source selection and bundled artifacts unchanged while moving each module source list and RPC protocol generation out of the root project. Retain the existing test source inputs during this first migration step.

wasphin force-pushed the refactor-cmake-module-targets branch from c3e70e8 to 22edf80 Compare October 17, 2026 11:32

wwbmmm requested a lite review from Copilot October 17, 2026 12:03

Copilot started reviewing on behalf of wwbmmm October 17, 2026 12:04

View session

Copilot AI reviewed Oct 17, 2026

View reviewed changes

Comment thread example/ubring_performance/CMakeLists.txt

Align the ubring example CMake minimum with the project

9e13e32

Require CMake 3.16 and use the same policy range as the root project and other examples. This makes the declared minimum consistent with use of the protobuf::libprotobuf imported target introduced in CMake 3.9.

wwbmmm requested a lite review from Copilot October 17, 2026 13:26

Copilot started reviewing on behalf of wwbmmm October 17, 2026 13:27

View session

Copilot AI reviewed Oct 17, 2026

View reviewed changes

Configure the install prefix before generating pkg-config metadata

6a91cde

Set CMAKE_INSTALL_PREFIX during configuration in both getting-started guides, then build and install without an install-time prefix override. This keeps the generated brpc.pc paths aligned with the installed files.

wwbmmm reviewed Oct 18, 2026

View reviewed changes

Comment thread src/mcpack2pb/CMakeLists.txt Outdated

Comment thread src/butil/CMakeLists.txt Outdated

Comment thread test/CMakeLists.txt Outdated

wasphin added 3 commits October 18, 2026 12:23

Remove unused CMake module configuration leftovers

1672bda

Drop the unread platform-library property and unused URMA parent-scope exports. Remove redundant debug-bundle glog blocks, since module-owned bundle dependencies already propagate the library through DYNAMIC_LIB.

Document installed-prefix builds for UBRing examples

7695dea

Set the brpc installation prefix at configure time, install the bundle, and pass the prefix explicitly when building the performance example. Update both build sections in the English and Chinese UBRing guides.

wasphin requested a lite review from Copilot October 18, 2026 10:28

Copilot started reviewing on behalf of wasphin October 18, 2026 10:29

View session

Copilot AI reviewed Oct 18, 2026

View reviewed changes

chenBright reviewed Oct 18, 2026

View reviewed changes

Navigation Menu

Repository navigation

Refactor CMake source and target ownership into module directories - #3618

Refactor CMake source and target ownership into module directories#3618
wasphin wants to merge 9 commits into
apache:masterfrom
wasphin:refactor-cmake-module-targets

wasphin commented Oct 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

wwbmmm left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

chenBright Oct 18, 2026

Uh oh!

chenBright Oct 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		list(APPEND butil_libraries ${GLOG_LIB})
		string(APPEND butil_private_libs " -lglog")

Conversation

wasphin commented Oct 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and the side effects?

Check List:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

wwbmmm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

chenBright Oct 18, 2026

Choose a reason for hiding this comment

Uh oh!

chenBright Oct 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wasphin commented Oct 17, 2026 •

edited

Loading