RockteMQ-AI commented on PR #1297:
URL:
https://github.com/apache/rocketmq-clients/pull/1297#issuecomment-4925123789
- **[Good] SOVERSION / VERSION on shared library** — `VERSION 5.1.0` +
`SOVERSION 5` enables proper soname versioning (`librocketmq.so.5 →
librocketmq.so.5.1.0`), which is important for downstream package management.
- **[Good] Uninstall target** — Standard `cmake_uninstall.cmake.in` pattern,
correctly reverses install order via `list(REVERSE)`.
- **[Suggestion] `rocketmq-config.cmake.in` missing transitive deps** — The
config template only has `find_dependency(absl)` and `find_dependency(gRPC)`,
but the public interface also links `absl::base` and `gRPC::grpc++`. If
protobuf or OpenSSL headers are exposed in the public API (e.g., via
`Tracing.h` or gRPC types in headers), consider adding:
```cmake
find_dependency(protobuf CONFIG)
find_dependency(OpenSSL)
```
Without these, downstream `find_package(rocketmq)` may fail if the
consumer doesn't already have them loaded. - **[Info] `BUILD_TESTS` default
changed from `ON` → `OFF`** — This means developers building locally without
`-DBUILD_TESTS=ON` will skip tests. CI explicitly enables it, so coverage is
maintained. Just noting this as a developer-experience change — existing build
scripts that relied on tests being built by default will need updating. -
**[Info] Static library excluded from install** — The comment explains the
Windows `.lib` collision rationale. Acceptable, but worth noting that consumers
who need static linking will have to build from source. - **[Info] Hardcoded
`VERSION 5.1.0`** — Consider reading the version from a file or the parent
project to keep it in sync with releases. Not a blocker for this PR. ###
Verdict Clean, well-structured CMake modernization. The install/uninstall
support and CI test enablement are valuable additions. The `$<BUILD_INTERF
ACE>` scoping for opencensus is a nice touch. The only actionable suggestion
is reviewing whether `rocketmq-config.cmake.in` needs additional
`find_dependency()` entries for transitive public deps. REVIEW_EOF echo "Review
written to /tmp/review_1297.md"
--
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]
