Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces internal _BRINGAUTO_PACKAGE with public BA_PACKAGE; expands signature and template handling (GIT_PATH_TEMPLATE, ESCAPE_TEMPLATE_ARGS); surfaces HTTP header and git-archive-path config; updates STORAGE/PREREQ init to return extra outputs; and adds extensive unit tests and mocks for BA_PACKAGE flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant BA as BA_PACKAGE
participant PR as BA_PACKAGE_PREREQ_CMCONF_INIT
participant CM as CMCONF (mock)
participant ST as CMLIB_STORAGE_TEMPLATE_INSTANCE (mock)
participant CL as CMLIB_DEPENDENCY (mock)
BA->>PR: request template, revision, git_archive_path, http_header, escape_flag
PR->>CM: CMCONF_GET(...) for URI_TEMPLATE, REVISION, GIT_ARCHIVE_PATH_TEMPLATE, HTTP_HEADER, TEMPLATE_ARGS_URI_ESCAPE
CM-->>PR: return configured values
PR-->>BA: resolved template, revision, git_archive_path, http_header, escape_flag
BA->>ST: expand storage template (ARCHIVE_NAME, GIT_PATH, REVISION, PACKAGE_GROUP_NAME)
ST-->>BA: return URI
BA->>CL: CMLIB_DEPENDENCY(TYPE/URI, ARCHIVE_TYPE, optional GIT_REVISION/GIT_PATH, OUTPUT_PATH_VAR)
CL-->>BA: return OUTPUT_PATH_VAR (cache path)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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.
Comment on lines
+164
to
+169
| 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.
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.
There was a problem hiding this comment.
🧹 Nitpick comments (5)
cmake_tests/BA_PACKAGE/archive_type_zip/CMakeLists.txt (1)
20-23: Consider assertingoutput_pathto complete the test contract.
output_pathis declared in theBA_PACKAGEcall but never verified. If the intent is only to checkARCHIVE_TYPEforwarding, the output variable is unused noise; if it should be populated, its value goes unchecked.💡 Minimal addition if output path validation is in scope
TEST_VAR_TRUE(MOCK_CMLIB_DEPENDENCY_CALLED) TEST_VAR_EQUALS_LITERAL(MOCK_CMLIB_DEPENDENCY_ARCHIVE_TYPE "ZIP") +TEST_VAR_DEFINED(output_path) # or TEST_VAR_EQUALS_LITERAL if the exact value is known🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/BA_PACKAGE/archive_type_zip/CMakeLists.txt` around lines 20 - 23, The BA_PACKAGE invocation declares an output variable output_path that is never asserted; either remove the unused output parameter from BA_PACKAGE or add an assertion to verify the produced path (e.g., check TEST_VAR_EQUALS_LITERAL or TEST_VAR_MATCHES on output_path) so the test verifies the contract. Locate the BA_PACKAGE(...) line and either drop the output_path argument or add a new test assertion referencing output_path (using existing test helpers like TEST_VAR_EQUALS_LITERAL or TEST_VAR_MATCHES) to validate the expected value.cmake_tests/mock/CMLIB_DEPENDENCY.cmake (1)
1-7: Module header describes scope too narrowly.The comment on line 3 says "for testing BA_PACKAGE_LIBRARY", but the mock is consumed by
BA_PACKAGE,BA_PACKAGE_LIBRARY, andBA_PACKAGE_EXECUTABLEtests alike.📝 Suggested update
-# Mock CMLIB_DEPENDENCY module for testing BA_PACKAGE_LIBRARY +# Mock CMLIB_DEPENDENCY module for testing BA_PACKAGE, BA_PACKAGE_LIBRARY, and BA_PACKAGE_EXECUTABLE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/mock/CMLIB_DEPENDENCY.cmake` around lines 1 - 7, The module header for the CMLIB_DEPENDENCY mock is too narrow — it states it's "for testing BA_PACKAGE_LIBRARY" but the mock is used by tests for BA_PACKAGE and BA_PACKAGE_EXECUTABLE as well; update the top comment in CMLIB_DEPENDENCY to reflect that it is a shared test mock consumed by BA_PACKAGE, BA_PACKAGE_LIBRARY, and BA_PACKAGE_EXECUTABLE tests, describing that it captures all arguments passed to CMLIB_DEPENDENCY without performing dependency resolution so all three test suites can verify call arguments.cmake_tests/BA_PACKAGE/empty_revision/CMakeLists.txt (1)
2-5: Test comment slightly overstates what the assertion verifies.The header says "does not pass REVISION ... when REVISION is empty", but
TEST_VAR_EQUALS_LITERAL(MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_REVISION "")only confirms the captured value is an empty string. SinceCMLIB_PARSE_ARGUMENTStreats both "argument omitted entirely" and "argument passed as""" identically (both yield""), the test cannot distinguish between these two behaviours. The comment should be adjusted to reflect what is actually checked.📝 Suggested comment update
-# Verifies that BA_PACKAGE does not pass REVISION to -# CMLIB_STORAGE_TEMPLATE_INSTANCE when REVISION is empty. +# Verifies that BA_PACKAGE passes an empty REVISION to +# CMLIB_STORAGE_TEMPLATE_INSTANCE when REVISION is set to "".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/BA_PACKAGE/empty_revision/CMakeLists.txt` around lines 2 - 5, The test header overstates behavior: it claims BA_PACKAGE "does not pass REVISION" when REVISION is empty but the assertion TEST_VAR_EQUALS_LITERAL(MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_REVISION "") only verifies the captured value is the empty string; because CMLIB_PARSE_ARGUMENTS treats an omitted argument and an argument passed as "" the same (both yield ""), update the comment/header to state that the test verifies the captured REVISION value is an empty string (i.e., that REVISION expands to "") rather than asserting REVISION was omitted entirely; reference MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_REVISION, TEST_VAR_EQUALS_LITERAL, CMLIB_PARSE_ARGUMENTS and BA_PACKAGE when editing the comment.cmake_tests/BA_PACKAGE_LIBRARY/release_basic/CMakeLists.txt (2)
19-20:<GIT_PATH>inURI_TEMPLATEis inconsistent with the emptyGIT_PATH_TEMPLATE.
URI_TEMPLATEreferences<GIT_PATH>(line 19), yetGIT_PATH_TEMPLATEis set to""(line 20). In real (non-mocked) execution this would expand to a double-slash (https://example.com/main//<PACKAGE_GROUP_NAME>/…). While the mock bypasses actual template expansion here, the setup is misleading and doesn't reflect a valid real-world configuration. Either omit<GIT_PATH>from the template for this test, or set a non-emptyGIT_PATH_TEMPLATEto keep the config self-consistent.🔧 Option A – Remove `` from the template (simplest for a basic test)
-BA_PACKAGE_VARS_SET(URI_TEMPLATE "https://example.com/<REVISION>/<GIT_PATH>/<PACKAGE_GROUP_NAME>/<ARCHIVE_NAME>") -BA_PACKAGE_VARS_SET(GIT_PATH_TEMPLATE "") +BA_PACKAGE_VARS_SET(URI_TEMPLATE "https://example.com/<REVISION>/<PACKAGE_GROUP_NAME>/<ARCHIVE_NAME>")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/BA_PACKAGE_LIBRARY/release_basic/CMakeLists.txt` around lines 19 - 20, The URI_TEMPLATE currently includes "<GIT_PATH>" while GIT_PATH_TEMPLATE is set to an empty string, causing inconsistent/invalid expansion; update the BA_PACKAGE_VARS_SET call(s) so they are self-consistent—either remove "<GIT_PATH>" from the URI_TEMPLATE value in the BA_PACKAGE_VARS_SET("URI_TEMPLATE" ...) call, or set a non-empty value for GIT_PATH_TEMPLATE via BA_PACKAGE_VARS_SET("GIT_PATH_TEMPLATE" ...) so the template expands correctly; choose the removal option for the basic test to keep the mock simple.
26-34: Consider asserting that the version string is also present in the URI.The test verifies the naming convention (
libmypkg-dev) but doesn't confirm the version (v1.0.0) appears in the generated URI. A stale or incorrect version passthrough would go undetected. Given this is only a "basic" test it may be out of scope, but worth noting.STRING(FIND "${MOCK_CMLIB_DEPENDENCY_URI}" "libmypkg-dev" found_lib_dev) IF(found_lib_dev EQUAL -1) MESSAGE(FATAL_ERROR "URI does not contain 'libmypkg-dev': ${MOCK_CMLIB_DEPENDENCY_URI}") ENDIF() + +STRING(FIND "${MOCK_CMLIB_DEPENDENCY_URI}" "v1.0.0" found_version) +IF(found_version EQUAL -1) + MESSAGE(FATAL_ERROR "URI does not contain expected version 'v1.0.0': ${MOCK_CMLIB_DEPENDENCY_URI}") +ENDIF()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake_tests/BA_PACKAGE_LIBRARY/release_basic/CMakeLists.txt` around lines 26 - 34, Add an assertion that the expected version token appears in the URI: use STRING(FIND on the MOCK_CMLIB_DEPENDENCY_URI to search for the version substring (e.g., "v1.0.0" or the appropriate version variable if one exists) and if the result equals -1 emit MESSAGE(FATAL_ERROR) with a clear message; place this check alongside the existing STRING(FIND checks so it fails the test if the version is missing from MOCK_CMLIB_DEPENDENCY_URI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmake_tests/BA_PACKAGE_LIBRARY/no_debug_skips_suffix/CMakeLists.txt`:
- Around line 26-28: The FATAL_ERROR message is misleading: the STRING(FIND)
looks for "libmypkgd-dev" via variable MOCK_CMLIB_DEPENDENCY_URI and the error
text incorrectly says "d-dev"; update the MESSAGE(FATAL_ERROR) to accurately
describe the detected suffix (e.g., mention "libmypkgd-dev" or "d-dev"
consistently) so the reported text matches the actual check using
found_debug_suffix and MOCK_CMLIB_DEPENDENCY_URI.
---
Nitpick comments:
In `@cmake_tests/BA_PACKAGE_LIBRARY/release_basic/CMakeLists.txt`:
- Around line 19-20: The URI_TEMPLATE currently includes "<GIT_PATH>" while
GIT_PATH_TEMPLATE is set to an empty string, causing inconsistent/invalid
expansion; update the BA_PACKAGE_VARS_SET call(s) so they are
self-consistent—either remove "<GIT_PATH>" from the URI_TEMPLATE value in the
BA_PACKAGE_VARS_SET("URI_TEMPLATE" ...) call, or set a non-empty value for
GIT_PATH_TEMPLATE via BA_PACKAGE_VARS_SET("GIT_PATH_TEMPLATE" ...) so the
template expands correctly; choose the removal option for the basic test to keep
the mock simple.
- Around line 26-34: Add an assertion that the expected version token appears in
the URI: use STRING(FIND on the MOCK_CMLIB_DEPENDENCY_URI to search for the
version substring (e.g., "v1.0.0" or the appropriate version variable if one
exists) and if the result equals -1 emit MESSAGE(FATAL_ERROR) with a clear
message; place this check alongside the existing STRING(FIND checks so it fails
the test if the version is missing from MOCK_CMLIB_DEPENDENCY_URI.
In `@cmake_tests/BA_PACKAGE/archive_type_zip/CMakeLists.txt`:
- Around line 20-23: The BA_PACKAGE invocation declares an output variable
output_path that is never asserted; either remove the unused output parameter
from BA_PACKAGE or add an assertion to verify the produced path (e.g., check
TEST_VAR_EQUALS_LITERAL or TEST_VAR_MATCHES on output_path) so the test verifies
the contract. Locate the BA_PACKAGE(...) line and either drop the output_path
argument or add a new test assertion referencing output_path (using existing
test helpers like TEST_VAR_EQUALS_LITERAL or TEST_VAR_MATCHES) to validate the
expected value.
In `@cmake_tests/BA_PACKAGE/empty_revision/CMakeLists.txt`:
- Around line 2-5: The test header overstates behavior: it claims BA_PACKAGE
"does not pass REVISION" when REVISION is empty but the assertion
TEST_VAR_EQUALS_LITERAL(MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_REVISION "") only
verifies the captured value is the empty string; because CMLIB_PARSE_ARGUMENTS
treats an omitted argument and an argument passed as "" the same (both yield
""), update the comment/header to state that the test verifies the captured
REVISION value is an empty string (i.e., that REVISION expands to "") rather
than asserting REVISION was omitted entirely; reference
MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_REVISION, TEST_VAR_EQUALS_LITERAL,
CMLIB_PARSE_ARGUMENTS and BA_PACKAGE when editing the comment.
In `@cmake_tests/mock/CMLIB_DEPENDENCY.cmake`:
- Around line 1-7: The module header for the CMLIB_DEPENDENCY mock is too narrow
— it states it's "for testing BA_PACKAGE_LIBRARY" but the mock is used by tests
for BA_PACKAGE and BA_PACKAGE_EXECUTABLE as well; update the top comment in
CMLIB_DEPENDENCY to reflect that it is a shared test mock consumed by
BA_PACKAGE, BA_PACKAGE_LIBRARY, and BA_PACKAGE_EXECUTABLE tests, describing that
it captures all arguments passed to CMLIB_DEPENDENCY without performing
dependency resolution so all three test suites can verify call arguments.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
BA_PACKAGE.cmake (2)
204-208: Remove or convert commented-out warning to a TODO/decision record.This block has been left as dead commented-out code with an uncertain note. If it's not needed, remove it to reduce noise; if there's a pending decision, convert it to a clear
# TODO:so it's trackable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BA_PACKAGE.cmake` around lines 204 - 208, Remove the dead commented-out conditional that checks package_name_upper vs package_name_upper_orig (the block using IF(NOT (package_name_upper STREQUAL package_name_upper_orig)) ... MESSAGE(WARNING) ... ENDIF()) or replace it with a clear tracker; specifically either delete those commented lines entirely to reduce noise, or convert them into a TODO/decision note at the top of the file (e.g., prefix with "# TODO:" and explain whether to re-enable the normalization warning and why, referencing the symbols package_name_upper and package_name_upper_orig and the MESSAGE(WARNING) behavior so the future decision is traceable).
253-274: Consider encoding additional URI-unsafe characters for robustness.Per RFC 3986, characters like
[,],{,},^,|, and\are also unsafe in URI components but are not encoded here. If any of these could appear in package names, paths, or revisions, they would pass through unescaped.This is likely fine for current inputs (distro IDs, version strings), but worth noting for future-proofing.
🤖 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 RFC3986-unsafe characters; update the function to also replace the characters [, ], {, }, ^, | and backslash by adding STRING(REPLACE ...) calls that map them to their percent-encoded forms (%5B, %5D, %7B, %7D, %5E, %7C, %5C) before setting the output so package names/paths/revisions are fully URI-safe.
🤖 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 131: Replace the misspelled word "concertized" with the correct word
"concretized" in the comment string in BA_PACKAGE.cmake (the line containing "#
and concertized by 'prefix' and 'suffix'.") so the comment reads "# and
concretized by 'prefix' and 'suffix'."; update that exact comment text to fix
the typo.
---
Duplicate comments:
In `@BA_PACKAGE.cmake`:
- Around line 24-27: There is a misspelling of "around" as "arounf" in the
BA_PACKAGE comment block; update both occurrences of "arounf" to "around" in the
BA_PACKAGE header/comment (the wrapper description that mentions adding package
path to CMAKE_PREFIX_PATH and propagating output variables) so the comment reads
correctly and consistently.
- Around line 161-196: Before URL-encoding mutate copies of the values so the
original values remain available for cache keys, GIT_REVISION and error
messages: save revision_var, package_string and package_name into temporary
originals (e.g., revision_orig, package_string_orig, package_name_orig)
immediately after BA_PACKAGE_VARS_GET; call _BA_PACKAGE_URL_ENCODE only on the
copied variables (not the originals); continue to pass the encoded copies into
the CMLIB_STORAGE_TEMPLATE_INSTANCE(remote_file ...)
GIT_PATH/ARCHIVE_NAME/PACKAGE_GROUP_NAME parameters, but when setting
GIT_REVISION (git_revision_arg), when deriving cache keys/regex stripping, and
when composing the FATAL_ERROR text, use the saved originals (revision_orig,
package_string_orig, package_name_orig) instead of the percent-encoded variables
so downstream git refs, cache keywords and user-facing messages remain correct.
---
Nitpick comments:
In `@BA_PACKAGE.cmake`:
- Around line 204-208: Remove the dead commented-out conditional that checks
package_name_upper vs package_name_upper_orig (the block using IF(NOT
(package_name_upper STREQUAL package_name_upper_orig)) ... MESSAGE(WARNING) ...
ENDIF()) or replace it with a clear tracker; specifically either delete those
commented lines entirely to reduce noise, or convert them into a TODO/decision
note at the top of the file (e.g., prefix with "# TODO:" and explain whether to
re-enable the normalization warning and why, referencing the symbols
package_name_upper and package_name_upper_orig and the MESSAGE(WARNING) behavior
so the future decision is traceable).
- Around line 253-274: The _BA_PACKAGE_URL_ENCODE function currently misses
several RFC3986-unsafe characters; update the function to also replace the
characters [, ], {, }, ^, | and backslash by adding STRING(REPLACE ...) calls
that map them to their percent-encoded forms (%5B, %5D, %7B, %7D, %5E, %7C, %5C)
before setting the output so package names/paths/revisions are fully URI-safe.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake_tests/mock/MOCK_VARIABLES.md`:
- Around line 116-124: The fenced code block containing the line
"MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT ──→ MOCK_CMLIB_DEPENDENCY_URI" is
missing a language specifier; update the opening backticks to include "text"
(i.e., change ``` to ```text) so the block reads as a text-fenced code block and
satisfies markdownlint MD040, keeping the content exactly the same.
In `@doc/MigrationGuide_1_to_2.md`:
- Around line 67-70: Remove or use the unused Markdown link reference
definitions found in MigrationGuide_1_to_2.md — specifically the labels "[CMCONF
Global Config]", "[CMLibStorage.cmake]", "[example/]", and "[cmakelib]": either
delete those four reference lines if they are not referenced elsewhere, or add
inline references to them in the document body so the linter stops flagging
MD053; update only the definitions block (the lines shown in the diff) and keep
link targets unchanged if you retain them.
- Around line 8-10: The markdown contains trailing spaces inside inline code
spans (e.g., `BA_PACKAGE_HTTP_AUTHORIZATION_HEADER ` and `BA_PACKAGE_HTTP_HEADER
`) which trips MD038; remove the trailing whitespace inside those backticks so
the code spans are exactly `BA_PACKAGE_HTTP_AUTHORIZATION_HEADER` and
`BA_PACKAGE_HTTP_HEADER`, and also verify other occurrences on the same file
(notably line ~29) to remove any trailing spaces inside backticks there as well.
---
Duplicate comments:
In `@BA_PACKAGE.cmake`:
- Around line 161-236: The ESCAPE_TEMPLATE_ARGS branch mutates package_name and
revision_var used later for cache keywords and GIT_REVISION; preserve originals
before calling _BA_PACKAGE_URL_ENCODE and use the saved originals when building
keywords, cache lookup (CMLIB_CACHE_GET), GIT_REVISION (git_revision_arg) and
the PACKAGE_NOT_FOUND/MESSAGE text. Concretely: before the
IF(escape_template_args) block copy package_name -> package_name_orig and
revision_var -> revision_var_orig, call _BA_PACKAGE_URL_ENCODE on the mutable
vars as now, but later reference package_name_orig for STRING(TOUPPER)/keywords
and package_string/package_name_orig/revision_var_orig for the cache error
message and for setting GIT_REVISION (git_revision_arg) and any cache key usage
(KEYWORDS ${keywords}) instead of the encoded values.
In `@cmake_tests/README.md`:
- Around line 12-14: Add a language identifier to the fenced code block
containing the shell command "cmake -P cmake_tests/CMakeLists.txt" by changing
the opening fence from ``` to ```sh so the block is treated as shell script
(addresses MD040); update the README.md fenced block that encloses that command
to begin with ```sh.
Comment on lines
+116
to
+124
| ``` | ||
| MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT ──→ MOCK_CMLIB_DEPENDENCY_URI | ||
| ``` | ||
|
|
||
| The mock does **not** perform template expansion. It returns `OUTPUT` (or a default URL) as `remote_file`, | ||
| which BA_PACKAGE then passes as `URI` to `CMLIB_DEPENDENCY`. Therefore `MOCK_CMLIB_DEPENDENCY_URI` | ||
| always equals `MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT` (or the default) — it does **not** reflect | ||
| the computed `package_string`. | ||
|
|
There was a problem hiding this comment.
Add language specifier to fenced code block.
The fenced code block lacks a language specifier, which is flagged by markdownlint (MD040). Add text after the opening backticks to resolve this.
📝 Proposed fix
-```
+```text
MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT ──→ MOCK_CMLIB_DEPENDENCY_URI</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] 116-116: 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/mock/MOCK_VARIABLES.md` around lines 116 - 124, The fenced code
block containing the line "MOCK_CMLIB_STORAGE_TEMPLATE_INSTANCE_OUTPUT ──→
MOCK_CMLIB_DEPENDENCY_URI" is missing a language specifier; update the opening
backticks to include "text" (i.e., change ``` to ```text) so the block reads as
a text-fenced code block and satisfies markdownlint MD040, keeping the content
exactly the same.
Comment on lines
+8
to
+10
| - `BA_PACKAGE_HTTP_AUTHORIZATION_HEADER` CMCONF variable renamed to `BA_PACKAGE_HTTP_HEADER`. | ||
| The value must now contain the full HTTP header (e.g. `Authorization: token <token>`) instead of just the token part. | ||
| Package Tracker no longer prepends `Authorization: ` automatically. |
There was a problem hiding this comment.
Remove trailing spaces inside code spans (MD038).
Trailing spaces inside backticks trigger the linter.
✏️ Proposed fix
- Package Tracker no longer prepends `Authorization: ` automatically.
+ Package Tracker no longer prepends `Authorization:` automatically (include the trailing space in the header value).
@@
- Prepend `Authorization: ` to the value:
+ Prepend `Authorization:` (with a trailing space) to the value:Also applies to: 29-29
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 10-10: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/MigrationGuide_1_to_2.md` around lines 8 - 10, The markdown contains
trailing spaces inside inline code spans (e.g.,
`BA_PACKAGE_HTTP_AUTHORIZATION_HEADER ` and `BA_PACKAGE_HTTP_HEADER `) which
trips MD038; remove the trailing whitespace inside those backticks so the code
spans are exactly `BA_PACKAGE_HTTP_AUTHORIZATION_HEADER` and
`BA_PACKAGE_HTTP_HEADER`, and also verify other occurrences on the same file
(notably line ~29) to remove any trailing spaces inside backticks there as well.
Comment on lines
+67
to
+70
| [CMCONF Global Config]: ./GlobalConfiguration.md | ||
| [CMLibStorage.cmake]: ../example/CMLibStorage.cmake | ||
| [example/]: ../example/ | ||
| [cmakelib]: https://github.com/cmakelib/cmakelib |
There was a problem hiding this comment.
Clean up unused link reference definitions (MD053).
Either reference these in the text or remove them to satisfy the linter.
✏️ Proposed fix (remove unused references)
-[CMCONF Global Config]: ./GlobalConfiguration.md
-[cmakelib]: https://github.com/cmakelib/cmakelib📝 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.
Suggested change
| [CMCONF Global Config]: ./GlobalConfiguration.md | |
| [CMLibStorage.cmake]: ../example/CMLibStorage.cmake | |
| [example/]: ../example/ | |
| [cmakelib]: https://github.com/cmakelib/cmakelib | |
| [CMLibStorage.cmake]: ../example/CMLibStorage.cmake | |
| [example/]: ../example/ |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 67-67: Link and image reference definitions should be needed
Unused link or image reference definition: "cmconf global config"
(MD053, link-image-reference-definitions)
[warning] 70-70: Link and image reference definitions should be needed
Unused link or image reference definition: "cmakelib"
(MD053, link-image-reference-definitions)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/MigrationGuide_1_to_2.md` around lines 67 - 70, Remove or use the unused
Markdown link reference definitions found in MigrationGuide_1_to_2.md —
specifically the labels "[CMCONF Global Config]", "[CMLibStorage.cmake]",
"[example/]", and "[cmakelib]": either delete those four reference lines if they
are not referenced elsewhere, or add inline references to them in the document
body so the linter stops flagging MD053; update only the definitions block (the
lines shown in the diff) and keep link targets unchanged if you retain them.
koudis
force-pushed
the
jan_kubalek/use_git_add_tests
branch
from
4f59b12 to
40fca3f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes