Conversation
📝 WalkthroughWalkthroughReplaces internal _BRINGAUTO_PACKAGE with a public BA_PACKAGE, adds GIT_PATH_TEMPLATE and URI-escaping support, surfaces HTTP header and git-archive-path config, updates STORAGE/PREREQ initialization, and adds a comprehensive cmake_tests suite with mocks for unit testing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant BA as BA_PACKAGE
participant PR as BA_PACKAGE_PREREQ_CMCONF_INIT
participant CM as CMCONF/CMCONF mock
participant CL as CMLIB_DEPENDENCY (mock)
participant ST as CMLIB_STORAGE_TEMPLATE_INSTANCE (mock)
BA->>PR: request template, revision, git_archive_path, http_header, escape_flag
PR->>CM: CMCONF_GET(...) queries (URI_TEMPLATE, REVISION, GIT_ARCHIVE_PATH_TEMPLATE, HTTP_HEADER, TEMPLATE_ARGS_URI_ESCAPE)
CM-->>PR: return configured values
PR-->>BA: return resolved template, revision, git_archive_path, http_header, escape_flag
BA->>ST: expand storage template (ARCHIVE_NAME, GIT_PATH etc.)
ST-->>BA: return URI
BA->>CL: invoke CMLIB_DEPENDENCY with TYPE/URI and optional GIT_REVISION/GIT_PATH args
CL-->>BA: return OUTPUT_PATH_VAR (e.g., /mock/cache/path)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helpers/BA_PACKAGE_PREREQ.cmake (1)
27-40:⚠️ Potential issue | 🟡 MinorDoc block does not document the three new output parameters.
The function comment (lines 35–38) still describes only the original two parameters (
template_output_var,revision_output_var). The three new parameters (git_archive_path_template_output_var,http_header_output_var,template_args_uri_escape_output_var) added at line 40 are completely absent from the documentation.📝 Suggested fix
# <function>( # <template_output_var> // variable name to receive the template (parent scope) # <revision_output_var> // variable name to receive the revision (parent scope) +# <git_archive_path_template_output_var> // variable name to receive the git archive path template (parent scope) +# <http_header_output_var> // variable name to receive the HTTP header (parent scope) +# <template_args_uri_escape_output_var> // variable name to receive the URI-escape flag (parent scope) # )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helpers/BA_PACKAGE_PREREQ.cmake` around lines 27 - 40, Doc block for FUNCTION BA_PACKAGE_PREREQ_CMCONF_INIT is missing documentation for the three new output parameters; update the comment above the function to list and describe git_archive_path_template_output_var, http_header_output_var, and template_args_uri_escape_output_var (what they return, scope=parent, and format/expected values), matching the style used for template_output_var and revision_output_var so callers understand the additional outputs.
🧹 Nitpick comments (16)
test/app/tests_list/gstreamer_style_naming.txt (1)
32-52: Comment documents more than this test installs — consider clarifying.The directory tree (lines 32–52) shows entries for
shared-library-shared,shared-library-for-prerun, andshared_out_of_dir_symlink.hthatTEST_PRERUNdoes not install (it only installsshared_library_gst). If these come from a shared base fixture, a brief note like# (shared base dependencies added by test framework)above the non-gst entries would prevent future readers from wondering where those files originate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/app/tests_list/gstreamer_style_naming.txt` around lines 32 - 52, The directory tree comment lists files not installed by TEST_PRERUN (only shared_library_gst is), which is confusing; update the comment in test/app/tests_list/gstreamer_style_naming.txt to clarify origin by adding a short note above the non-gst entries (e.g. "# (shared base dependencies added by test framework)") and/or explicitly mark which entries are installed by TEST_PRERUN (mention shared_library_gst) so readers understand that shared-library-shared, shared-library-for-prerun and shared_out_of_dir_symlink.h come from the shared base fixture.cmake_tests/TEST.cmake (1)
7-7: Consider guarding theINCLUDEwith an existence check.If the CMLIB installation doesn't ship
test/TEST.cmake(e.g., a stripped or minimal build), CMake will emit an opaque "file not found" error. Theai_summarydescribes the include as "when available", implying optional intent.♻️ Suggested defensive guard
-INCLUDE("${CMLIB_DIR}/test/TEST.cmake") +IF(EXISTS "${CMLIB_DIR}/test/TEST.cmake") + INCLUDE("${CMLIB_DIR}/test/TEST.cmake") +ELSE() + MESSAGE(FATAL_ERROR "Cannot find CMLIB test helper '${CMLIB_DIR}/test/TEST.cmake'") +ENDIF()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/TEST.cmake` at line 7, Guard the INCLUDE call so CMake only tries to include the optional test/TEST.cmake if it actually exists: check IF(EXISTS "<path>") using the same variable CMLIB_DIR and the literal test/TEST.cmake before calling INCLUDE("${CMLIB_DIR}/test/TEST.cmake"), and optionally emit a message(NOTICE/STATUS) when the file is skipped to make the behavior explicit; update the invocation around the INCLUDE(...) line and any related logic that assumes that file was loaded.cmake_tests/mock/CMUTIL.cmake (1)
26-29: Config variable initialization is inconsistent with sibling mocks.
MOCK_CMUTIL_PLATFORM_STRINGis initialized via a bareSET(... CACHE INTERNAL ...), whileCMLIB_CACHE.cmakeandCMLIB_STORAGE.cmakeboth use an explicitIF(NOT DEFINED ...)guard for their configurable return-value variables. Both patterns are functionally equivalent forCACHE INTERNAL(sinceSETwithoutFORCEwon't overwrite an existing cache entry), but the inconsistency may confuse future contributors.♻️ Optional: align with the sibling-mock pattern
-SET(MOCK_CMUTIL_PLATFORM_STRING "" - CACHE INTERNAL - "Platform string to return; uses constructed value if empty" -) +IF(NOT DEFINED MOCK_CMUTIL_PLATFORM_STRING) + SET(MOCK_CMUTIL_PLATFORM_STRING "" + CACHE INTERNAL + "Platform string to return; uses constructed value if empty" + ) +ENDIF()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/mock/CMUTIL.cmake` around lines 26 - 29, MOCK_CMUTIL_PLATFORM_STRING is set directly with SET(... CACHE INTERNAL ...) unlike sibling mocks; change the initialization in CMUTIL.cmake to use the same guarded pattern as CMLIB_CACHE.cmake and CMLIB_STORAGE.cmake by wrapping the cache variable assignment in an IF(NOT DEFINED MOCK_CMUTIL_PLATFORM_STRING) guard so the configurable return-value variable is only defined when absent, matching the sibling-mock style and avoiding future confusion.cmake_tests/mock/CMLIB_DEPENDENCY.cmake (1)
74-74: Return path is hardcoded; no configuration variable exposed.
CMLIB_CACHEandCMLIB_STORAGEmocks both expose aMOCK_..._RETURN_PATH/MOCK_..._OUTPUTconfiguration variable so tests can drive different return values.CMLIB_DEPENDENCYhard-wires"/mock/cache/path", making it impossible to test scenarios that need a different output path (e.g., per-package paths, failure injection) without modifying the mock file.♻️ Optional: align with sibling-mock pattern
Add at the top-level initialization block:
+IF(NOT DEFINED MOCK_CMLIB_DEPENDENCY_RETURN_PATH) + SET(MOCK_CMLIB_DEPENDENCY_RETURN_PATH "/mock/cache/path" + CACHE INTERNAL + "Path returned via OUTPUT_PATH_VAR; default simulates a successful cache hit" + ) +ENDIF()Then in the function body:
- SET(${__OUTPUT_PATH_VAR} "/mock/cache/path" PARENT_SCOPE) + SET(${__OUTPUT_PATH_VAR} "${MOCK_CMLIB_DEPENDENCY_RETURN_PATH}" PARENT_SCOPE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/mock/CMLIB_DEPENDENCY.cmake` at line 74, The mock currently hardcodes the return path by setting ${__OUTPUT_PATH_VAR} to "/mock/cache/path"; change this to read a configurable variable so tests can override it. Add a top-level default/initialization for a new config variable (e.g., CMLIB_DEPENDENCY_MOCK_OUTPUT or MOCK_DEPENDENCY_OUTPUT) and in the function that sets ${__OUTPUT_PATH_VAR} replace the hardcoded string with that config variable (falling back to "/mock/cache/path" if not defined) so callers can set the mock return path without editing the mock file.cmake_tests/mock/CMLIB_STORAGE.cmake (1)
76-87: Conditional capture makes "explicitly passed empty" indistinguishable from "not passed".The
IF(__ARCHIVE_NAME)/IF(__GIT_PATH)/ etc. guards mean that if a caller passes an explicitly empty value, the captured cache variable is not updated and retains its initialized"". A test asserting that an argument was passed as""would pass vacuously. Compare with lines 68–72 ofCMLIB_DEPENDENCY.cmake, where all captures are unconditional.For the tests introduced in this PR this has no impact, but it's a subtle trap for future test authors.
♻️ Optional: capture unconditionally
- IF(__ARCHIVE_NAME) - SET_PROPERTY(CACHE MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME PROPERTY VALUE "${__ARCHIVE_NAME}") - ENDIF() - IF(__GIT_PATH) - SET_PROPERTY(CACHE MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_GIT_PATH PROPERTY VALUE "${__GIT_PATH}") - ENDIF() - IF(__REVISION) - SET_PROPERTY(CACHE MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_REVISION PROPERTY VALUE "${__REVISION}") - ENDIF() - IF(__PACKAGE_GROUP_NAME) - SET_PROPERTY(CACHE MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_PACKAGE_GROUP_NAME PROPERTY VALUE "${__PACKAGE_GROUP_NAME}") - ENDIF() + SET_PROPERTY(CACHE MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME PROPERTY VALUE "${__ARCHIVE_NAME}") + SET_PROPERTY(CACHE MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_GIT_PATH PROPERTY VALUE "${__GIT_PATH}") + SET_PROPERTY(CACHE MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_REVISION PROPERTY VALUE "${__REVISION}") + SET_PROPERTY(CACHE MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_PACKAGE_GROUP_NAME PROPERTY VALUE "${__PACKAGE_GROUP_NAME}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/mock/CMLIB_STORAGE.cmake` around lines 76 - 87, The IF guards around the captures for __ARCHIVE_NAME, __GIT_PATH, __REVISION and __PACKAGE_GROUP_NAME make an explicitly passed empty string indistinguishable from "not passed"; change the code so each SET_PROPERTY(CACHE MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_... PROPERTY VALUE "${__...}") is executed unconditionally (remove the IF/ENDIF wrappers) so empty values overwrite the cached variables, mirroring the unconditional captures used in CMLIB_DEPENDENCY.cmake.cmake_tests/BA_PACKAGE/release_basic/CMakeLists.txt (1)
26-37: Consider adding a negative assertion for theDEBUGkeyword.The test verifies
BACPACK,MYPKG, andRELEASEare present but doesn't assert thatDEBUGis absent in Release mode. A symmetrical check would guard against regressions where both are accidentally added.✨ Proposed addition
IF(idx_release EQUAL -1) MESSAGE(FATAL_ERROR "KEYWORDS missing RELEASE: ${MOCK_CMLIB_DEPENDENCY_KEYWORDS}") ENDIF() + +LIST(FIND MOCK_CMLIB_DEPENDENCY_KEYWORDS "DEBUG" idx_debug) +IF(NOT idx_debug EQUAL -1) + MESSAGE(FATAL_ERROR "KEYWORDS should NOT contain DEBUG in Release build: ${MOCK_CMLIB_DEPENDENCY_KEYWORDS}") +ENDIF()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/BA_PACKAGE/release_basic/CMakeLists.txt` around lines 26 - 37, Add a negative assertion that the DEBUG keyword is not present in the MOCK_CMLIB_DEPENDENCY_KEYWORDS for this Release test: use LIST(FIND MOCK_CMLIB_DEPENDENCY_KEYWORDS "DEBUG" idx_debug) and then fail with MESSAGE(FATAL_ERROR "...") if idx_debug is NOT EQUAL -1, similar to the existing checks for BACPACK/MYPKG/RELEASE (place the check alongside the other LIST(FIND)/IF blocks referencing MOCK_CMLIB_DEPENDENCY_KEYWORDS and idx_*).cmake_tests/BA_PACKAGE_EXECUTABLE/CMakeLists.txt (1)
10-13: Missing error-case and warning test registrations per AI summary.The AI-generated summary describes
package_name_dash_warning(withALWAYS_SCRIPT_MODE ON) and twoTEST_INVALID_CMAKE_RUNinvocations (invalid_package_name_error,cache_only_not_found_error) that are absent from the actual code. Like theBA_PACKAGE_LIBRARYsuite, this leaves error handling paths for the executable wrapper untested through this test harness.If these are intentionally deferred or covered elsewhere, a brief comment noting that would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/BA_PACKAGE_EXECUTABLE/CMakeLists.txt` around lines 10 - 13, The test list for BA_PACKAGE_EXECUTABLE is missing the error/warning registrations referenced in the AI summary; add TEST_RUN for the warning case and TEST_INVALID_CMAKE_RUN entries for the two error cases or explicitly document why they're omitted. Specifically, add a test registration for package_name_dash_warning with ALWAYS_SCRIPT_MODE ON (or the equivalent TEST_RUN invocation that enables ALWAYS_SCRIPT_MODE), and add TEST_INVALID_CMAKE_RUN registrations named invalid_package_name_error and cache_only_not_found_error so the executable wrapper’s error paths are exercised (use the same TEST_RUN/TEST_INVALID_CMAKE_RUN helpers used elsewhere); if these cases are intentionally covered elsewhere, insert a short comment above the TEST_RUN list clarifying that omission.cmake_tests/BA_PACKAGE_LIBRARY/CMakeLists.txt (1)
10-13: Missing test scenarios — BA_PACKAGE_LIBRARY suite is significantly narrower than the BA_PACKAGE suite.The AI-generated summary describes 13 tests for this suite (including
release_basic,debug_keyword,no_git_args_without_template,cache_only_skips_dependency,escape_template_args_applied/not_applied,package_name_dash_warning,invalid_package_name_error, andcache_only_not_found_error), but only 4 are actually registered. The namedebug_suffix_appliedin the summary also doesn't matchdebug_suffixin the code.Scenarios like
cache_only_skips_dependency,escape_template_args_applied/not_applied,no_git_args_without_template, andinvalid_package_name_errorare equally applicable toBA_PACKAGE_LIBRARYcallers. If these are intentionally delegated to the underlyingBA_PACKAGEsuite, a brief comment explaining the scope decision would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/BA_PACKAGE_LIBRARY/CMakeLists.txt` around lines 10 - 13, The BA_PACKAGE_LIBRARY suite currently only registers four tests (TEST_RUN entries for debug_suffix, no_debug_skips_suffix, output_path_var_propagates, cmake_prefix_path_updated) but the review expects additional scenarios; either add TEST_RUN entries for the missing tests referenced in the summary (release_basic, debug_keyword or debug_suffix_applied, no_git_args_without_template, cache_only_skips_dependency, escape_template_args_applied, escape_template_args_not_applied, package_name_dash_warning, invalid_package_name_error, cache_only_not_found_error) to this CMakeLists, or add a clear comment in the BA_PACKAGE_LIBRARY suite explaining that those scenarios are intentionally delegated to BA_PACKAGE; also reconcile the name mismatch for debug_suffix_applied vs debug_suffix so the test name matches the summary or vice versa.cmake_tests/BA_PACKAGE/debug_suffix_applied/CMakeLists.txt (1)
1-10: Misleading test name: this tests generic prefix/suffix, not debug suffix.The directory
debug_suffix_appliedand projectTEST_BA_PACKAGE_DEBUG_SUFFIX_APPLIEDimply Debug-build suffix behavior, but the test setsCMAKE_BUILD_TYPE "Release"and uses arbitrary"myprefix"/"mysuffix"values. This is a general prefix+suffix expansion test. Consider renaming to something likeprefix_suffix_applied/TEST_BA_PACKAGE_PREFIX_SUFFIX_APPLIEDto avoid confusion with the debug-specific test inBA_PACKAGE_LIBRARY/debug_suffix/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/BA_PACKAGE/debug_suffix_applied/CMakeLists.txt` around lines 1 - 10, Rename the test to reflect that it verifies generic prefix/suffix expansion rather than a debug-only suffix: change the directory name from "debug_suffix_applied" to "prefix_suffix_applied" and update the project identifier in CMakeLists.txt from PROJECT(TEST_BA_PACKAGE_DEBUG_SUFFIX_APPLIED) to PROJECT(TEST_BA_PACKAGE_PREFIX_SUFFIX_APPLIED), and optionally adjust any test labels or metadata that reference the old name; leave the CMAKE_BUILD_TYPE "Release" and the arbitrary "myprefix"/"mysuffix" values as-is since the test is not debug-specific.cmake_tests/BA_PACKAGE_PREREQ_CMCONF_INIT/env_var_with_local_use_warning/CMakeLists.txt (1)
1-29: Warning behavior is untestable — consider documenting the limitation or testing via output capture.The test name and top-of-file comment claim the test verifies that a warning is emitted when
LOCAL_USE=ONandENV{BA_PACKAGE_LOCAL_PATH}are both set, but only the output variable values are asserted.MESSAGE(WARNING)does not produce a non-zero CMake exit code, so the warning itself is never validated.Either:
- Add a comment noting the warning can't be automatically asserted (so the test scope is intentionally limited to verifying output variables), or
- If the cmake_tests runner supports
PASS_REGULAR_EXPRESSION/ stderr capture, capture and match the expected warning text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/BA_PACKAGE_PREREQ_CMCONF_INIT/env_var_with_local_use_warning/CMakeLists.txt` around lines 1 - 29, The test currently asserts only output variables but claims to verify a MESSAGE(WARNING) when both TEST_CMCONF_BA_PACKAGE_LOCAL_USE / BA_PACKAGE_LOCAL_USE and ENV{BA_PACKAGE_LOCAL_PATH} are set; either document that the warning cannot be programmatically asserted here by adding a top-file comment clarifying the limitation (reference: BA_PACKAGE_PREREQ_CMCONF_INIT invocation, TEST_CMCONF_BA_PACKAGE_LOCAL_USE, and ENV{BA_PACKAGE_LOCAL_PATH}), or update the test harness to capture/stub CMake output and assert the warning text (use the test runner’s PASS_REGULAR_EXPRESSION or stderr-capture mechanism to match the expected MESSAGE(WARNING) string emitted by BA_PACKAGE_PREREQ_CMCONF_INIT).cmake_tests/BA_PACKAGE_EXECUTABLE/debug_suffix/CMakeLists.txt (1)
32-35: The"lib"substring check is fragile — consider anchoring to the package name segment.
STRING(FIND ... "lib")matches anywhere in the URI (e.g., a future mock path likehttps://gitlab.com/...would false-positive). Since the mock URL is tightly controlled today this is low-risk, but a more targeted pattern (e.g., checking the package-name portion of the URI starts withoutlib) would be more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/BA_PACKAGE_EXECUTABLE/debug_suffix/CMakeLists.txt` around lines 32 - 35, The current STRING(FIND check against MOCK_CMLIB_DEPENDENCY_URI is too broad; instead extract the package-name segment from the URI and validate that that segment does not start with "lib". Update the check to first isolate the last path component (e.g., via get_filename_component or splitting on "/" into components) and then perform an anchored test (STRING(REGEX MATCH "^lib" ...) or equivalent anchored check) against that package-name variable rather than searching the whole URI; adjust the MESSAGE(FATAL_ERROR) to reference the package-name variable for clarity.cmake_tests/BA_PACKAGE/git_path_template_args/CMakeLists.txt (1)
26-32: Strengthen assertions to verify exact values, not just truthiness.
MOCK_CMLIB_DEPENDENCY_GIT_REVISIONshould equal"main"(the value set viaBA_PACKAGE_VARS_SET(REVISION "main")), andMOCK_CMLIB_DEPENDENCY_GIT_PATHshould equal the mock-expanded template output ("package/mock_distro/0.0.0/mock_arch/mypkg/mypkg_v1.0.0.zip"fromMOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT). The current truthiness-only checks would not catch a regression that passes any non-empty wrong value.♻️ Proposed fix: add exact-value assertions
-IF(NOT MOCK_CMLIB_DEPENDENCY_GIT_REVISION) - MESSAGE(FATAL_ERROR "GIT_REVISION should be set when GIT_PATH_TEMPLATE is defined") -ENDIF() - -IF(NOT MOCK_CMLIB_DEPENDENCY_GIT_PATH) - MESSAGE(FATAL_ERROR "GIT_PATH should be set when GIT_PATH_TEMPLATE is defined") -ENDIF() +TEST_VAR_EQUALS_LITERAL(MOCK_CMLIB_DEPENDENCY_GIT_REVISION "main") +TEST_VAR_EQUALS_LITERAL(MOCK_CMLIB_DEPENDENCY_GIT_PATH "package/mock_distro/0.0.0/mock_arch/mypkg/mypkg_v1.0.0.zip")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/BA_PACKAGE/git_path_template_args/CMakeLists.txt` around lines 26 - 32, Replace the truthiness checks on MOCK_CMLIB_DEPENDENCY_GIT_REVISION and MOCK_CMLIB_DEPENDENCY_GIT_PATH with exact string comparisons: verify MOCK_CMLIB_DEPENDENCY_GIT_REVISION STREQUAL "main" (the value set via BA_PACKAGE_VARS_SET(REVISION "main")) and verify MOCK_CMLIB_DEPENDENCY_GIT_PATH STREQUAL "package/mock_distro/0.0.0/mock_arch/mypkg/mypkg_v1.0.0.zip" (the expected output from MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT); if either comparison fails, emit a MESSAGE(FATAL_ERROR) explaining the mismatch so the test fails on incorrect values rather than just empty/nonnull values.cmake_tests/mock/CMDEF_VARS.cmake (1)
15-29: Consider addingFORCEto match the stated "override" intent.The header comment says to include this file "to override the real CMDEF values," but
CACHE INTERNALwithoutFORCEis a no-op if those variables are already cached (e.g., if real CMDEF detection ran before this mock is included). AddingFORCEensures the mock values actually take effect.♻️ Suggested fix
SET(CMDEF_ARCHITECTURE "mock_arch" CACHE INTERNAL "Mock CPU architecture for testing" + FORCE ) SET(CMDEF_DISTRO_ID "mock_distro" CACHE INTERNAL "Mock Linux distribution ID for testing" + FORCE ) SET(CMDEF_DISTRO_VERSION_ID "0.0.0" CACHE INTERNAL "Mock distribution version for testing" + FORCE )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/mock/CMDEF_VARS.cmake` around lines 15 - 29, The mock CMDEF file intends to override real cached variables but uses SET(... CACHE INTERNAL ...) without FORCE, so the sets are ignored if the cache already exists; update the SET calls for CMDEF_ARCHITECTURE, CMDEF_DISTRO_ID, and CMDEF_DISTRO_VERSION_ID to include the FORCE option (i.e., SET(... CACHE INTERNAL "..." FORCE)) so the mock values reliably override any previously cached values when this file is included.helpers/BA_PACKAGE_PREREQ.cmake (1)
85-87: Warning fires unconditionally after theIF/ELSEblock, including in local-use paths.
BA_PACKAGE_GIT_ARCHIVE_PATH_TEMPLATEandBA_PACKAGE_HTTP_HEADERare only retrieved viaCMCONF_GETin theELSE()branch (lines 49–53). WhenBA_PACKAGE_LOCAL_USEis ON orENV{BA_PACKAGE_LOCAL_PATH}is set, these variables are never fetched from CMCONF but may still be defined from the caller's scope (CMake functions inherit parent-scope values). In that scenario the warning fires spuriously for a configuration that is not actually in use.Moving the check inside the
ELSE()branch (lines 71–77) makes intent explicit and avoids false positives:♻️ Suggested fix
ELSE() SET(template "${BA_PACKAGE_URI_TEMPLATE_REMOTE}") SET(revision "${BA_PACKAGE_URI_REVISION}") SET(git_archive_path "${BA_PACKAGE_GIT_ARCHIVE_PATH_TEMPLATE}") SET(http_header "${BA_PACKAGE_HTTP_HEADER}") SET(template_args_uri_escape "${BA_PACKAGE_TEMPLATE_ARGS_URI_ESCAPE}") + IF(BA_PACKAGE_GIT_ARCHIVE_PATH_TEMPLATE AND BA_PACKAGE_HTTP_HEADER) + MESSAGE(WARNING "BA_PACKAGE_HTTP_HEADER is defined together with BA_PACKAGE_GIT_ARCHIVE_PATH_TEMPLATE. ARCHIVE_PATH_TEMPLATE implies the URI type is GIT but HTTP_HEADERS are used only for raw HTTP Downloads!") + ENDIF() ENDIF() ENDIF() # unreachable, but for clarity... IF(revision AND (BA_PACKAGE_LOCAL_USE OR DEFINED ENV{BA_PACKAGE_LOCAL_PATH})) MESSAGE(WARNING "Revision is defined but local repository is used.") ENDIF() - - IF(BA_PACKAGE_GIT_ARCHIVE_PATH_TEMPLATE AND BA_PACKAGE_HTTP_HEADER) - MESSAGE(WARNING "BA_PACKAGE_HTTP_HEADER is defined together with BA_PACKAGE_GIT_ARCHIVE_PATH_TEMPLATE. ARCHIVE_PATH_TEMPLATE implies the URI type is GIT but HTTP_HEADERS are used only for raw HTTP Downloads!") - ENDIF()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helpers/BA_PACKAGE_PREREQ.cmake` around lines 85 - 87, The unconditional warning is firing even when local-use paths are active because BA_PACKAGE_GIT_ARCHIVE_PATH_TEMPLATE and BA_PACKAGE_HTTP_HEADER are only populated via CMCONF_GET in the ELSE branch; move the IF(BA_PACKAGE_GIT_ARCHIVE_PATH_TEMPLATE AND BA_PACKAGE_HTTP_HEADER) check into the same ELSE() branch where CMCONF_GET runs (the block guarded by BA_PACKAGE_LOCAL_USE and ENV{BA_PACKAGE_LOCAL_PATH}) so the warning is only evaluated when those variables are populated from config (retain the same MESSAGE(WARNING) text and ENDIF() pairing); reference the symbols BA_PACKAGE_GIT_ARCHIVE_PATH_TEMPLATE, BA_PACKAGE_HTTP_HEADER, BA_PACKAGE_LOCAL_USE, ENV{BA_PACKAGE_LOCAL_PATH}, and the CMCONF_GET usage when making the change.STORAGE.cmake (1)
14-21:BA_PACKAGE_VARS_SETcalls forREVISIONandURI_TEMPLATEare still gated onBA_PACKAGE_CMCONF_USE.When
BA_PACKAGE_CMCONF_USE=FALSE(e.g., testing),BA_PACKAGE_PREREQ_CMCONF_INITis never called and none of theBA_PACKAGE_VARS_SETcalls run, leavingREVISION,URI_TEMPLATE,GIT_PATH_TEMPLATE, andESCAPE_TEMPLATE_ARGSuninitialised. Any downstreamBA_PACKAGE_VARS_GETfor these keys will silently return empty, potentially breaking non-CMCONF usage paths.The
HTTP_HEADERhandling (lines 23–33) was correctly moved outside the guard; the same pattern should apply to the other variables. Based on learnings,BA_PACKAGE_VARS_SETcalls forREVISIONandURI_TEMPLATEshould be executed unconditionally, even whenBA_PACKAGE_CMCONF_USEis FALSE, as these variables serve a broader purpose beyond CMCONF.♻️ Suggested fix
IF(BA_PACKAGE_CMCONF_USE) BA_PACKAGE_PREREQ_CMCONF_INIT(template revision git_archive_path_template http_header template_args_uri_escape) - BA_PACKAGE_VARS_SET(REVISION "${revision}") - BA_PACKAGE_VARS_SET(URI_TEMPLATE "${template}") - BA_PACKAGE_VARS_SET(GIT_PATH_TEMPLATE "${git_archive_path_template}") - BA_PACKAGE_VARS_SET(ESCAPE_TEMPLATE_ARGS "${template_args_uri_escape}") - BA_PACKAGE_VARS_SET(HTTP_HEADER "${http_header}") ENDIF() + +BA_PACKAGE_VARS_SET(REVISION "${revision}") +BA_PACKAGE_VARS_SET(URI_TEMPLATE "${template}") +BA_PACKAGE_VARS_SET(GIT_PATH_TEMPLATE "${git_archive_path_template}") +BA_PACKAGE_VARS_SET(ESCAPE_TEMPLATE_ARGS "${template_args_uri_escape}") +BA_PACKAGE_VARS_SET(HTTP_HEADER "${http_header}")Based on learnings: "BA_PACKAGE_VARS_SET calls for REVISION and URI_TEMPLATE should be executed unconditionally, even when BA_PACKAGE_CMCONF_USE is FALSE. These variables do not relate specifically to CMCONF and serve a broader purpose in the package system."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@STORAGE.cmake` around lines 14 - 21, The BA_PACKAGE_VARS_SET calls for REVISION and URI_TEMPLATE are currently inside the BA_PACKAGE_CMCONF_USE guard and thus not set when BA_PACKAGE_CMCONF_USE=FALSE; move the BA_PACKAGE_VARS_SET(REVISION "${revision}") and BA_PACKAGE_VARS_SET(URI_TEMPLATE "${template}") calls out of the IF(BA_PACKAGE_CMCONF_USE) block so they run unconditionally, keep BA_PACKAGE_PREREQ_CMCONF_INIT(...), BA_PACKAGE_VARS_SET(GIT_PATH_TEMPLATE ...), and BA_PACKAGE_VARS_SET(ESCAPE_TEMPLATE_ARGS ...) inside the guard (since they depend on CMCONF initialization), and leave HTTP_HEADER handling as-is.BA_PACKAGE.cmake (1)
252-273:_BA_PACKAGE_URL_ENCODEis missing several RFC 3986 unsafe characters.The function covers the most common reserved/unsafe characters but omits:
[(%5B),](%5D),<(%3C),>(%3E),{(%7B),}(%7D),|(%7C),\(%5C),^(%5E),`(%60). For the expected inputs (distro IDs, version strings, machine names, package names, git revisions), these characters are unlikely to appear, so this has no practical impact today. Consider adding the missing replacements for completeness:♻️ Proposed additions
STRING(REPLACE "@" "%40" result "${result}") + STRING(REPLACE "[" "%5B" result "${result}") + STRING(REPLACE "]" "%5D" result "${result}") + STRING(REPLACE "<" "%3C" result "${result}") + STRING(REPLACE ">" "%3E" result "${result}") + STRING(REPLACE "{" "%7B" result "${result}") + STRING(REPLACE "}" "%7D" result "${result}") + STRING(REPLACE "|" "%7C" result "${result}") + STRING(REPLACE "\\" "%5C" result "${result}") + STRING(REPLACE "^" "%5E" result "${result}") SET(${output} "${result}" PARENT_SCOPE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BA_PACKAGE.cmake` around lines 252 - 273, The _BA_PACKAGE_URL_ENCODE function omits several RFC 3986 unsafe characters; update that function to add STRING(REPLACE ...) calls for each missing character so encoded output is complete—specifically add replacements in _BA_PACKAGE_URL_ENCODE for "[" -> "%5B", "]" -> "%5D", "<" -> "%3C", ">" -> "%3E", "{" -> "%7B", "}" -> "%7D", "|" -> "%7C", "\" -> "%5C", "^" -> "%5E", and "`" -> "%60", ensuring each uses the same result variable and ends by setting the output via SET(${output} "${result}" PARENT_SCOPE) as existing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BA_PACKAGE_VARS.cmake`:
- Around line 30-33: Fix the typo in the cache description for the
BA_PACKAGE_VARS__GIT_PATH_TEMPLATE variable: update the string in the
SET(BA_PACKAGE_VARS__GIT_PATH_TEMPLATE ...) call to change "URI_TEMPALTE" to
"URI_TEMPLATE" so the documentation text reads correctly.
In `@BA_PACKAGE.cmake`:
- Line 24: Replace the misspelled comment text "arounf" with "around" in
BA_PACKAGE.cmake; search for the exact token "arounf" (appears twice) and update
both occurrences so the comment reads "Wrapper around BA_PACKAGE".
- Around line 164-169: The URL-encoding block mutates package_name and
package_string in-place which breaks the later derivation of package_name_upper
used for BACPACK cache keywords and shows URL-encoded paths in FATAL_ERROR;
before the IF(escape_template_args) encoding, save the originals into new
variables (e.g. package_name_for_cache and package_string_for_message), then
perform the _BA_PACKAGE_URL_ENCODE on the originals as now, and update
downstream uses: compute package_name_upper from package_name_for_cache (and use
that for BACPACK/CACHE_ONLY logic) and use package_string_for_message in the
FATAL_ERROR message so user-facing and cache-key derivations use the unescaped
values.
In `@cmake_tests/BA_PACKAGE_EXECUTABLE/release_basic/CMakeLists.txt`:
- Line 14: The assertions are validating MOCK_CMLIB_DEPENDENCY_URI (which is
hardcoded) instead of the actual archive name; update the three STRING(FIND ...)
checks to operate on MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME so they
verify what BA_PACKAGE_EXECUTABLE constructed, i.e., assert that
MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME contains "mypkg_v1.0.0" and
does NOT contain "lib" or "-dev" while keeping the same search logic and
expected results.
In `@cmake_tests/BA_PACKAGE_LIBRARY/debug_suffix/CMakeLists.txt`:
- Line 14: Replace the tautological assertion that compares
MOCK_CMLIB_DEPENDENCY_URI (which echoes
MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT) with a check against the captured
input name MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME so the test
verifies the archive name produced by BA_PACKAGE_LIBRARY →
CMLIB_STORAGE_TEMPLATE_INSTANCE before the mock returns the URI; update the
assertion to look for the expected "libmypkgd-dev" substring in
MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME (not the output URI), and fix
the FATAL_ERROR text to mention the exact search string ("libmypkgd-dev") so the
failure message matches the actual check.
In `@cmake_tests/BA_PACKAGE_LIBRARY/no_debug_skips_suffix/CMakeLists.txt`:
- Around line 26-28: The error message in the STRING(FIND ...) check is
misleading: the code searches MOCK_CMLIB_DEPENDENCY_URI for the substring
"libmypkgd-dev" via STRING(FIND ...) but the MESSAGE(FATAL_ERROR ...) text
refers to a "'d-dev' suffix" which is ambiguous; update the MESSAGE(FATAL_ERROR)
text to explicitly mention the exact substring being prohibited (e.g., "URI
should NOT contain 'libmypkgd-dev' when NO_DEBUG=ON:
${MOCK_CMLIB_DEPENDENCY_URI}") so the message matches the condition checked by
STRING(FIND ...) and references MOCK_CMLIB_DEPENDENCY_URI and the offending
substring.
In `@cmake_tests/BA_PACKAGE/package_name_dash_warning/CMakeLists.txt`:
- Around line 1-6: Update the test comment or assertions to match what the test
actually verifies: either add an assertion checking the mock warning flag (e.g.,
add TEST_VAR_TRUE(MOCK_MESSAGE_WARNING_CALLED) alongside
TEST_VAR_TRUE(MOCK_CMLIB_DEPENDENCY_CALLED) to verify a WARNING was emitted) or
change the top comment text to "Verifies that BA_PACKAGE with a dash-containing
name does not fatal-error and still invokes CMLIB_DEPENDENCY" so it no longer
claims it verifies a WARNING; reference the existing
TEST_VAR_TRUE(MOCK_CMLIB_DEPENDENCY_CALLED) and the mock warning symbol
MOCK_MESSAGE_WARNING_CALLED when making the edit.
In `@cmake_tests/README.md`:
- Around line 12-14: The fenced code block in cmake_tests/README.md is missing a
language specifier (MD040); update the opening fence for the block containing
the command "cmake -P cmake_tests/CMakeLists.txt" from ``` to ```sh so the block
is marked as shell script and satisfies the linter.
In `@README.md`:
- Around line 90-92: Update the README sentence "it must ensured that" to
include the missing auxiliary verb by changing it to "it must be ensured that"
so the line reads: "If the same cache path is used for Release and Debug build
types it must be ensured that the package differs between Debug/Release build
configs and does not have files with the same paths."
- Line 65: Update the README sentence for GIT_PATH_TEMPLATE to include the
missing article: change "It takes same template params as `URI_TEMPLATE`" to "It
takes the same template params as `URI_TEMPLATE`", referencing the
`GIT_PATH_TEMPLATE` and `URI_TEMPLATE` identifiers in the updated line.
In `@test/app/CMakeLists.txt`:
- Around line 115-116: The call to TEST_POST_TARGET_SETUP currently passes the
literal token "TARGET" as the first positional argument so any
FUNCTION(TEST_POST_TARGET_SETUP target) implementation will receive "TARGET"
instead of the actual target name; fix this by making the call match the
function signature—use positional style TEST_POST_TARGET_SETUP(${PROJECT_NAME})
so the declared parameter target receives the project name, or alternatively
update the function signature/impl to accept a keyword form (parse a TARGET
keyword from ARGN) if you prefer to keep the existing call style; update the
corresponding stub implementations for TEST_POST_TARGET_SETUP to match whichever
convention you choose.
In `@test/app/lib/shared_library_gst/CMakeLists.txt`:
- Around line 25-30: The OUTPUT_NAME currently hardcodes "-1.0" for the
${PROJECT_NAME}-shared target which can diverge from ${LIBRARY_VERSION}; update
CMake to compute the desired suffix from LIBRARY_VERSION (e.g., extract the
major version or entire version string via CMake string(REGEX REPLACE ...) or
string(REPLACE ...) into a variable like LIB_OUTPUT_SUFFIX) and then set
OUTPUT_NAME to "${PROJECT_NAME}-${LIB_OUTPUT_SUFFIX}" for the
${PROJECT_NAME}-shared target so the filename matches LIBRARY_VERSION
consistently.
---
Outside diff comments:
In `@helpers/BA_PACKAGE_PREREQ.cmake`:
- Around line 27-40: Doc block for FUNCTION BA_PACKAGE_PREREQ_CMCONF_INIT is
missing documentation for the three new output parameters; update the comment
above the function to list and describe git_archive_path_template_output_var,
http_header_output_var, and template_args_uri_escape_output_var (what they
return, scope=parent, and format/expected values), matching the style used for
template_output_var and revision_output_var so callers understand the additional
outputs.
---
Nitpick comments:
In `@BA_PACKAGE.cmake`:
- Around line 252-273: The _BA_PACKAGE_URL_ENCODE function omits several RFC
3986 unsafe characters; update that function to add STRING(REPLACE ...) calls
for each missing character so encoded output is complete—specifically add
replacements in _BA_PACKAGE_URL_ENCODE for "[" -> "%5B", "]" -> "%5D", "<" ->
"%3C", ">" -> "%3E", "{" -> "%7B", "}" -> "%7D", "|" -> "%7C", "\" -> "%5C", "^"
-> "%5E", and "`" -> "%60", ensuring each uses the same result variable and ends
by setting the output via SET(${output} "${result}" PARENT_SCOPE) as existing.
In `@cmake_tests/BA_PACKAGE_EXECUTABLE/CMakeLists.txt`:
- Around line 10-13: The test list for BA_PACKAGE_EXECUTABLE is missing the
error/warning registrations referenced in the AI summary; add TEST_RUN for the
warning case and TEST_INVALID_CMAKE_RUN entries for the two error cases or
explicitly document why they're omitted. Specifically, add a test registration
for package_name_dash_warning with ALWAYS_SCRIPT_MODE ON (or the equivalent
TEST_RUN invocation that enables ALWAYS_SCRIPT_MODE), and add
TEST_INVALID_CMAKE_RUN registrations named invalid_package_name_error and
cache_only_not_found_error so the executable wrapper’s error paths are exercised
(use the same TEST_RUN/TEST_INVALID_CMAKE_RUN helpers used elsewhere); if these
cases are intentionally covered elsewhere, insert a short comment above the
TEST_RUN list clarifying that omission.
In `@cmake_tests/BA_PACKAGE_EXECUTABLE/debug_suffix/CMakeLists.txt`:
- Around line 32-35: The current STRING(FIND check against
MOCK_CMLIB_DEPENDENCY_URI is too broad; instead extract the package-name segment
from the URI and validate that that segment does not start with "lib". Update
the check to first isolate the last path component (e.g., via
get_filename_component or splitting on "/" into components) and then perform an
anchored test (STRING(REGEX MATCH "^lib" ...) or equivalent anchored check)
against that package-name variable rather than searching the whole URI; adjust
the MESSAGE(FATAL_ERROR) to reference the package-name variable for clarity.
In `@cmake_tests/BA_PACKAGE_LIBRARY/CMakeLists.txt`:
- Around line 10-13: The BA_PACKAGE_LIBRARY suite currently only registers four
tests (TEST_RUN entries for debug_suffix, no_debug_skips_suffix,
output_path_var_propagates, cmake_prefix_path_updated) but the review expects
additional scenarios; either add TEST_RUN entries for the missing tests
referenced in the summary (release_basic, debug_keyword or debug_suffix_applied,
no_git_args_without_template, cache_only_skips_dependency,
escape_template_args_applied, escape_template_args_not_applied,
package_name_dash_warning, invalid_package_name_error,
cache_only_not_found_error) to this CMakeLists, or add a clear comment in the
BA_PACKAGE_LIBRARY suite explaining that those scenarios are intentionally
delegated to BA_PACKAGE; also reconcile the name mismatch for
debug_suffix_applied vs debug_suffix so the test name matches the summary or
vice versa.
In
`@cmake_tests/BA_PACKAGE_PREREQ_CMCONF_INIT/env_var_with_local_use_warning/CMakeLists.txt`:
- Around line 1-29: The test currently asserts only output variables but claims
to verify a MESSAGE(WARNING) when both TEST_CMCONF_BA_PACKAGE_LOCAL_USE /
BA_PACKAGE_LOCAL_USE and ENV{BA_PACKAGE_LOCAL_PATH} are set; either document
that the warning cannot be programmatically asserted here by adding a top-file
comment clarifying the limitation (reference: BA_PACKAGE_PREREQ_CMCONF_INIT
invocation, TEST_CMCONF_BA_PACKAGE_LOCAL_USE, and ENV{BA_PACKAGE_LOCAL_PATH}),
or update the test harness to capture/stub CMake output and assert the warning
text (use the test runner’s PASS_REGULAR_EXPRESSION or stderr-capture mechanism
to match the expected MESSAGE(WARNING) string emitted by
BA_PACKAGE_PREREQ_CMCONF_INIT).
In `@cmake_tests/BA_PACKAGE/debug_suffix_applied/CMakeLists.txt`:
- Around line 1-10: Rename the test to reflect that it verifies generic
prefix/suffix expansion rather than a debug-only suffix: change the directory
name from "debug_suffix_applied" to "prefix_suffix_applied" and update the
project identifier in CMakeLists.txt from
PROJECT(TEST_BA_PACKAGE_DEBUG_SUFFIX_APPLIED) to
PROJECT(TEST_BA_PACKAGE_PREFIX_SUFFIX_APPLIED), and optionally adjust any test
labels or metadata that reference the old name; leave the CMAKE_BUILD_TYPE
"Release" and the arbitrary "myprefix"/"mysuffix" values as-is since the test is
not debug-specific.
In `@cmake_tests/BA_PACKAGE/git_path_template_args/CMakeLists.txt`:
- Around line 26-32: Replace the truthiness checks on
MOCK_CMLIB_DEPENDENCY_GIT_REVISION and MOCK_CMLIB_DEPENDENCY_GIT_PATH with exact
string comparisons: verify MOCK_CMLIB_DEPENDENCY_GIT_REVISION STREQUAL "main"
(the value set via BA_PACKAGE_VARS_SET(REVISION "main")) and verify
MOCK_CMLIB_DEPENDENCY_GIT_PATH STREQUAL
"package/mock_distro/0.0.0/mock_arch/mypkg/mypkg_v1.0.0.zip" (the expected
output from MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT); if either comparison
fails, emit a MESSAGE(FATAL_ERROR) explaining the mismatch so the test fails on
incorrect values rather than just empty/nonnull values.
In `@cmake_tests/BA_PACKAGE/release_basic/CMakeLists.txt`:
- Around line 26-37: Add a negative assertion that the DEBUG keyword is not
present in the MOCK_CMLIB_DEPENDENCY_KEYWORDS for this Release test: use
LIST(FIND MOCK_CMLIB_DEPENDENCY_KEYWORDS "DEBUG" idx_debug) and then fail with
MESSAGE(FATAL_ERROR "...") if idx_debug is NOT EQUAL -1, similar to the existing
checks for BACPACK/MYPKG/RELEASE (place the check alongside the other
LIST(FIND)/IF blocks referencing MOCK_CMLIB_DEPENDENCY_KEYWORDS and idx_*).
In `@cmake_tests/mock/CMDEF_VARS.cmake`:
- Around line 15-29: The mock CMDEF file intends to override real cached
variables but uses SET(... CACHE INTERNAL ...) without FORCE, so the sets are
ignored if the cache already exists; update the SET calls for
CMDEF_ARCHITECTURE, CMDEF_DISTRO_ID, and CMDEF_DISTRO_VERSION_ID to include the
FORCE option (i.e., SET(... CACHE INTERNAL "..." FORCE)) so the mock values
reliably override any previously cached values when this file is included.
In `@cmake_tests/mock/CMLIB_DEPENDENCY.cmake`:
- Line 74: The mock currently hardcodes the return path by setting
${__OUTPUT_PATH_VAR} to "/mock/cache/path"; change this to read a configurable
variable so tests can override it. Add a top-level default/initialization for a
new config variable (e.g., CMLIB_DEPENDENCY_MOCK_OUTPUT or
MOCK_DEPENDENCY_OUTPUT) and in the function that sets ${__OUTPUT_PATH_VAR}
replace the hardcoded string with that config variable (falling back to
"/mock/cache/path" if not defined) so callers can set the mock return path
without editing the mock file.
In `@cmake_tests/mock/CMLIB_STORAGE.cmake`:
- Around line 76-87: The IF guards around the captures for __ARCHIVE_NAME,
__GIT_PATH, __REVISION and __PACKAGE_GROUP_NAME make an explicitly passed empty
string indistinguishable from "not passed"; change the code so each
SET_PROPERTY(CACHE MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_... PROPERTY VALUE
"${__...}") is executed unconditionally (remove the IF/ENDIF wrappers) so empty
values overwrite the cached variables, mirroring the unconditional captures used
in CMLIB_DEPENDENCY.cmake.
In `@cmake_tests/mock/CMUTIL.cmake`:
- Around line 26-29: MOCK_CMUTIL_PLATFORM_STRING is set directly with SET(...
CACHE INTERNAL ...) unlike sibling mocks; change the initialization in
CMUTIL.cmake to use the same guarded pattern as CMLIB_CACHE.cmake and
CMLIB_STORAGE.cmake by wrapping the cache variable assignment in an IF(NOT
DEFINED MOCK_CMUTIL_PLATFORM_STRING) guard so the configurable return-value
variable is only defined when absent, matching the sibling-mock style and
avoiding future confusion.
In `@cmake_tests/TEST.cmake`:
- Line 7: Guard the INCLUDE call so CMake only tries to include the optional
test/TEST.cmake if it actually exists: check IF(EXISTS "<path>") using the same
variable CMLIB_DIR and the literal test/TEST.cmake before calling
INCLUDE("${CMLIB_DIR}/test/TEST.cmake"), and optionally emit a
message(NOTICE/STATUS) when the file is skipped to make the behavior explicit;
update the invocation around the INCLUDE(...) line and any related logic that
assumes that file was loaded.
In `@helpers/BA_PACKAGE_PREREQ.cmake`:
- Around line 85-87: The unconditional warning is firing even when local-use
paths are active because BA_PACKAGE_GIT_ARCHIVE_PATH_TEMPLATE and
BA_PACKAGE_HTTP_HEADER are only populated via CMCONF_GET in the ELSE branch;
move the IF(BA_PACKAGE_GIT_ARCHIVE_PATH_TEMPLATE AND BA_PACKAGE_HTTP_HEADER)
check into the same ELSE() branch where CMCONF_GET runs (the block guarded by
BA_PACKAGE_LOCAL_USE and ENV{BA_PACKAGE_LOCAL_PATH}) so the warning is only
evaluated when those variables are populated from config (retain the same
MESSAGE(WARNING) text and ENDIF() pairing); reference the symbols
BA_PACKAGE_GIT_ARCHIVE_PATH_TEMPLATE, BA_PACKAGE_HTTP_HEADER,
BA_PACKAGE_LOCAL_USE, ENV{BA_PACKAGE_LOCAL_PATH}, and the CMCONF_GET usage when
making the change.
In `@STORAGE.cmake`:
- Around line 14-21: The BA_PACKAGE_VARS_SET calls for REVISION and URI_TEMPLATE
are currently inside the BA_PACKAGE_CMCONF_USE guard and thus not set when
BA_PACKAGE_CMCONF_USE=FALSE; move the BA_PACKAGE_VARS_SET(REVISION
"${revision}") and BA_PACKAGE_VARS_SET(URI_TEMPLATE "${template}") calls out of
the IF(BA_PACKAGE_CMCONF_USE) block so they run unconditionally, keep
BA_PACKAGE_PREREQ_CMCONF_INIT(...), BA_PACKAGE_VARS_SET(GIT_PATH_TEMPLATE ...),
and BA_PACKAGE_VARS_SET(ESCAPE_TEMPLATE_ARGS ...) inside the guard (since they
depend on CMCONF initialization), and leave HTTP_HEADER handling as-is.
In `@test/app/tests_list/gstreamer_style_naming.txt`:
- Around line 32-52: The directory tree comment lists files not installed by
TEST_PRERUN (only shared_library_gst is), which is confusing; update the comment
in test/app/tests_list/gstreamer_style_naming.txt to clarify origin by adding a
short note above the non-gst entries (e.g. "# (shared base dependencies added by
test framework)") and/or explicitly mark which entries are installed by
TEST_PRERUN (mention shared_library_gst) so readers understand that
shared-library-shared, shared-library-for-prerun and shared_out_of_dir_symlink.h
come from the shared base fixture.
| SET(BA_PACKAGE_VARS__GIT_PATH_TEMPLATE "NonExistentPath" | ||
| CACHE INTERNAL | ||
| "Git path template to use. If non empty the URI_TEMPALTE represents remote Git repository. Takes same template params as URI_TEMPLATE." | ||
| ) |
There was a problem hiding this comment.
Typo in cache description: URI_TEMPALTE → URI_TEMPLATE.
🔤 Proposed fix
- "Git path template to use. If non empty the URI_TEMPALTE represents remote Git repository. Takes same template params as URI_TEMPLATE."
+ "Git path template to use. If non empty the URI_TEMPLATE represents remote Git repository. Takes same template params as URI_TEMPLATE."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BA_PACKAGE_VARS.cmake` around lines 30 - 33, Fix the typo in the cache
description for the BA_PACKAGE_VARS__GIT_PATH_TEMPLATE variable: update the
string in the SET(BA_PACKAGE_VARS__GIT_PATH_TEMPLATE ...) call to change
"URI_TEMPALTE" to "URI_TEMPLATE" so the documentation text reads correctly.
| # | ||
| # OUTPUT_PATH_VAR - name of the variable where the absolute path of the package root will be stored. | ||
| # | ||
| # Wrapper arounf BA_PACKAGE |
There was a problem hiding this comment.
Fix typo: "arounf" → "around" (appears on both Line 24 and Line 82).
📝 Proposed fix
-# Wrapper arounf BA_PACKAGE
+# Wrapper around BA_PACKAGEAlso applies to: 82-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BA_PACKAGE.cmake` at line 24, Replace the misspelled comment text "arounf"
with "around" in BA_PACKAGE.cmake; search for the exact token "arounf" (appears
twice) and update both occurrences so the comment reads "Wrapper around
BA_PACKAGE".
| IF(escape_template_args) | ||
| _BA_PACKAGE_URL_ENCODE("${revision_var}" revision_var) | ||
| _BA_PACKAGE_URL_ENCODE("${git_path}" git_path) | ||
| _BA_PACKAGE_URL_ENCODE("${package_string}" package_string) | ||
| _BA_PACKAGE_URL_ENCODE("${package_name}" package_name) | ||
| ENDIF() |
There was a problem hiding this comment.
URL-encoding package_name in-place corrupts the cache keyword derived from it.
After line 168 encodes package_name, it is reused at Line 198 to derive package_name_upper for the BACPACK cache keywords. If the name contains any character from the encoded set (e.g. / → %2F), STRING(REGEX REPLACE "[^A-Z0-9]" "") strips % but keeps 2F, producing a different (incorrect) cache key. A CACHE_ONLY call with the same name would compute the same mutated key and still hit, but a non-CACHE_ONLY call would register under MY2FPACKAGE while users might expect MYPACKAGE.
Similarly, package_string (line 167) is used in the FATAL_ERROR message at Line 224, showing an URL-encoded path to the user when ESCAPE_TEMPLATE_ARGS is active.
Fix: preserve originals for non-URL purposes before the encoding block.
🛠️ Proposed fix
BA_PACKAGE_VARS_GET(ESCAPE_TEMPLATE_ARGS escape_template_args)
+ SET(package_name_for_cache "${package_name}")
+ SET(package_string_for_message "${package_string}")
IF(escape_template_args)
_BA_PACKAGE_URL_ENCODE("${revision_var}" revision_var)
_BA_PACKAGE_URL_ENCODE("${git_path}" git_path)
_BA_PACKAGE_URL_ENCODE("${package_string}" package_string)
_BA_PACKAGE_URL_ENCODE("${package_name}" package_name)
ENDIF()Then use package_name_for_cache on Line 198 and package_string_for_message on Line 224:
- STRING(TOUPPER "${package_name}" package_name_upper_orig)
+ STRING(TOUPPER "${package_name_for_cache}" package_name_upper_orig)- MESSAGE(FATAL_ERROR "Package not found: ${package_string}")
+ MESSAGE(FATAL_ERROR "Package not found: ${package_string_for_message}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BA_PACKAGE.cmake` around lines 164 - 169, The URL-encoding block mutates
package_name and package_string in-place which breaks the later derivation of
package_name_upper used for BACPACK cache keywords and shows URL-encoded paths
in FATAL_ERROR; before the IF(escape_template_args) encoding, save the originals
into new variables (e.g. package_name_for_cache and package_string_for_message),
then perform the _BA_PACKAGE_URL_ENCODE on the originals as now, and update
downstream uses: compute package_name_upper from package_name_for_cache (and use
that for BACPACK/CACHE_ONLY logic) and use package_string_for_message in the
FATAL_ERROR message so user-facing and cache-key derivations use the unescaped
values.
|
|
||
| FIND_PACKAGE(CMLIB REQUIRED) | ||
|
|
||
| SET(MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT "https://example.com/main/mock_distro/0.0.0/mock_arch/mypkg/mypkg_v1.0.0_mock_arch_mock_distro_0.0.0.zip" CACHE INTERNAL "") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the relevant cmake files in the test directory
fd -type f -name "*.cmake" | grep -i "cmlib\|storage\|dependency" | head -20Repository: bacpack-system/package-tracker
Length of output: 244
🏁 Script executed:
#!/bin/bash
# Find the test files mentioned in the review
find . -type f -path "*/BA_PACKAGE_EXECUTABLE/*" -name "CMakeLists.txt" | sortRepository: bacpack-system/package-tracker
Length of output: 408
🏁 Script executed:
#!/bin/bash
# Read the actual test file mentioned in the review
cat -n cmake_tests/BA_PACKAGE_EXECUTABLE/release_basic/CMakeLists.txt | head -50Repository: bacpack-system/package-tracker
Length of output: 1953
🏁 Script executed:
#!/bin/bash
# Search for MOCK_CMLIB_DEPENDENCY_URI to understand where it comes from
rg "MOCK_CMLIB_DEPENDENCY_URI" -A 2 -B 2 --max-count 20Repository: bacpack-system/package-tracker
Length of output: 6923
🏁 Script executed:
#!/bin/bash
# Find BA_PACKAGE_EXECUTABLE definition
rg "FUNCTION\(BA_PACKAGE_EXECUTABLE\|MACRO\(BA_PACKAGE_EXECUTABLE" -A 20 --max-count 5Repository: bacpack-system/package-tracker
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Search for how CMLIB_DEPENDENCY is called
rg "CMLIB_DEPENDENCY\(" -B 3 -A 3 | head -50Repository: bacpack-system/package-tracker
Length of output: 386
🏁 Script executed:
#!/bin/bash
# Look for MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME usage
rg "MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME"Repository: bacpack-system/package-tracker
Length of output: 1061
🏁 Script executed:
#!/bin/bash
# Find the debug_suffix_applied test mentioned in review
find . -path "*/debug_suffix_applied/*" -o -path "*/debug_suffix/*" | grep CMakeListsRepository: bacpack-system/package-tracker
Length of output: 259
🏁 Script executed:
#!/bin/bash
# Get the full context of CMLIB_DEPENDENCY call
rg "CMLIB_DEPENDENCY\(" -B 5 -A 15 | head -80Repository: bacpack-system/package-tracker
Length of output: 953
🏁 Script executed:
#!/bin/bash
# Check the debug_suffix_applied test to see how it verifies correctly
cat -n cmake_tests/BA_PACKAGE/debug_suffix_applied/CMakeLists.txt | head -60Repository: bacpack-system/package-tracker
Length of output: 1317
🏁 Script executed:
#!/bin/bash
# Look for BA_PACKAGE_EXECUTABLE or BA_PACKAGE_LIBRARY implementation
fd -name "BA_PACKAGE*.cmake" | xargs ls -laRepository: bacpack-system/package-tracker
Length of output: 1379
🏁 Script executed:
#!/bin/bash
# Check how URI is constructed - look for URI variable setting in BA_PACKAGE context
rg "URI.*=" cmake_tests/BA_PACKAGE_EXECUTABLE/release_basic/CMakeLists.txt -B 2 -A 2Repository: bacpack-system/package-tracker
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Find BA_PACKAGE implementation and related files
cat -n BA_PACKAGE.cmake | head -100Repository: bacpack-system/package-tracker
Length of output: 3319
🏁 Script executed:
#!/bin/bash
# Find BA_PACKAGE_EXECUTABLE in BA_PACKAGE.cmake
rg "FUNCTION\(BA_PACKAGE_EXECUTABLE\|^BA_PACKAGE_EXECUTABLE" -A 30Repository: bacpack-system/package-tracker
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Search for where remote_file is set before CMLIB_DEPENDENCY call
cat -n BA_PACKAGE.cmake | grep -B 20 "CMLIB_DEPENDENCY" | head -40Repository: bacpack-system/package-tracker
Length of output: 802
🏁 Script executed:
#!/bin/bash
# Get more of BA_PACKAGE.cmake around the CMLIB_DEPENDENCY call
cat -n BA_PACKAGE.cmake | sed -n '190,250p'Repository: bacpack-system/package-tracker
Length of output: 2456
🏁 Script executed:
#!/bin/bash
# Look for remote_file construction
rg "remote_file\s*=" BA_PACKAGE.cmake -B 3 -A 1Repository: bacpack-system/package-tracker
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Check how CMLIB_STORAGE is being mocked and what it returns
cat -n cmake_tests/mock/CMLIB_STORAGE.cmake | head -80Repository: bacpack-system/package-tracker
Length of output: 3630
🏁 Script executed:
#!/bin/bash
# Look at more of CMLIB_STORAGE.cmake to see what it returns
cat -n cmake_tests/mock/CMLIB_STORAGE.cmake | sed -n '80,150p'Repository: bacpack-system/package-tracker
Length of output: 875
🏁 Script executed:
#!/bin/bash
# Now let's trace the full flow - find where remote_file gets its value
cat -n BA_PACKAGE.cmake | sed -n '165,195p'Repository: bacpack-system/package-tracker
Length of output: 1457
🏁 Script executed:
#!/bin/bash
# Check if there's a default mock URI set in CMLIB_STORAGE
rg "default.*mock.*url\|example.com" cmake_tests/mock/CMLIB_STORAGE.cmake -iRepository: bacpack-system/package-tracker
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Find BA_PACKAGE_EXECUTABLE implementation
cat -n BA_PACKAGE.cmake | sed -n '95,130p'Repository: bacpack-system/package-tracker
Length of output: 1154
🏁 Script executed:
#!/bin/bash
# Check what prefix and suffix BA_PACKAGE_EXECUTABLE passes to BA_PACKAGE
rg "BA_PACKAGE_EXECUTABLE" -A 20 BA_PACKAGE.cmakeRepository: bacpack-system/package-tracker
Length of output: 1195
🏁 Script executed:
#!/bin/bash
# Compare with debug_suffix test for executables to confirm the pattern
cat -n cmake_tests/BA_PACKAGE_EXECUTABLE/debug_suffix/CMakeLists.txtRepository: bacpack-system/package-tracker
Length of output: 1980
Assertions should check MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME to verify actual behavior.
The three STRING(FIND ...) checks operate on MOCK_CMLIB_DEPENDENCY_URI, which is set to the hardcoded MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT (line 14). This URI is pre-configured to satisfy all three conditions (mypkg_v1.0.0 present, lib absent, -dev absent), so these assertions pass regardless of what BA_PACKAGE_EXECUTABLE actually constructs.
To verify that BA_PACKAGE_EXECUTABLE correctly omits the lib prefix and -dev suffix, inspect MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME — the archive name argument passed into the storage template function. This captures what BA_PACKAGE_EXECUTABLE actually constructed, as shown correctly in the debug_suffix_applied test pattern.
♻️ Proposed fix
-STRING(FIND "${MOCK_CMLIB_DEPENDENCY_URI}" "mypkg_v1.0.0" found_pkg)
-IF(found_pkg EQUAL -1)
- MESSAGE(FATAL_ERROR "URI does not contain 'mypkg_v1.0.0': ${MOCK_CMLIB_DEPENDENCY_URI}")
-ENDIF()
-
-STRING(FIND "${MOCK_CMLIB_DEPENDENCY_URI}" "lib" found_lib)
-IF(NOT found_lib EQUAL -1)
- MESSAGE(FATAL_ERROR "URI should not contain 'lib' prefix: ${MOCK_CMLIB_DEPENDENCY_URI}")
-ENDIF()
-
-STRING(FIND "${MOCK_CMLIB_DEPENDENCY_URI}" "-dev" found_dev)
-IF(NOT found_dev EQUAL -1)
- MESSAGE(FATAL_ERROR "URI should not contain '-dev' suffix: ${MOCK_CMLIB_DEPENDENCY_URI}")
-ENDIF()
+STRING(FIND "${MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME}" "mypkg_v1.0.0" found_pkg)
+IF(found_pkg EQUAL -1)
+ MESSAGE(FATAL_ERROR "ARCHIVE_NAME should contain 'mypkg_v1.0.0': ${MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME}")
+ENDIF()
+
+STRING(FIND "${MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME}" "lib" found_lib)
+IF(NOT found_lib EQUAL -1)
+ MESSAGE(FATAL_ERROR "ARCHIVE_NAME should not contain 'lib' prefix: ${MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME}")
+ENDIF()
+
+STRING(FIND "${MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME}" "-dev" found_dev)
+IF(NOT found_dev EQUAL -1)
+ MESSAGE(FATAL_ERROR "ARCHIVE_NAME should not contain '-dev' suffix: ${MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME}")
+ENDIF()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmake_tests/BA_PACKAGE_EXECUTABLE/release_basic/CMakeLists.txt` at line 14,
The assertions are validating MOCK_CMLIB_DEPENDENCY_URI (which is hardcoded)
instead of the actual archive name; update the three STRING(FIND ...) checks to
operate on MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME so they verify what
BA_PACKAGE_EXECUTABLE constructed, i.e., assert that
MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME contains "mypkg_v1.0.0" and
does NOT contain "lib" or "-dev" while keeping the same search logic and
expected results.
|
|
||
| FIND_PACKAGE(CMLIB REQUIRED) | ||
|
|
||
| SET(MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT "https://example.com/main/mock_distro/0.0.0/mock_arch/mypkg/libmypkgd-dev_v1.0.0_mock_arch_mock_distro_0.0.0.zip" CACHE INTERNAL "") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "CMakeLists.txt" -path "*debug_suffix*" | head -20Repository: bacpack-system/package-tracker
Length of output: 259
🏁 Script executed:
find . -name "CMLIB_DEPENDENCY.cmake" -o -name "CMLIB_STORAGE.cmake" | head -20Repository: bacpack-system/package-tracker
Length of output: 154
🏁 Script executed:
find . -name "debug_suffix" -type dRepository: bacpack-system/package-tracker
Length of output: 168
🏁 Script executed:
cat -n ./cmake_tests/BA_PACKAGE_LIBRARY/debug_suffix/CMakeLists.txtRepository: bacpack-system/package-tracker
Length of output: 1420
🏁 Script executed:
cat -n ./cmake_tests/BA_PACKAGE/debug_suffix_applied/CMakeLists.txtRepository: bacpack-system/package-tracker
Length of output: 1317
🏁 Script executed:
cat -n ./cmake_tests/mock/CMLIB_STORAGE.cmakeRepository: bacpack-system/package-tracker
Length of output: 4320
🏁 Script executed:
cat -n ./cmake_tests/mock/CMLIB_DEPENDENCY.cmakeRepository: bacpack-system/package-tracker
Length of output: 3140
Potentially tautological assertion — check MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME instead.
The test pre-configures MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT (line 14) to a URI that already contains libmypkgd-dev, then checks MOCK_CMLIB_DEPENDENCY_URI (line 26) for the same string. Based on the mock flow — BA_PACKAGE_LIBRARY calls CMLIB_STORAGE_TEMPLATE_INSTANCE → receives the pre-configured output → passes it to CMLIB_DEPENDENCY — MOCK_CMLIB_DEPENDENCY_URI will always equal MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT. The assertion therefore tests the hardcoded mock value, not whether the library correctly constructed the debug archive name.
The meaningful check would mirror what debug_suffix_applied/CMakeLists.txt does: assert on MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME (the name passed into the storage mock) rather than the URI echoed back out. This variable is already captured by the mock.
Additionally, the FATAL_ERROR message on line 28 says "URI does not contain 'd-dev' suffix" while the actual search string is "libmypkgd-dev" — these should be consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmake_tests/BA_PACKAGE_LIBRARY/debug_suffix/CMakeLists.txt` at line 14,
Replace the tautological assertion that compares MOCK_CMLIB_DEPENDENCY_URI
(which echoes MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT) with a check against
the captured input name MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME so the
test verifies the archive name produced by BA_PACKAGE_LIBRARY →
CMLIB_STORAGE_TEMPLATE_INSTANCE before the mock returns the URI; update the
assertion to look for the expected "libmypkgd-dev" substring in
MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_ARCHIVE_NAME (not the output URI), and fix
the FATAL_ERROR text to mention the exact search string ("libmypkgd-dev") so the
failure message matches the actual check.
| ``` | ||
| cmake -P cmake_tests/CMakeLists.txt | ||
| ``` |
There was a problem hiding this comment.
Add a language specifier to the fenced code block (MD040).
✏️ Proposed fix
-```
+```sh
cmake -P cmake_tests/CMakeLists.txt</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmake_tests/README.md` around lines 12 - 14, The fenced code block in
cmake_tests/README.md is missing a language specifier (MD040); update the
opening fence for the block containing the command "cmake -P
cmake_tests/CMakeLists.txt" from ``` to ```sh so the block is marked as shell
script and satisfies the linter.
| - `<GIT_PATH>` - path to Packages in the repository for a given system. Set to `${CMDEF_DISTRO_ID}/${CMDEF_DISTRO_VERSION_ID}/${CMDEF_ARCHITECTURE}`, | ||
| - `<PACKAGE_GROUP_NAME>` - package group name as stated in [BringAuto Packager Context] | ||
| - `<ARCHIVE_NAME>` - full name of the Package. Set to ${package_group_name}_${version_tag}_${platform_string}.zip. The platform string is derived from CMDEF variables: `CMDEF_ARCHITECTURE`, `CMDEF_DISTRO_ID`, `CMDEF_DISTRO_VERSION_ID`. | ||
| - `GIT_PATH_TEMPLATE` - [CMake-lib] template to construct path to the package in the repository. If set `URI_TEMPLATE` shall represents remote Git repository. It takes same template params as `URI_TEMPLATE`. |
There was a problem hiding this comment.
Grammar nit: missing article.
"It takes same template params" → "It takes the same template params".
✏️ Proposed fix
-- `GIT_PATH_TEMPLATE` - [CMake-lib] template to construct path to the package in the repository. If set `URI_TEMPLATE` shall represents remote Git repository. It takes same template params as `URI_TEMPLATE`.
+- `GIT_PATH_TEMPLATE` - [CMake-lib] template to construct path to the package in the repository. If set `URI_TEMPLATE` shall represent a remote Git repository. It takes the same template params as `URI_TEMPLATE`.📝 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.
| - `GIT_PATH_TEMPLATE` - [CMake-lib] template to construct path to the package in the repository. If set `URI_TEMPLATE` shall represents remote Git repository. It takes same template params as `URI_TEMPLATE`. | |
| - `GIT_PATH_TEMPLATE` - [CMake-lib] template to construct path to the package in the repository. If set `URI_TEMPLATE` shall represent a remote Git repository. It takes the same template params as `URI_TEMPLATE`. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 65, Update the README sentence for GIT_PATH_TEMPLATE to
include the missing article: change "It takes same template params as
`URI_TEMPLATE`" to "It takes the same template params as `URI_TEMPLATE`",
referencing the `GIT_PATH_TEMPLATE` and `URI_TEMPLATE` identifiers in the
updated line.
| If the same cache path is used for Release and Debug build types | ||
| it must ensured that the package differs between Debug/Release build configs | ||
| and does not have files with the same paths. |
There was a problem hiding this comment.
Grammar nit: missing auxiliary verb.
"it must ensured that" → "it must be ensured that".
✏️ Proposed fix
-If the same cache path is used for Release and Debug build types
-it must ensured that the package differs between Debug/Release build configs
+If the same cache path is used for Release and Debug build types,
+it must be ensured that the package differs between Debug/Release build configs📝 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.
| If the same cache path is used for Release and Debug build types | |
| it must ensured that the package differs between Debug/Release build configs | |
| and does not have files with the same paths. | |
| If the same cache path is used for Release and Debug build types, | |
| it must be ensured that the package differs between Debug/Release build configs | |
| and does not have files with the same paths. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 90 - 92, Update the README sentence "it must ensured
that" to include the missing auxiliary verb by changing it to "it must be
ensured that" so the line reads: "If the same cache path is used for Release and
Debug build types it must be ensured that the package differs between
Debug/Release build configs and does not have files with the same paths."
| TEST_POST_TARGET_SETUP(TARGET ${PROJECT_NAME}) | ||
|
|
There was a problem hiding this comment.
Positional parameter target will receive the string "TARGET", not the project name.
When a CMake function is invoked, formal parameters (${arg1}, …) are replaced with the arguments passed in positional order.
FUNCTION(TEST_POST_TARGET_SETUP target) declares target as a positional parameter. The call TEST_POST_TARGET_SETUP(TARGET ${PROJECT_NAME}) passes two arguments: "TARGET" and "MyTestApp". CMake maps the first positional argument to target, so inside any non-empty implementation ${target} resolves to the literal string "TARGET", and the real target name ends up in ${ARGN}.
All three stubs are currently empty no-ops, so there is no runtime failure today. However, any future implementor who writes a non-trivial TEST_POST_TARGET_SETUP that actually uses ${target} will silently operate on the wrong value. The fix is to align the calling convention with the function signature.
🔧 Option A – use positional call style (simplest)
-TEST_POST_TARGET_SETUP(TARGET ${PROJECT_NAME})
+TEST_POST_TARGET_SETUP(${PROJECT_NAME})And update the stub definitions in all three .txt test list files (no change needed to the signature itself since the body is empty):
FUNCTION(TEST_POST_TARGET_SETUP target)
ENDFUNCTION()🔧 Option B – keep keyword call style, update function signature and stubs
Change the call site (no modification needed) and update the stub/real implementations to parse the TARGET keyword:
FUNCTION(TEST_POST_TARGET_SETUP)
+ cmake_parse_arguments(PARSE_ARGV 0 _arg "" "TARGET" "")
+ # target name available as ${_arg_TARGET}
ENDFUNCTION()📝 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.
| TEST_POST_TARGET_SETUP(TARGET ${PROJECT_NAME}) | |
| TEST_POST_TARGET_SETUP(${PROJECT_NAME}) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/app/CMakeLists.txt` around lines 115 - 116, The call to
TEST_POST_TARGET_SETUP currently passes the literal token "TARGET" as the first
positional argument so any FUNCTION(TEST_POST_TARGET_SETUP target)
implementation will receive "TARGET" instead of the actual target name; fix this
by making the call match the function signature—use positional style
TEST_POST_TARGET_SETUP(${PROJECT_NAME}) so the declared parameter target
receives the project name, or alternatively update the function signature/impl
to accept a keyword form (parse a TARGET keyword from ARGN) if you prefer to
keep the existing call style; update the corresponding stub implementations for
TEST_POST_TARGET_SETUP to match whichever convention you choose.
| SET_TARGET_PROPERTIES(${PROJECT_NAME}-shared | ||
| PROPERTIES | ||
| OUTPUT_NAME ${PROJECT_NAME}-1.0 | ||
| SOVERSION 0 | ||
| VERSION ${LIBRARY_VERSION} | ||
| ) |
There was a problem hiding this comment.
Hardcoded -1.0 in OUTPUT_NAME will diverge from LIBRARY_VERSION.
OUTPUT_NAME is fixed to shared-library-gst-1.0 regardless of what is passed as LIBRARY_VERSION. If a caller passes LIBRARY_VERSION=2.0.0, the output filename still embeds 1.0. Derive the major (or full) version from LIBRARY_VERSION to keep the name consistent:
🐛 Proposed fix
+STRING(REGEX MATCH "^[0-9]+" LIBRARY_MAJOR_VERSION "${LIBRARY_VERSION}")
SET_TARGET_PROPERTIES(${PROJECT_NAME}-shared
PROPERTIES
- OUTPUT_NAME ${PROJECT_NAME}-1.0
+ OUTPUT_NAME ${PROJECT_NAME}-${LIBRARY_MAJOR_VERSION}.0
SOVERSION 0
VERSION ${LIBRARY_VERSION}
)📝 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.
| SET_TARGET_PROPERTIES(${PROJECT_NAME}-shared | |
| PROPERTIES | |
| OUTPUT_NAME ${PROJECT_NAME}-1.0 | |
| SOVERSION 0 | |
| VERSION ${LIBRARY_VERSION} | |
| ) | |
| STRING(REGEX MATCH "^[0-9]+" LIBRARY_MAJOR_VERSION "${LIBRARY_VERSION}") | |
| SET_TARGET_PROPERTIES(${PROJECT_NAME}-shared | |
| PROPERTIES | |
| OUTPUT_NAME ${PROJECT_NAME}-${LIBRARY_MAJOR_VERSION}.0 | |
| SOVERSION 0 | |
| VERSION ${LIBRARY_VERSION} | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/app/lib/shared_library_gst/CMakeLists.txt` around lines 25 - 30, The
OUTPUT_NAME currently hardcodes "-1.0" for the ${PROJECT_NAME}-shared target
which can diverge from ${LIBRARY_VERSION}; update CMake to compute the desired
suffix from LIBRARY_VERSION (e.g., extract the major version or entire version
string via CMake string(REGEX REPLACE ...) or string(REPLACE ...) into a
variable like LIB_OUTPUT_SUFFIX) and then set OUTPUT_NAME to
"${PROJECT_NAME}-${LIB_OUTPUT_SUFFIX}" for the ${PROJECT_NAME}-shared target so
the filename matches LIBRARY_VERSION consistently.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
BA_PACKAGE.cmake (1)
253-274:_BA_PACKAGE_URL_ENCODEis missing several RFC 3986 unsafe characters.The following characters are not encoded but are unsafe in URI components:
<(%3C),>(%3E),[(%5B),](%5D),^(%5E),`(%60),{(%7B),|(%7C),}(%7D),\(%5C). If input values are guaranteed never to contain these (typical for distro IDs, version IDs, machine names), the current set is acceptable; otherwise the encoding is incomplete.♻️ Proposed additions
STRING(REPLACE "@" "%40" result "${result}") + STRING(REPLACE "<" "%3C" result "${result}") + STRING(REPLACE ">" "%3E" result "${result}") + STRING(REPLACE "[" "%5B" result "${result}") + STRING(REPLACE "]" "%5D" result "${result}") + STRING(REPLACE "^" "%5E" result "${result}") + STRING(REPLACE "`" "%60" result "${result}") + STRING(REPLACE "{" "%7B" result "${result}") + STRING(REPLACE "|" "%7C" result "${result}") + STRING(REPLACE "}" "%7D" result "${result}") + STRING(REPLACE "\\" "%5C" result "${result}") SET(${output} "${result}" PARENT_SCOPE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BA_PACKAGE.cmake` around lines 253 - 274, The _BA_PACKAGE_URL_ENCODE function currently misses several RFC 3986 reserved/unsafe characters; update the function to add STRING(REPLACE calls for each missing character mapping so inputs escape `<` -> `%3C`, `>` -> `%3E`, `[` -> `%5B`, `]` -> `%5D`, `^` -> `%5E`, `` ` `` -> `%60`, `{` -> `%7B`, `|` -> `%7C`, `}` -> `%7D`, and `\` -> `%5C` (keeping the existing SET(${output} "${result}" PARENT_SCOPE) behavior); place each new STRING(REPLACE "X" "%YY" result "${result}") in the same style as the other replacements inside _BA_PACKAGE_URL_ENCODE so the function returns a fully encoded result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BA_PACKAGE.cmake`:
- Line 194: Save the unencoded revision string before the ESCAPE_TEMPLATE_ARGS
block (e.g. set revision_var_orig "${revision_var}") so the code can safely
percent-encode revision_var in-place for template use, but pass the original
unencoded value to CMLIB_DEPENDENCY; specifically, change the GIT_REVISION
assignment to use revision_var_orig (SET(git_revision_arg GIT_REVISION
"${revision_var_orig}")) while keeping the in-place encoding of revision_var for
other template-escaped paths.
---
Duplicate comments:
In `@BA_PACKAGE.cmake`:
- Line 24: Fix the typo in the header comment of BA_PACKAGE.cmake where "arounf"
is misspelled; update both occurrences (the comment string "Wrapper arounf
BA_PACKAGE") to "Wrapper around BA_PACKAGE" so the header reads correctly in the
file (ensure you replace both instances at the two comment locations).
- Around line 164-169: The URL-encoding block (guarded by escape_template_args)
mutates package_name and package_string in-place so the encoded values are later
incorrectly used to form the BACPACK cache key and shown in the FATAL_ERROR;
preserve original values before encoding by creating and using separate
variables (e.g. package_name_for_cache and package_string_for_message) prior to
calling _BA_PACKAGE_URL_ENCODE, then use package_name_for_cache when
constructing the BACPACK cache key and package_string_for_message when building
the FATAL_ERROR message so the cache key and error output remain based on the
unencoded originals.
---
Nitpick comments:
In `@BA_PACKAGE.cmake`:
- Around line 253-274: The _BA_PACKAGE_URL_ENCODE function currently misses
several RFC 3986 reserved/unsafe characters; update the function to add
STRING(REPLACE calls for each missing character mapping so inputs escape `<` ->
`%3C`, `>` -> `%3E`, `[` -> `%5B`, `]` -> `%5D`, `^` -> `%5E`, `` ` `` -> `%60`,
`{` -> `%7B`, `|` -> `%7C`, `}` -> `%7D`, and `\` -> `%5C` (keeping the existing
SET(${output} "${result}" PARENT_SCOPE) behavior); place each new STRING(REPLACE
"X" "%YY" result "${result}") in the same style as the other replacements inside
_BA_PACKAGE_URL_ENCODE so the function returns a fully encoded result.
| ARCHIVE_NAME "${package_string}" | ||
| PACKAGE_GROUP_NAME "${package_name}" | ||
| ) | ||
| SET(git_revision_arg GIT_REVISION "${revision_var}") |
There was a problem hiding this comment.
URL-encoded revision_var passed as GIT_REVISION breaks git operations.
When ESCAPE_TEMPLATE_ARGS is enabled, revision_var is percent-encoded in-place at Line 165. Line 194 then passes this encoded value as GIT_REVISION to CMLIB_DEPENDENCY. Any revision containing / (e.g. refs/heads/main → refs%2Fheads%2Fmain) will be an invalid git reference at the git layer, causing fetch failures at runtime.
The existing proposed fix for the past comment already encodes revision_var in-place; revision_var_orig must also be saved before the encoding block and used here.
🐛 Proposed fix
BA_PACKAGE_VARS_GET(REVISION revision_var)
+ SET(revision_var_orig "${revision_var}")
SET(git_path "${CMDEF_DISTRO_ID}/${CMDEF_DISTRO_VERSION_ID}/${machine}")
BA_PACKAGE_VARS_GET(ESCAPE_TEMPLATE_ARGS escape_template_args)
IF(escape_template_args)
_BA_PACKAGE_URL_ENCODE("${revision_var}" revision_var)
...
ENDIF()Then at Line 194:
- SET(git_revision_arg GIT_REVISION "${revision_var}")
+ SET(git_revision_arg GIT_REVISION "${revision_var_orig}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BA_PACKAGE.cmake` at line 194, Save the unencoded revision string before the
ESCAPE_TEMPLATE_ARGS block (e.g. set revision_var_orig "${revision_var}") so the
code can safely percent-encode revision_var in-place for template use, but pass
the original unencoded value to CMLIB_DEPENDENCY; specifically, change the
GIT_REVISION assignment to use revision_var_orig (SET(git_revision_arg
GIT_REVISION "${revision_var_orig}")) while keeping the in-place encoding of
revision_var for other template-escaped paths.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes