Baf 1716/cmake migration#23
Conversation
Replace cmlib dependency resolution with plain CMake + FetchContent, following the pattern already validated for external-server-cpp (BAF-1706) and mission-module (BAF-1715). FLEET_PROTOCOL_BUILD_MODULE_GATEWAY / FLEET_PROTOCOL_BUILD_EXTERNAL_SERVER and all gateway/server build logic are untouched; no source files changed. Validated end-to-end in bringauto/cpp-build-environment for all three flag combinations (gateway-only, external-server-only, both). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- CMakeLists.txt dropped FIND_PACKAGE(CMLIB ...) during the FetchContent migration, so cmake/FindCMLIB.cmake was never loaded and CMDEF_ADD_LIBRARY/ CMDEF_INSTALL were undefined -> configure failed immediately. - CMDEF_PACKAGE shim was a no-op and silently dropped VERSION, so cpack always produced 0.1.1 instead of the real module version.
WalkthroughThis PR replaces CMLIB/CMCONF-based dependency management (BA_PACKAGE_LIBRARY, CMCONF_INIT_SYSTEM) with a find_package/FetchContent-based system. It adds new FindBA*.cmake modules for each dependency (protobuf, nlohmann_json, zlib, Aeron, Boost, cpprestsdk, fleet-http-client, fleet-protocol-interface, fleet-protocol-cpp, async-function-execution) and a FindCMLIB.cmake compatibility shim, plus forwarding shim modules for canonical package names. ChangesDependency resolution migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (8)
cmake/FindBAFleetProtocolInterface.cmake (4)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHeader comment is stale relative to the alias list.
Only 3 of the 6 aliases created in the
foreach(Lines 62-68) are documented here (internal-client-interfaceandmodule-gateway-interfaceare missing).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/FindBAFleetProtocolInterface.cmake` around lines 3 - 4, The header comment in FindBAFleetProtocolInterface.cmake is out of sync with the aliases created in the foreach that defines the interface targets. Update the “Provides” comment to list all generated aliases, including internal-client-interface and module-gateway-interface, so it accurately matches the alias list used by the module.
34-39: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSilent no-op if either target is missing defeats the documented fix.
The comment explains this link is needed to fix a real compile-path bug in fleet-protocol's protobuf target, but if
protobuf-cpp-interfaceorprotobuf::libprotobufdoesn't exist, the block silently does nothing, and the failure resurfaces later as an opaque compile error onExternalProtocol.pb.cc.♻️ Suggested warning on the unmet condition
if(TARGET protobuf-cpp-interface AND TARGET protobuf::libprotobuf) target_link_libraries(protobuf-cpp-interface PUBLIC protobuf::libprotobuf) +elseif(TARGET protobuf-cpp-interface) + message(WARNING "[BA] protobuf::libprotobuf target not found; protobuf-cpp-interface include paths may be incomplete") endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/FindBAFleetProtocolInterface.cmake` around lines 34 - 39, The conditional in FindBAFleetProtocolInterface.cmake currently makes the protobuf include-path fix a silent no-op when either target is missing. Update the target check around target_link_libraries for protobuf-cpp-interface and protobuf::libprotobuf to emit a warning or fatal diagnostic when the expected targets are unavailable, so the missing dependency is surfaced immediately instead of failing later in ExternalProtocol.pb.cc.
75-75: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
_FOUNDis set unconditionally, even if FetchContent didn't produce the expected targets.If the fetched
fleet-protocolrepo ever renames/dropscommon-headers-interface(or the sub-target list), this still reports success, turning a clear "package not found" failure into a confusing missing-target link error downstream.♻️ Suggested validation before declaring success
-set(BAFleetProtocolInterface_FOUND TRUE) +if(NOT TARGET fleet-protocol-interface::common-headers-interface) + message(FATAL_ERROR "[BA] fleet-protocol-interface: expected target 'common-headers-interface' was not created") +endif() +set(BAFleetProtocolInterface_FOUND TRUE)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/FindBAFleetProtocolInterface.cmake` at line 75, The FindBAFleetProtocolInterface.cmake logic marks BAFleetProtocolInterface_FOUND as true unconditionally, even when FetchContent did not create the expected targets. Update the package check around the FetchContent/target validation so it only sets BAFleetProtocolInterface_FOUND after confirming common-headers-interface and any required sub-targets actually exist; otherwise leave it false and fail the find step cleanly.
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnescaped path used as a regex pattern.
MATCHEStreats${CMAKE_BINARY_DIR}as a regex; paths containing regex metacharacters (e.g.+,(,)) could produce unintended matches. Low likelihood in practice, but worth escaping for robustness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/FindBAFleetProtocolInterface.cmake` at line 13, The path check in the `fleet-protocol-interface_DIR` condition is using `MATCHES`, so `${CMAKE_BINARY_DIR}` is being treated as a regex pattern instead of a literal path. Update the comparison in `FindBAFleetProtocolInterface.cmake` to use an escaped literal form or a non-regex string comparison so `fleet-protocol-interface_DIR` is matched against `CMAKE_BINARY_DIR` safely even when the path contains regex metacharacters.cmake/FindBAFleetProtocolCpp.cmake (2)
39-39: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
_FOUNDset unconditionally without verifying the expected target exists.Same concern as the sibling module: if FetchContent succeeds but
fleet-protocol-cxx-helpers-staticisn't created, this still reports success.♻️ Suggested validation
-set(BAFleetProtocolCpp_FOUND TRUE) +if(NOT TARGET fleet-protocol-cxx-helpers-static::fleet-protocol-cxx-helpers-static) + message(FATAL_ERROR "[BA] fleet-protocol-cpp: expected target 'fleet-protocol-cxx-helpers-static' was not created") +endif() +set(BAFleetProtocolCpp_FOUND TRUE)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/FindBAFleetProtocolCpp.cmake` at line 39, The module currently marks BAFleetProtocolCpp as found unconditionally, even if the expected target was never created. Update the FindBAFleetProtocolCpp.cmake logic so the success flag is set only after validating that the fleet-protocol-cxx-helpers-static target actually exists, mirroring the check used in the sibling finder module. Keep the existing FetchContent flow, but gate the BAFleetProtocolCpp_FOUND assignment behind the target existence check.
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame unescaped regex concern as the sibling module.
MATCHES "${CMAKE_BINARY_DIR}"treats the path as a regex without escaping metacharacters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/FindBAFleetProtocolCpp.cmake` at line 12, The path check in the module-level directory guard uses MATCHES with an unescaped build path, so the CMake string is being treated as a regex. Update the condition in FindBAFleetProtocolCpp.cmake to avoid regex matching on CMAKE_BINARY_DIR, either by escaping the path before MATCHES or by switching to a non-regex comparison, and keep the same logic around the fleet-protocol-cxx-helpers-static_DIR check.cmake/FindBABoost.cmake (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
include_guard(GLOBAL).FindBACppRestSdk.cmake and FindBAFleetHttpClient.cmake both start with
include_guard(GLOBAL); this file doesn't. Adding it keeps re-inclusion behavior consistent across the new BA* modules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/FindBABoost.cmake` around lines 1 - 4, Add include_guard(GLOBAL) to the FindBABoost.cmake module so it matches the re-inclusion protection used by FindBACppRestSdk.cmake and FindBAFleetHttpClient.cmake. Keep the existing CMP0167 policy setup, but place the global include guard at the top of this module so repeated inclusion is safely ignored.cmake/FindBAFleetHttpClient.cmake (1)
28-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winImplicit local-checkout override risks non-reproducible builds.
Redirecting FetchContent to
../fleet-http-clientpurely because that directory happens to exist silently bypasses the pinnedv2.0.2tag with whatever is checked out there. This is useful for local development, but there's no explicit opt-in (e.g., an option/env var), so a stray or stale sibling checkout in CI or another contributor's environment would change what version gets built without any visible signal in the build log stating a deliberate override was requested.Consider gating this behind an explicit option (e.g.,
-DBA_FLEET_HTTP_CLIENT_LOCAL_SOURCE=ON) rather than an implicit path existence check, so the override is always intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/FindBAFleetHttpClient.cmake` around lines 28 - 31, The implicit local checkout override in FindBAFleetHttpClient.cmake should not trigger just because ../fleet-http-client exists. Update the FetchContent override logic to require an explicit opt-in, such as a BA_FLEET_HTTP_CLIENT_LOCAL_SOURCE CMake option (or equivalent env var), before setting FETCHCONTENT_SOURCE_DIR_FLEET_HTTP_CLIENT_SHARED; keep the default path pinned to v2.0.2 otherwise. Make the change in the conditional around the local source redirect so the override is intentional and visible in the build configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmake/Findasync-function-execution-shared.cmake`:
- Around line 3-4: The wrapper module currently hardcodes a required dependency
lookup in Findasync-function-execution-shared.cmake, which ignores the caller’s
find_package flags and version request. Update the BAAsyncFunctionExecution
lookup to forward the outer find_package(async-function-execution-shared ...)
semantics, including QUIET, REQUIRED, and any requested version, so the module
behaves like a proper proxy rather than always forcing a required search.
In `@cmake/FindBAAeron.cmake`:
- Around line 51-54: The include wiring for aeron_driver and aeron_driver_static
is unguarded, so the CMake configure can fail if either target is not created by
FetchContent_MakeAvailable(aeron). Add a TARGET existence check around the
target_include_directories() calls, using the same pattern already used in the
install loop with _aeron_tgt, so include paths are only applied when each target
is actually present.
- Around line 1-9: Remove the global include guard from FindBAAeron.cmake so
repeated find_package(BAAeron) calls can re-evaluate in different directory
scopes. Keep the existing if(TARGET aeron) fast path and the rest of the BAAeron
discovery logic intact, since that already makes the module idempotent without
relying on include_guard(GLOBAL).
In `@cmake/FindBAAsyncFunctionExecution.cmake`:
- Around line 24-49: The FindBAAsyncFunctionExecution module is mutating global
build state by forcing BUILD_SHARED_LIBS OFF in the parent cache. Update the
logic around FetchContent_Declare/FetchContent_MakeAvailable so the
shared/static choice is scoped only to the async_function_execution subproject,
and restore any previous BUILD_SHARED_LIBS value after fetching. Keep the
alias/install handling for async-function-execution-shared, but avoid leaving
global cache changes behind.
In `@cmake/FindBABoost.cmake`:
- Around line 8-12: The Boost config probe in FindBABoost.cmake only checks for
a BoostConfig.cmake file and can incorrectly mark Boost as found even when
required libraries are missing. Update the find_package(Boost 1.86 CONFIG QUIET
...) logic in the FindBABoost module to require the same component set used by
the fallback build, so it verifies the needed libraries before setting
BABoost_FOUND and returning. Use the existing BABoost_FIND_COMPONENTS handling
and the module’s find_package(Boost) flow to ensure missing compiled components
correctly fall back to FetchContent.
- Around line 26-34: The `BUILD_SHARED_LIBS` setting in `FindBABoost.cmake` is
leaking into the caller scope because it is set globally before
`FetchContent_MakeAvailable(Boost)` and never restored. Save the existing value
of `BUILD_SHARED_LIBS` before forcing it OFF for Boost, then restore that saved
value immediately after the Boost fetch/setup completes. Use the
`FetchContent_Declare(Boost)` / `FetchContent_MakeAvailable(Boost)` block as the
place to bracket the temporary override.
In `@cmake/FindBACppRestSdk.cmake`:
- Around line 26-32: The cache writes in FindBACppRestSdk.cmake are using FORCE,
which overrides user- or project-provided values and can leak across
re-configures. Update the option setup around BUILD_SHARED_LIBS,
CPPREST_EXCLUDE_WEBSOCKETS, WERROR, BUILD_SAMPLES, and BUILD_TESTS so they are
only provided as defaults without FORCE, preserving any pre-set values and
avoiding collisions with other fetched dependencies.
In `@cmake/FindBAFleetProtocolCpp.cmake`:
- Around line 12-20: The version requested by find_package is being dropped in
both shim modules, so incompatible config packages can still satisfy the lookup.
Update the find_package call in FindBAFleetProtocolCpp.cmake to forward
BAFleetProtocolCpp_FIND_VERSION, and update
Findfleet-protocol-cxx-helpers-static.cmake to forward
fleet-protocol-cxx-helpers-static_FIND_VERSION, keeping the version requirement
intact through both wrappers.
In `@cmake/FindBAFleetProtocolInterface.cmake`:
- Around line 13-21: The version requirement is being lost in the
`find_package()` shim, so an incompatible config package can still be accepted.
Update the `FindBAFleetProtocolInterface` logic around the
`find_package(fleet-protocol-interface QUIET CONFIG)` call to forward
`BAFleetProtocolInterface_FIND_VERSION` (and ensure the lower-level
`Findfleet-protocol-interface` shim forwards
`fleet-protocol-interface_FIND_VERSION` too), so both package lookups honor the
requested version instead of doing an unversioned config search.
In `@cmake/FindCMLIB.cmake`:
- Around line 12-13: CMDEF_LIBRARY_INSTALL_DIR is hardcoded to “lib” in
FindCMLIB.cmake, but CMDEF_INSTALL uses CMAKE_INSTALL_LIBDIR, so downstream
RPATH computation can diverge on lib64 platforms. Update the FindCMLIB.cmake
logic and the CMDEF_INSTALL-related CMake setup to derive
CMDEF_LIBRARY_INSTALL_DIR from GNUInstallDirs/CMAKE_INSTALL_LIBDIR instead of a
fixed string, keeping the existing variable name so BringAuto CMakeLists still
reference it consistently. Ensure the value matches the actual install
destination used by CMDEF_INSTALL.
- Around line 64-82: CMDEF_INSTALL currently accepts NAMESPACE but only performs
install(TARGETS), so installed targets are not exported as namespaced imports
and downstream find_package() consumers cannot resolve io-module-interface::
targets. Update CMDEF_INSTALL to either wire in an export set plus package
config/version installation so the namespace is actually applied on install, or
remove the unused NAMESPACE argument from CMDEF_INSTALL if namespaced imports
are not intended; use the CMDEF_INSTALL macro and its install(TARGETS) block as
the place to make the change.
In `@cmake/Findfleet-http-client-shared.cmake`:
- Around line 3-4: The wrapper in Findfleet-http-client-shared.cmake is
hardcoding the dependency lookup and ignoring the caller’s find_package
semantics. Update the delegation from BAFleetHttpClient so it forwards the
requested version and preserves QUIET/REQUIRED behavior instead of always
forcing REQUIRED, and keep fleet-http-client-shared_FOUND set based on the
delegated result.
In `@cmake/Findfleet-protocol-cxx-helpers-static.cmake`:
- Around line 3-4: The Findfleet-protocol-cxx-helpers-static package lookup is
forcing BAFleetProtocolCpp to be REQUIRED, which ignores the caller’s requested
version and QUIET/REQUIRED behavior. Update the find_package call in
Findfleet-protocol-cxx-helpers-static so it forwards the caller’s package
arguments to BAFleetProtocolCpp instead of hardcoding REQUIRED, and keep the
helper package’s FOUND status tied to that forwarded lookup.
In `@cmake/Findfleet-protocol-interface.cmake`:
- Around line 3-4: The wrapper in Findfleet-protocol-interface.cmake is
hardcoding find_package(BAFleetProtocolInterface REQUIRED), which ignores the
caller’s QUIET/REQUIRED and version arguments from
find_package(fleet-protocol-interface). Update the forwarding logic so the
delegate call to BAFleetProtocolInterface preserves the original package request
semantics and version constraints, and keep fleet-protocol-interface_FOUND
aligned with the delegated result. Use the existing find_package invocation and
fleet-protocol-interface_FOUND symbol to locate the fix.
---
Nitpick comments:
In `@cmake/FindBABoost.cmake`:
- Around line 1-4: Add include_guard(GLOBAL) to the FindBABoost.cmake module so
it matches the re-inclusion protection used by FindBACppRestSdk.cmake and
FindBAFleetHttpClient.cmake. Keep the existing CMP0167 policy setup, but place
the global include guard at the top of this module so repeated inclusion is
safely ignored.
In `@cmake/FindBAFleetHttpClient.cmake`:
- Around line 28-31: The implicit local checkout override in
FindBAFleetHttpClient.cmake should not trigger just because ../fleet-http-client
exists. Update the FetchContent override logic to require an explicit opt-in,
such as a BA_FLEET_HTTP_CLIENT_LOCAL_SOURCE CMake option (or equivalent env
var), before setting FETCHCONTENT_SOURCE_DIR_FLEET_HTTP_CLIENT_SHARED; keep the
default path pinned to v2.0.2 otherwise. Make the change in the conditional
around the local source redirect so the override is intentional and visible in
the build configuration.
In `@cmake/FindBAFleetProtocolCpp.cmake`:
- Line 39: The module currently marks BAFleetProtocolCpp as found
unconditionally, even if the expected target was never created. Update the
FindBAFleetProtocolCpp.cmake logic so the success flag is set only after
validating that the fleet-protocol-cxx-helpers-static target actually exists,
mirroring the check used in the sibling finder module. Keep the existing
FetchContent flow, but gate the BAFleetProtocolCpp_FOUND assignment behind the
target existence check.
- Line 12: The path check in the module-level directory guard uses MATCHES with
an unescaped build path, so the CMake string is being treated as a regex. Update
the condition in FindBAFleetProtocolCpp.cmake to avoid regex matching on
CMAKE_BINARY_DIR, either by escaping the path before MATCHES or by switching to
a non-regex comparison, and keep the same logic around the
fleet-protocol-cxx-helpers-static_DIR check.
In `@cmake/FindBAFleetProtocolInterface.cmake`:
- Around line 3-4: The header comment in FindBAFleetProtocolInterface.cmake is
out of sync with the aliases created in the foreach that defines the interface
targets. Update the “Provides” comment to list all generated aliases, including
internal-client-interface and module-gateway-interface, so it accurately matches
the alias list used by the module.
- Around line 34-39: The conditional in FindBAFleetProtocolInterface.cmake
currently makes the protobuf include-path fix a silent no-op when either target
is missing. Update the target check around target_link_libraries for
protobuf-cpp-interface and protobuf::libprotobuf to emit a warning or fatal
diagnostic when the expected targets are unavailable, so the missing dependency
is surfaced immediately instead of failing later in ExternalProtocol.pb.cc.
- Line 75: The FindBAFleetProtocolInterface.cmake logic marks
BAFleetProtocolInterface_FOUND as true unconditionally, even when FetchContent
did not create the expected targets. Update the package check around the
FetchContent/target validation so it only sets BAFleetProtocolInterface_FOUND
after confirming common-headers-interface and any required sub-targets actually
exist; otherwise leave it false and fail the find step cleanly.
- Line 13: The path check in the `fleet-protocol-interface_DIR` condition is
using `MATCHES`, so `${CMAKE_BINARY_DIR}` is being treated as a regex pattern
instead of a literal path. Update the comparison in
`FindBAFleetProtocolInterface.cmake` to use an escaped literal form or a
non-regex string comparison so `fleet-protocol-interface_DIR` is matched against
`CMAKE_BINARY_DIR` safely even when the path contains regex metacharacters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d3dde35-627f-42ba-bd5f-dc9c0d942109
📒 Files selected for processing (18)
CMLibStorage.cmakeCMakeLists.txtcmake/Dependencies.cmakecmake/FindBAAeron.cmakecmake/FindBAAsyncFunctionExecution.cmakecmake/FindBABoost.cmakecmake/FindBACppRestSdk.cmakecmake/FindBAFleetHttpClient.cmakecmake/FindBAFleetProtocolCpp.cmakecmake/FindBAFleetProtocolInterface.cmakecmake/FindBANlohmannJson.cmakecmake/FindBAProtobuf.cmakecmake/FindBAZlib.cmakecmake/FindCMLIB.cmakecmake/Findasync-function-execution-shared.cmakecmake/Findfleet-http-client-shared.cmakecmake/Findfleet-protocol-cxx-helpers-static.cmakecmake/Findfleet-protocol-interface.cmake
💤 Files with no reviewable changes (1)
- CMLibStorage.cmake
| find_package(BAAsyncFunctionExecution REQUIRED) | ||
| set(async-function-execution-shared_FOUND TRUE) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Forward the caller's find_package flags.
Hardcoding REQUIRED means find_package(async-function-execution-shared QUIET) will still hard-fail, and any version request is ignored. Please propagate the requested version and QUIET/REQUIRED semantics to BAAsyncFunctionExecution instead of turning every lookup into a required one.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/Findasync-function-execution-shared.cmake` around lines 3 - 4, The
wrapper module currently hardcodes a required dependency lookup in
Findasync-function-execution-shared.cmake, which ignores the caller’s
find_package flags and version request. Update the BAAsyncFunctionExecution
lookup to forward the outer find_package(async-function-execution-shared ...)
semantics, including QUIET, REQUIRED, and any requested version, so the module
behaves like a proper proxy rather than always forcing a required search.
| include_guard(GLOBAL) | ||
|
|
||
| # Provides: aeron (imported target from aeron cmake package) | ||
| # Resolution order: in-scope target → system config package → FetchContent source build. | ||
|
|
||
| if(TARGET aeron) | ||
| set(BAAeron_FOUND TRUE) | ||
| return() | ||
| endif() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
workdir="$(mktemp -d)"
cd "$workdir"
cat > CMakeLists.txt <<'CMAKE'
cmake_minimum_required(VERSION 3.20)
project(test NONE)
# Simulate the package file content.
file(WRITE "${CMAKE_BINARY_DIR}/FindBAAeron.cmake" [=[
include_guard(GLOBAL)
if(TARGET aeron)
set(BAAeron_FOUND TRUE)
return()
endif()
set(BAAeron_FOUND FALSE)
]=])
# First call from top scope, no target.
find_package(BAAeron REQUIRED)
# Create target, then call again from a nested scope.
add_library(aeron INTERFACE)
function(call_again)
find_package(BAAeron REQUIRED)
message(STATUS "BAAeron_FOUND in function: '${BAAeron_FOUND}'")
endfunction()
call_again()
message(STATUS "BAAeron_FOUND in top scope after second call: '${BAAeron_FOUND}'")
CMAKE
cmake -S . -B buildRepository: bringauto/io-module
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the package module and any forwarding shims / call sites.
git ls-files | rg '(^|/)FindBAAeron\.cmake$|(^|/)CMakeLists\.txt$|(^|/)cmake/.*BAAeron|(^|/)aeron'
# Show the package file with line numbers if present.
if git ls-files | rg -q '^cmake/FindBAAeron\.cmake$'; then
echo "----- cmake/FindBAAeron.cmake -----"
cat -n cmake/FindBAAeron.cmake
fi
# Search for related variables / find_package calls.
rg -n --hidden --glob '!**/.git/**' 'find_package\s*\(\s*(BAAeron|aeron)\b|BAAeron_FOUND|include_guard\s*\(' .Repository: bringauto/io-module
Length of output: 4507
Remove the global include guard here
include_guard(GLOBAL) can break repeated find_package(BAAeron) calls from a different directory scope that doesn’t already inherit BAAeron_FOUND. The if(TARGET aeron) fast path already makes this file idempotent, so the guard is unnecessary and risky.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/FindBAAeron.cmake` around lines 1 - 9, Remove the global include guard
from FindBAAeron.cmake so repeated find_package(BAAeron) calls can re-evaluate
in different directory scopes. Keep the existing if(TARGET aeron) fast path and
the rest of the BAAeron discovery logic intact, since that already makes the
module idempotent without relying on include_guard(GLOBAL).
| target_include_directories(aeron_driver PUBLIC | ||
| "$<BUILD_INTERFACE:${_aeron_inc_shim}>") | ||
| target_include_directories(aeron_driver_static PUBLIC | ||
| "$<BUILD_INTERFACE:${_aeron_inc_shim}>") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing TARGET guard before wiring includes into aeron_driver/aeron_driver_static.
Unlike the install loop below (lines 59-66) which checks if(TARGET ${_aeron_tgt}) before acting, these two target_include_directories() calls assume the targets unconditionally exist after FetchContent_MakeAvailable(aeron). If either target is absent (e.g. due to platform-specific Aeron build configuration), this fails the configure with a hard CMake error rather than degrading gracefully.
🛡️ Proposed fix
-target_include_directories(aeron_driver PUBLIC
- "$<BUILD_INTERFACE:${_aeron_inc_shim}>")
-target_include_directories(aeron_driver_static PUBLIC
- "$<BUILD_INTERFACE:${_aeron_inc_shim}>")
+if(TARGET aeron_driver)
+ target_include_directories(aeron_driver PUBLIC
+ "$<BUILD_INTERFACE:${_aeron_inc_shim}>")
+endif()
+if(TARGET aeron_driver_static)
+ target_include_directories(aeron_driver_static PUBLIC
+ "$<BUILD_INTERFACE:${_aeron_inc_shim}>")
+endif()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target_include_directories(aeron_driver PUBLIC | |
| "$<BUILD_INTERFACE:${_aeron_inc_shim}>") | |
| target_include_directories(aeron_driver_static PUBLIC | |
| "$<BUILD_INTERFACE:${_aeron_inc_shim}>") | |
| if(TARGET aeron_driver) | |
| target_include_directories(aeron_driver PUBLIC | |
| "$<BUILD_INTERFACE:${_aeron_inc_shim}>") | |
| endif() | |
| if(TARGET aeron_driver_static) | |
| target_include_directories(aeron_driver_static PUBLIC | |
| "$<BUILD_INTERFACE:${_aeron_inc_shim}>") | |
| endif() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/FindBAAeron.cmake` around lines 51 - 54, The include wiring for
aeron_driver and aeron_driver_static is unguarded, so the CMake configure can
fail if either target is not created by FetchContent_MakeAvailable(aeron). Add a
TARGET existence check around the target_include_directories() calls, using the
same pattern already used in the install loop with _aeron_tgt, so include paths
are only applied when each target is actually present.
| set(BUILD_SHARED_LIBS OFF CACHE BOOL "" FORCE) | ||
| if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/../async-function-execution/CMakeLists.txt") | ||
| set(FETCHCONTENT_SOURCE_DIR_ASYNC_FUNCTION_EXECUTION | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/../async-function-execution" CACHE PATH "" FORCE) | ||
| endif() | ||
| FetchContent_Declare(async_function_execution | ||
| GIT_REPOSITORY https://github.com/bringauto/async-function-execution.git | ||
| GIT_TAG v1.0.0 | ||
| GIT_SHALLOW TRUE) | ||
| FetchContent_MakeAvailable(async_function_execution) | ||
| # CMDEF_ADD_LIBRARY creates async-function-execution-shared; create the namespaced alias. | ||
| if(TARGET async-function-execution-shared | ||
| AND NOT TARGET async-function-execution-shared::async-function-execution-shared) | ||
| add_library(async-function-execution-shared::async-function-execution-shared | ||
| ALIAS async-function-execution-shared) | ||
| endif() | ||
|
|
||
| # Install the shared library so the binary can find it via RPATH at runtime. | ||
| # CMDEF_INSTALL() is a no-op in our shim, so we register the install rule here. | ||
| if(TARGET async-function-execution-shared) | ||
| get_target_property(_afe_ttype async-function-execution-shared TYPE) | ||
| if(_afe_ttype STREQUAL "SHARED_LIBRARY") | ||
| install(TARGETS async-function-execution-shared | ||
| LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}) | ||
| endif() | ||
| unset(_afe_ttype) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid forcing BUILD_SHARED_LIBS from inside a find module.
CACHE FORCE rewrites the parent cache, so every later dependency fetched in this configure inherits OFF. That can flip unrelated targets between static/shared builds and make the install rule below disappear if the fetched project obeys the override. Please scope this to the FetchContent subproject and restore the previous value instead of mutating global state here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/FindBAAsyncFunctionExecution.cmake` around lines 24 - 49, The
FindBAAsyncFunctionExecution module is mutating global build state by forcing
BUILD_SHARED_LIBS OFF in the parent cache. Update the logic around
FetchContent_Declare/FetchContent_MakeAvailable so the shared/static choice is
scoped only to the async_function_execution subproject, and restore any previous
BUILD_SHARED_LIBS value after fetching. Keep the alias/install handling for
async-function-execution-shared, but avoid leaving global cache changes behind.
| find_package(Boost 1.86 CONFIG QUIET COMPONENTS ${BABoost_FIND_COMPONENTS}) | ||
| if(Boost_FOUND) | ||
| set(BABoost_FOUND TRUE) | ||
| return() | ||
| endif() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
System-Boost check doesn't validate required components.
BABoost_FIND_COMPONENTS is empty in the actual call sites (find_package(BABoost REQUIRED) in Dependencies.cmake, FIND_PACKAGE(Boost CONFIG REQUIRED) in CMakeLists.txt never pass COMPONENTS), so line 8 effectively becomes find_package(Boost 1.86 CONFIG QUIET COMPONENTS) — it only checks that a BoostConfig.cmake exists, not that regex/date_time/filesystem/thread/etc. are actually built. A system Boost package that's missing some of these compiled libraries would still satisfy Boost_FOUND here, and downstream target_link_libraries(... Boost::filesystem ...) calls would fail later with a much more confusing "target not found" error instead of falling through to the FetchContent build that guarantees the full set.
🐛 Proposed fix: require the same component set used for the fallback build
+set(_BABoost_REQUIRED_COMPONENTS
+ regex date_time atomic random chrono system filesystem thread asio uuid)
find_package(Boost 1.86 CONFIG QUIET COMPONENTS ${BABoost_FIND_COMPONENTS})
+find_package(Boost 1.86 CONFIG QUIET COMPONENTS ${_BABoost_REQUIRED_COMPONENTS})
if(Boost_FOUND)
set(BABoost_FOUND TRUE)
return()
endif()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| find_package(Boost 1.86 CONFIG QUIET COMPONENTS ${BABoost_FIND_COMPONENTS}) | |
| if(Boost_FOUND) | |
| set(BABoost_FOUND TRUE) | |
| return() | |
| endif() | |
| set(_BABoost_REQUIRED_COMPONENTS | |
| regex date_time atomic random chrono system filesystem thread asio uuid) | |
| find_package(Boost 1.86 CONFIG QUIET COMPONENTS ${BABoost_FIND_COMPONENTS}) | |
| find_package(Boost 1.86 CONFIG QUIET COMPONENTS ${_BABoost_REQUIRED_COMPONENTS}) | |
| if(Boost_FOUND) | |
| set(BABoost_FOUND TRUE) | |
| return() | |
| endif() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/FindBABoost.cmake` around lines 8 - 12, The Boost config probe in
FindBABoost.cmake only checks for a BoostConfig.cmake file and can incorrectly
mark Boost as found even when required libraries are missing. Update the
find_package(Boost 1.86 CONFIG QUIET ...) logic in the FindBABoost module to
require the same component set used by the fallback build, so it verifies the
needed libraries before setting BABoost_FOUND and returning. Use the existing
BABoost_FIND_COMPONENTS handling and the module’s find_package(Boost) flow to
ensure missing compiled components correctly fall back to FetchContent.
| # Variable referenced in RPATH settings by BringAuto CMakeLists files. | ||
| set(CMDEF_LIBRARY_INSTALL_DIR "lib") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
CMDEF_LIBRARY_INSTALL_DIR hardcoded to "lib" diverges from actual install destination.
CMDEF_INSTALL installs libraries to ${CMAKE_INSTALL_LIBDIR} (Line 76), but CMDEF_LIBRARY_INSTALL_DIR is hardcoded to "lib" (Line 13) and, per the comment, is used by downstream BringAuto CMakeLists.txt files to compute RPATH. On distros where GNUInstallDirs resolves CMAKE_INSTALL_LIBDIR to lib64 (e.g. Fedora/RHEL/openSUSE x86_64), the RPATH computed from this variable won't match the real install location, risking "shared library not found" at runtime.
🔧 Proposed fix
-set(CMDEF_LIBRARY_INSTALL_DIR "lib")
+set(CMDEF_LIBRARY_INSTALL_DIR "${CMAKE_INSTALL_LIBDIR}")Also applies to: 64-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/FindCMLIB.cmake` around lines 12 - 13, CMDEF_LIBRARY_INSTALL_DIR is
hardcoded to “lib” in FindCMLIB.cmake, but CMDEF_INSTALL uses
CMAKE_INSTALL_LIBDIR, so downstream RPATH computation can diverge on lib64
platforms. Update the FindCMLIB.cmake logic and the CMDEF_INSTALL-related CMake
setup to derive CMDEF_LIBRARY_INSTALL_DIR from
GNUInstallDirs/CMAKE_INSTALL_LIBDIR instead of a fixed string, keeping the
existing variable name so BringAuto CMakeLists still reference it consistently.
Ensure the value matches the actual install destination used by CMDEF_INSTALL.
| if(NOT COMMAND CMDEF_INSTALL) | ||
| # Unlike fetched third-party deps (which get their own manual install() rules | ||
| # in cmake/FindBA*.cmake), the module's own gateway/external-server .so is | ||
| # ONLY installed through this macro — a no-op here would make | ||
| # -DBRINGAUTO_INSTALL=ON silently produce an empty install tree for the | ||
| # actual deliverable. Do a real install (harmless no-op on INTERFACE | ||
| # libraries, which have no artifacts to copy). | ||
| macro(CMDEF_INSTALL) | ||
| cmake_parse_arguments(_cmdef_inst "" "TARGET;NAMESPACE" "" ${ARGN}) | ||
| if(_cmdef_inst_TARGET AND TARGET ${_cmdef_inst_TARGET}) | ||
| install(TARGETS ${_cmdef_inst_TARGET} | ||
| RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR} | ||
| LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
| ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}) | ||
| endif() | ||
| unset(_cmdef_inst_TARGET) | ||
| unset(_cmdef_inst_NAMESPACE) | ||
| endmacro() | ||
| endif() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how many CMDEF_INSTALL calls use NAMESPACE, to gauge how much of the packaging story depends on it.
rg -n -B2 -A3 'CMDEF_INSTALL' CMakeLists.txtRepository: bringauto/io-module
Length of output: 732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the macro and surrounding packaging logic.
sed -n '1,220p' cmake/FindCMLIB.cmake
printf '\n---\n'
# Find any export/config/package install machinery in the repo.
rg -n --hidden --glob '!**/.git/**' \
-e 'install\s*\(.*EXPORT' \
-e 'install\s*\(EXPORT' \
-e 'export\s*\(' \
-e 'configure_package_config_file' \
-e 'write_basic_package_version_file' \
-e 'find_package\s*\(' \
-e 'NAMESPACE' \
cmake CMakeLists.txt .Repository: bringauto/io-module
Length of output: 10503
CMDEF_INSTALL ignores NAMESPACE; installed targets still won’t be namespaced imports.
It only runs install(TARGETS ...), so io-module-interface::... can’t be resolved from the install tree. If downstreams are meant to consume this via find_package(), add an export set plus package config/version files, or drop the unused NAMESPACE argument.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/FindCMLIB.cmake` around lines 64 - 82, CMDEF_INSTALL currently accepts
NAMESPACE but only performs install(TARGETS), so installed targets are not
exported as namespaced imports and downstream find_package() consumers cannot
resolve io-module-interface:: targets. Update CMDEF_INSTALL to either wire in an
export set plus package config/version installation so the namespace is actually
applied on install, or remove the unused NAMESPACE argument from CMDEF_INSTALL
if namespaced imports are not intended; use the CMDEF_INSTALL macro and its
install(TARGETS) block as the place to make the change.
| find_package(BAFleetHttpClient REQUIRED) | ||
| set(fleet-http-client-shared_FOUND TRUE) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Forward the caller's find_package flags.
Hardcoding REQUIRED means find_package(fleet-http-client-shared QUIET) will still hard-fail, and any version request is ignored. Please preserve the caller's version and QUIET/REQUIRED semantics when delegating to BAFleetHttpClient.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/Findfleet-http-client-shared.cmake` around lines 3 - 4, The wrapper in
Findfleet-http-client-shared.cmake is hardcoding the dependency lookup and
ignoring the caller’s find_package semantics. Update the delegation from
BAFleetHttpClient so it forwards the requested version and preserves
QUIET/REQUIRED behavior instead of always forcing REQUIRED, and keep
fleet-http-client-shared_FOUND set based on the delegated result.
| find_package(BAFleetProtocolCpp REQUIRED) | ||
| set(fleet-protocol-cxx-helpers-static_FOUND TRUE) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Forward the caller's find_package flags.
Hardcoding REQUIRED means find_package(fleet-protocol-cxx-helpers-static QUIET) will still hard-fail, and any version request is ignored. Please forward the caller's requested version and QUIET/REQUIRED behavior to BAFleetProtocolCpp instead of forcing a fixed mode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/Findfleet-protocol-cxx-helpers-static.cmake` around lines 3 - 4, The
Findfleet-protocol-cxx-helpers-static package lookup is forcing
BAFleetProtocolCpp to be REQUIRED, which ignores the caller’s requested version
and QUIET/REQUIRED behavior. Update the find_package call in
Findfleet-protocol-cxx-helpers-static so it forwards the caller’s package
arguments to BAFleetProtocolCpp instead of hardcoding REQUIRED, and keep the
helper package’s FOUND status tied to that forwarded lookup.
| find_package(BAFleetProtocolInterface REQUIRED) | ||
| set(fleet-protocol-interface_FOUND TRUE) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Forward the caller's find_package flags.
Hardcoding REQUIRED means find_package(fleet-protocol-interface QUIET) will still hard-fail, and any version request is ignored. Please preserve the caller's version and QUIET/REQUIRED semantics when delegating to BAFleetProtocolInterface.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/Findfleet-protocol-interface.cmake` around lines 3 - 4, The wrapper in
Findfleet-protocol-interface.cmake is hardcoding
find_package(BAFleetProtocolInterface REQUIRED), which ignores the caller’s
QUIET/REQUIRED and version arguments from
find_package(fleet-protocol-interface). Update the forwarding logic so the
delegate call to BAFleetProtocolInterface preserves the original package request
semantics and version constraints, and keep fleet-protocol-interface_FOUND
aligned with the delegated result. Use the existing find_package invocation and
fleet-protocol-interface_FOUND symbol to locate the fix.
2 participants
Summary
Migrates io-module (v1.3.5) off cmlib onto plain CMake + FetchContent,
following the same pattern established in external-server-cpp (BAF-1706).
FIND_PACKAGE(CMLIB ...)/CMDEF_ADD_LIBRARY/CMDEF_INSTALLusage with a
cmake/FindCMLIB.cmakecompatibility shim, so the rest ofCMakeLists.txt is unchanged.
FindBA*.cmakemodules that resolve each dependency via a system/config package first, falling back to FetchContent (nlohmann_json,
fleet-protocol-interface, fleet-protocol-cxx-helpers-static,
async-function-execution-shared, aeron, fleet-http-client-shared, Boost,
ZLIB, cpprestsdk).
Fixes on top of the original migration commit
Found while verifying the migration builds:
FIND_PACKAGE(CMLIB COMPONENTS CMDEF CMUTIL STORAGE REQUIRED)fromCMakeLists.txt, so
cmake/FindCMLIB.cmake(which definesCMDEF_ADD_LIBRARY/CMDEF_INSTALL) was never loaded. Restored the call.CMDEF_PACKAGEshim was a no-op andsilently dropped its
VERSIONargument, socpackalways produced0.1.1instead of the real module version. Shim now setsCPACK_PACKAGE_VERSIONfrom the passed version.Note: CMakeLists.txt still pins
IO_MODULE_VERSION 1.3.5; the currentreleased tag is v1.3.6. Pre-existing version-bump gap, unrelated to this
migration — flagging in case it's worth a follow-up.
Verification
-DFLEET_PROTOCOL_BUILD_EXTERNAL_SERVER=OFF): configure, build, andcpackall pass —io_module-1.3.5-Linux.zip.-DBRINGAUTO_PACKAGE=ON, matching what the real deploy pipeline runs):passes.
fleet-os-deploypipeline end to end against a temp tag onthis branch (
temp/baf-1716-deploy-verify) — steps 1-7 and the aarch64dependency-sysroot assembly all pass. The run is currently blocked only
by the
fleet-os-3cross-compile Docker image not being available in anyreachable registry — an infra gap unrelated to this change (it fails
before even reaching this module, on
module-gateway).Closes BAF-1716.
Summary by CodeRabbit
New Features
Bug Fixes