Jan kubalek/component versioning #32
Conversation
|
Warning Rate limit exceeded@koudis has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis update introduces extensive enhancements to the CMLIB project, focusing on improved dependency management, robust file and URI handling, and comprehensive testing infrastructure. Key changes include dynamic URI transformation utilities, expanded support for file:// URIs, refined cache and revision controls, enriched documentation, and a modular, scenario-driven test suite covering argument parsing, error conditions, and resource management. Changes
Sequence Diagram(s)File Download and Dependency Resolution (with FILE/HTTP/GIT URI Types)sequenceDiagram
participant UserScript
participant CMLIB_DEPENDENCY
participant CMLIB_FILE_DOWNLOAD
participant _CMLIB_FILE_DOWNLOAD_FROM_STANDARD_URI
participant TestResourceManager
UserScript->>CMLIB_DEPENDENCY: Call with TYPE/URI/URI_TYPE/OUTPUT_PATH_VAR
CMLIB_DEPENDENCY->>CMLIB_FILE_DOWNLOAD: Download file (with URI_TYPE)
CMLIB_FILE_DOWNLOAD->>_CMLIB_FILE_DOWNLOAD_FROM_STANDARD_URI: Dispatch based on URI_TYPE
_CMLIB_FILE_DOWNLOAD_FROM_STANDARD_URI->>_CMLIB_FILE_DOWNLOAD_FROM_STANDARD_URI: Validate URI (scheme, path)
_CMLIB_FILE_DOWNLOAD_FROM_STANDARD_URI-->>CMLIB_FILE_DOWNLOAD: Download or copy file
CMLIB_FILE_DOWNLOAD-->>CMLIB_DEPENDENCY: Return output path
CMLIB_DEPENDENCY-->>UserScript: Output path variable set
Test Resource Download and File URI RetrievalsequenceDiagram
participant TestScript
participant TestResourceManager
TestScript->>TestResourceManager: TEST_RESOURCES_DOWNLOAD_ENABLE()
TestScript->>TestResourceManager: TEST_RESOURCES_DOWNLOAD()
TestResourceManager->>TestResourceManager: Clone cmakelib-test repo if enabled
TestScript->>TestResourceManager: TEST_RESROUCES_GET_FILE_URI("path", out_var)
TestResourceManager-->>TestScript: Return file:// URI for resource
Git URI Transformation UtilitysequenceDiagram
participant TestScript
participant TransformGitUri
TestScript->>TransformGitUri: TRANSFORM_GIT_URI(URI, OUTPUT_VAR)
alt URI is HTTP(S)
TransformGitUri-->>TestScript: Output SSH format URI
else URI is SSH
TransformGitUri-->>TestScript: Output unchanged
end
TestScript->>TransformGitUri: TRANSFORM_GIT_URI_TO_HTTPS(URI, OUTPUT_VAR)
alt URI is SSH
TransformGitUri-->>TestScript: Output HTTPS format URI
else URI is HTTP(S)
TransformGitUri-->>TestScript: Output unchanged
end
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
♻️ Duplicate comments (2)
test/DEPENDENCY/cache_control/mixed_no_keywords_first/CMakeLists.txt (1)
17-22: Same env-var guard as in other cache-control testsFor consistency and clearer diagnostics, add the
CMLIB_REQUIRED_ENV_REMOTE_URLpresence check here as well (see comment insame_file_diff_revision).test/PARSE_ARGUMENTS/test_cases/error_conditions/CMakeLists.txt (1)
34-40: Same abort issue for malformed-input testWrap it with
TEST_INVALID_CMAKE_RUNor the entire suite fails on the first configure error.
🧹 Nitpick comments (34)
test/test_resources/.gitignore (1)
1-1: Prefer directory patternresources/overresources/*.
resources/*ignores the contents but still lets the directory itself be committed if an empty placeholder sneaks in.
Using the trailing-slash form is the conventional way to ignore the directory and everything under it:-resources/* +resources/test/README.md (1)
6-8: Tone & punctuation polish.Minor wording/consistency nits:
-CMlib is consistent and working if and only if all tests pass! -If one test fails the CMlib does not work as expected! Even if the failure is not directly related to the tested functionality! Whole consistency is needed! +CMLIB is considered consistent **only** if all tests pass. +If a single test fails, CMLIB does not work as expected—even if the failure is not directly related to the tested functionality. Complete consistency is required.• Unifies the capitalization (
CMLIB),
• removes repetitive exclamation marks,
• adds a missing comma and improves flow.
Feel free to ignore if marketing-style emphasis is deliberate.test/CMakeLists.txt (1)
11-15: Guard resource downloading behind an option to support offline builds.Invoking
TEST_RESOURCES_DOWNLOAD_ENABLE() TEST_RESOURCES_DOWNLOAD()unconditionally triggers network traffic at configure time. Consider wrapping the block in an option/cache variable so users can do:
cmake -DENABLE_TEST_RESOURCE_DOWNLOAD=OFF ..and still configure the project when offline:
+option(ENABLE_TEST_RESOURCE_DOWNLOAD "Download external test resources" ON) +if(ENABLE_TEST_RESOURCE_DOWNLOAD) TEST_RESOURCES_DOWNLOAD_ENABLE() TEST_RESOURCES_DOWNLOAD() TEST_RESOURCES_DOWNLOAD_DISABLE() +endif()example/DEPENDENCY/cache_control_disabled/CMakeLists.txt (1)
21-26: Verify thatTRANSFORM_GIT_URI_TO_HTTPShandles non-git ZIP pathsThe macro name suggests Git-to-HTTPS translation but you pass an archive URL ending with
.zip.
Confirm the helper accepts such paths; otherwise a simple string assignment may be clearer and cheaper than invoking the transform macro.test/DEPENDENCY/README.md (2)
4-4: Subject–verb agreement typo
DEPENDENCY tests does not➜DEPENDENCY tests do not.-DEPENDENCY tests does not test every possible argument combination. +DEPENDENCY tests do not test every possible argument combination.
13-13: Escape bare URL to satisfy markdownlintWrap
git@github.com:user/repo.gitin back-ticks or use a markdown link to silence MD034.- (file://, http://, https://, git://, git@github.com:user/repo.git) + (file://, http://, https://, git://, `git@github.com:user/repo.git`)test/DEPENDENCY/type_check/CMakeLists.txt (2)
13-13: Prefer PREPEND to guarantee project-local modules win over system modulesAppending your project root to
CMAKE_MODULE_PATHpushes it to the end of the search list, letting any identically-named modules installed on the host overshadow yours.list(PREPEND …)keeps test isolation deterministic.-LIST(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/../../../") +list(PREPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/../../../")
18-22: Collapse repetitiveTEST_RUNcalls into a concise loopFour almost-identical invocations add noise and risk drifting out of sync. A short loop trims five lines and scales if more
TYPE_*cases appear.foreach(_case FILE MODULE ARCHIVE DIRECTORY) test_run("${CMAKE_CURRENT_LIST_DIR}/type_${_case}") endforeach()test/DEPENDENCY/download_check/fail/file_uri_directory/CMakeLists.txt (1)
15-17: Reuse the canonical helper to build file:// URIsThe repository already ships
TEST_RESOURCES_GET_FILE_URI; hard-coding the path duplicates logic, can break on Windows drive letters, and inconsistently encodes spaces.-SET(TEST_DIR_PATH "${CMAKE_CURRENT_LIST_DIR}/../../../../test_resources") -GET_FILENAME_COMPONENT(TEST_DIR_PATH "${TEST_DIR_PATH}" ABSOLUTE) -SET(FILE_URI "file://${TEST_DIR_PATH}") +include("${CMAKE_CURRENT_LIST_DIR}/../../../test_resources/test_resources.cmake") +test_resources_get_file_uri("." FILE_URI) # “.” returns the directory itselfConfirm that the helper macro is available from this relative path; if not, prepend its location to
CMAKE_MODULE_PATHas done in other tests.test/DEPENDENCY/argument_missing/CMakeLists.txt (1)
26-29: Optional: make the expected failure explicit for CTestConsider wrapping the dependency call in
EXPECT_FAIL()(or your existing negative-test helper) to ensure the test is marked passed only when the specific error is triggered, not on any earlier abort.test/DEPENDENCY/cache_control/mixed_keywords_first/CMakeLists.txt (1)
17-22: Same remote-URL guard as aboveReplicate the defensive check for
CMLIB_REQUIRED_ENV_REMOTE_URLto avoid hidden empty URIs and double slashes when the env var already ends with “/”.test/test_resources/README.md (3)
11-11: Fix grammatical errors in documentation.The static analysis correctly identified issues:
- Remove duplicate "to" in "are used to to control"
- Consider using "whether" instead of "if" for better formality
Apply this diff to fix the errors:
-- Use `TEST_RESOURCES_DOWNLOAD_ENABLE()` and `TEST_RESOURCES_DOWNLOAD_DISABLE()` are used to to control if the download is allowed or not. +- Use `TEST_RESOURCES_DOWNLOAD_ENABLE()` and `TEST_RESOURCES_DOWNLOAD_DISABLE()` are used to control whether the download is allowed or not.
12-12: Fix typo in function name.There's a typo in the function name
TEST_RESROUCES_GET_FILE_URI- should beTEST_RESOURCES_GET_FILE_URI.Apply this diff to fix the typo:
-- Use TEST_RESROUCES_GET_FILE_URI() to get file:// URI for test files from downloaded repository. +- Use TEST_RESOURCES_GET_FILE_URI() to get file:// URI for test files from downloaded repository.
21-21: Fix typo in function name in example.The same typo appears in the example code.
Apply this diff to fix the typo:
-TEST_RESROUCES_GET_FILE_URI("relative/path/to/resource" resource_uri) +TEST_RESOURCES_GET_FILE_URI("relative/path/to/resource" resource_uri)test/DEPENDENCY/download_check/file_uri_download/CMakeLists.txt (1)
24-26: Failure message omits context when${output_path}is emptyIf
CMLIB_DEPENDENCYerrors out and never setsoutput_path, the${output_path}placeholder expands to an empty string, producing an unhelpful error.
Consider guarding against an unset variable or printing “” for clarity.test/DEPENDENCY/download_check/fail/uri_type_mismatches/git_with_file/CMakeLists.txt (1)
21-27: RedundantTYPE/URI_TYPEcombination may hide root causeFor a mismatch test you only need one of the two keywords. Having both
TYPE FILEandURI_TYPE FILEadds noise and could change behaviour if the macro resolves conflicts internally. Keeping the minimal failing-case improves maintainability.test/DEPENDENCY/cache_control/keywords_with_diff_revision/CMakeLists.txt (1)
17-22: ConsiderTRANSFORM_GIT_URI_TO_HTTPSfor consistencyMost new tests rely on
TRANSFORM_GIT_URI_TO_HTTPS, while this one uses the more genericTRANSFORM_GIT_URI. Unless you need the SSH variant here, switching keeps the suite uniform.test/FILE_DOWNLOAD/CMakeLists.txt (1)
185-187: Quote paths passed toFILE(REMOVE)Un-quoted variables are split on whitespace.
If${tmp}ever contains a space the remove operation will silently fail or, worse, delete the wrong file(s).-FILE(REMOVE ${tmp}) +FILE(REMOVE "${tmp}")Do the same for
${test_branch_tmp}further down.example/FILE_DOWNLOAD/CMakeLists.txt (1)
10-13: Same env-var guard as in tests
TRANSFORM_GIT_URIwill fail ifCMLIB_REQUIRED_ENV_REMOTE_URLis unset.
Replicate the defensive check suggested for the test suite to keep the example copy-&-paste-able.test/DEPENDENCY/download_check/fail/uri_type_mismatches/http_with_file/CMakeLists.txt (1)
15-20: Consider clarifying intent with an explicitURI_TYPE HTTPCurrently
TYPE FILEandURI_TYPE FILEmatch, so the only mismatch is thehttps://scheme.
If the objective is to test “URI_TYPE FILE but non-file scheme”, leavingURI_TYPEimplicit (or even setting it toFILEas you did) is valid but may be confusing to future readers scanning directory names. Adding a short inline comment such as# scheme mismatch onlywould remove the ambiguity.No functional change required – suggestion for clarity only.
test/DEPENDENCY/type_check/type_archive/CMakeLists.txt (2)
22-30: Improve cross-platform compatibility for archive creation.The
tarcommand with--format=zipmay not be available on all systems. Consider adding a fallback or checking for availability.# Create ZIP archive from test resources directory +# Check if tar supports zip format +EXECUTE_PROCESS( + COMMAND ${CMAKE_COMMAND} -E tar --help + OUTPUT_VARIABLE tar_help + ERROR_QUIET +) + +IF(NOT tar_help MATCHES "zip") + MESSAGE(FATAL_ERROR "CMAKE tar command does not support zip format on this platform") +ENDIF() + EXECUTE_PROCESS( COMMAND ${CMAKE_COMMAND} -E tar "cf" "${TEST_ARCHIVE}" --format=zip . WORKING_DIRECTORY "${RESOURCES_DIR}" RESULT_VARIABLE archive_result )
32-32: Consider using CMAKE_PATH for file URI construction.For better cross-platform compatibility, consider using CMake's path utilities for file URI construction.
-SET(FILE_URI "file://${TEST_ARCHIVE}") +FILE(TO_CMAKE_PATH "${TEST_ARCHIVE}" TEST_ARCHIVE_NORMALIZED) +SET(FILE_URI "file://${TEST_ARCHIVE_NORMALIZED}")test/DEPENDENCY/download_check/fail/unreachable_uris/git_uri_unreachable_ssh/CMakeLists.txt (1)
12-13: Consider the deep relative path structure.The include path goes up 7 directory levels, which might be fragile if the test directory structure changes.
Consider using a more robust approach for locating the CMLIB module path, perhaps by setting a variable at the test suite level or using
CMAKE_CURRENT_SOURCE_DIRrelative to a known base.test/DEPENDENCY/cache_control/same_file_diff_remotes/CMakeLists.txt (1)
29-41: Shadowingoutput_pathtwice loses the first resultBoth
CMLIB_DEPENDENCYinvocations write to the same cache variableoutput_path.
Although this test is expected to fail, using two different variables (output_path_a,output_path_b) makes debugging easier when the failure reason changes in future.test/FILE_DOWNLOAD/compute_hash/CMakeLists.txt (1)
12-14: Early include paths make the script fragileAbsolute relative paths (
../../transform_git_uri.cmake) assume the file layout never changes.
Importing vialist(APPEND CMAKE_MODULE_PATH …)and then plaininclude(transform_git_uri)keeps the path logic in one place and avoids duplication.system_modules/CMLIB_COMPONENT.cmake (2)
24-48: Fix typo in comment and variable naming inconsistency.Line 26 has a typo: "aspecified" should be "as specified". Also, there's a typo in the variable name on line 29: "VARANAME" should be "VARNAME".
-# When CMLIB_LOCAL_BASE_PATH is set the revisions aspecified by these variables are ignored. +# When CMLIB_LOCAL_BASE_PATH is set the revisions as specified by these variables are ignored. -SET(_CMLIB_COMPONENT_REVISION_VARANAME_PREFIX "CMLIB_COMPONENT_REVISION_" +SET(_CMLIB_COMPONENT_REVISION_VARNAME_PREFIX "CMLIB_COMPONENT_REVISION_"
34-47: Consider using consistent revision strategies.The revision settings show mixed approaches:
cmdefuses a specific version tag (v0.2.1), whilestorageandcmutilusemaster. This inconsistency could lead to different stability levels across components.Consider standardizing the revision strategy - either use specific version tags for all components for better reproducibility, or document the rationale for using
masterfor certain components.test/cache_var.cmake (1)
61-79: Improve error handling in CACHE_VAR_RESTORE.The function checks if the global property exists but doesn't handle the case where the original value property might not exist. Also, the RETURN() call after MESSAGE(FATAL_ERROR) is redundant.
FUNCTION(CACHE_VAR_RESTORE var_name) GET_PROPERTY(was_defined GLOBAL PROPERTY CACHE_VAR_WAS_DEFINED_${var_name} SET) IF(NOT was_defined) MESSAGE(FATAL_ERROR "No stored information found for cache variable '${var_name}'. Cannot restore.") - RETURN() ENDIF() GET_PROPERTY(originally_defined GLOBAL PROPERTY CACHE_VAR_WAS_DEFINED_${var_name}) IF(originally_defined) + GET_PROPERTY(original_value_exists GLOBAL PROPERTY CACHE_VAR_ORIGINAL_${var_name} SET) + IF(NOT original_value_exists) + MESSAGE(FATAL_ERROR "Original value property missing for cache variable '${var_name}'.") + ENDIF() GET_PROPERTY(original_value GLOBAL PROPERTY CACHE_VAR_ORIGINAL_${var_name}) SET(${var_name} "${original_value}" CACHE STRING "Restored after test" FORCE) ELSE() UNSET(${var_name} CACHE) ENDIF()test/DEPENDENCY/download_check/git_revision_validation/revision_specific_files/CMakeLists.txt (1)
22-44: Consider improving error context in failure messages.The error messages could provide more context about which branch and test case failed, making debugging easier.
IF(NOT EXISTS "${output_directory}") - MESSAGE(FATAL_ERROR "Directory does not exist: ${output_directory}") + MESSAGE(FATAL_ERROR "Directory does not exist for revision '${git_revision}': ${output_directory}") ENDIF() FOREACH(file_path IN LISTS expected_files) SET(full_file_path "${output_directory}/${file_path}") IF(NOT EXISTS "${full_file_path}") - MESSAGE(FATAL_ERROR "Expected file does not exist in ${git_revision}: ${full_file_path}") + MESSAGE(FATAL_ERROR "Expected file does not exist in revision '${git_revision}' (${keywords_suffix}): ${full_file_path}") ENDIF() ENDFOREACH()test/DEPENDENCY/download_check/git_revision_validation/master_branch_download/CMakeLists.txt (1)
50-51: Global cache deletion risks test interference
FILE(REMOVE_RECURSE "${CMLIB_REQUIRED_ENV_TMP_PATH}")wipes the entire cache dir.
When the test suite is run in parallel (e.g.,ctest -j), concurrent tests depending on the same cache explode.Consider scoping the cleanup to the directory returned in
output_directoryinstead of nuking the root, or gate the removal behind a mutex/unique sub-dir for this test.README.md (2)
22-25: Use an absolute path when exportingCMLIB_DIR
export CMLIB_DIR=$(PWD)/cmakelibwill evaluate at shell start-up, not when the line is written, and therefore points to whatever directory the user happens to be in. Capture the absolute path instead:-echo "export CMLIB_DIR=$(PWD)/cmakelib" >> .bashrc +echo "export CMLIB_DIR=$HOME/cmakelib/cmakelib" >> ~/.bashrc
51-58: Several minor grammar fixes-However it is not recommanded. +However, it is not recommended. -Library is stored on User computer +The library is stored on the user's computerThese tweaks improve readability; feel free to batch-apply with other copy-editing passes.
test/DEPENDENCY/download_check/fail/CMakeLists.txt (1)
78-78: Remove debug message from production code.The STATUS message appears to be leftover debugging code and should be removed.
-MESSAGE(STATUS "${dir_uri}")system_modules/CMLIB_FILE_DOWNLOAD.cmake (1)
270-285: Consider enhancing error reporting for different URI types.The current error message doesn't distinguish between HTTP and FILE download failures. Consider including the URI type in the debug message for better diagnostics.
IF(NOT (status EQUAL 0)) - _CMLIB_LIBRARY_DEBUG_MESSAGE("Download from '${__URI}' failed: ${download_status_list}") + _CMLIB_LIBRARY_DEBUG_MESSAGE("Download from ${__URI_TYPE} URI '${__URI}' failed: ${download_status_list}") ENDIF()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (67)
.gitignore(1 hunks)README.md(6 hunks)example/DEPENDENCY/cache_control_disabled/CMakeLists.txt(1 hunks)example/DEPENDENCY/cache_control_enabled/CMakeLists.txt(1 hunks)example/FILE_DOWNLOAD/CMakeLists.txt(1 hunks)system_modules/CMLIB_COMPONENT.cmake(4 hunks)system_modules/CMLIB_DEPENDENCY.cmake(2 hunks)system_modules/CMLIB_FILE_DOWNLOAD.cmake(14 hunks)system_modules/CMLIB_PARSE_ARGUMENTS.cmake(2 hunks)test/CMakeLists.txt(1 hunks)test/DEPENDENCY/CMakeLists.txt(1 hunks)test/DEPENDENCY/README.md(1 hunks)test/DEPENDENCY/argument_missing/.gitignore(0 hunks)test/DEPENDENCY/argument_missing/CMakeLists.txt(1 hunks)test/DEPENDENCY/cache_control/CMakeLists.txt(1 hunks)test/DEPENDENCY/cache_control/keywords/CMakeLists.txt(1 hunks)test/DEPENDENCY/cache_control/keywords_same_file_diff_remote/CMakeLists.txt(2 hunks)test/DEPENDENCY/cache_control/keywords_with_diff_remote/CMakeLists.txt(1 hunks)test/DEPENDENCY/cache_control/keywords_with_diff_revision/CMakeLists.txt(1 hunks)test/DEPENDENCY/cache_control/mixed_keywords_first/CMakeLists.txt(1 hunks)test/DEPENDENCY/cache_control/mixed_no_keywords_first/CMakeLists.txt(1 hunks)test/DEPENDENCY/cache_control/same_file_diff_remotes/CMakeLists.txt(1 hunks)test/DEPENDENCY/cache_control/same_file_diff_revision/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/file_uri_directory/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/unreachable_uris/file_uri_unreachable/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/unreachable_uris/git_uri_unreachable_git/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/unreachable_uris/git_uri_unreachable_https/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/unreachable_uris/git_uri_unreachable_ssh/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/unreachable_uris/http_uri_unreachable/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/uri_type_mismatches/file_with_git/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/uri_type_mismatches/file_with_http/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/uri_type_mismatches/git_with_file/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/uri_type_mismatches/http_with_file/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/uri_type_mismatches/http_with_git/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/file_uri_download/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/git_revision_validation/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/git_revision_validation/master_branch_download/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/git_revision_validation/revision_specific_files/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/git_revision_validation/test_branch_download/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/git_uri_download/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/http_uri_download/CMakeLists.txt(1 hunks)test/DEPENDENCY/type_check/CMakeLists.txt(1 hunks)test/DEPENDENCY/type_check/type_archive/CMakeLists.txt(1 hunks)test/DEPENDENCY/type_check/type_directory/CMakeLists.txt(1 hunks)test/DEPENDENCY/type_check/type_file/CMakeLists.txt(1 hunks)test/DEPENDENCY/type_check/type_module/CMakeLists.txt(1 hunks)test/FILE_DOWNLOAD/CMakeLists.txt(1 hunks)test/FILE_DOWNLOAD/compute_hash/CMakeLists.txt(3 hunks)test/PARSE_ARGUMENTS/CMakeLists.txt(1 hunks)test/PARSE_ARGUMENTS/test_cases/argument_types/CMakeLists.txt(1 hunks)test/PARSE_ARGUMENTS/test_cases/basic_functionality/CMakeLists.txt(1 hunks)test/PARSE_ARGUMENTS/test_cases/cleanup/CMakeLists.txt(1 hunks)test/PARSE_ARGUMENTS/test_cases/cleanup/cleanup_test_macros.cmake(1 hunks)test/PARSE_ARGUMENTS/test_cases/complex_scenarios/CMakeLists.txt(1 hunks)test/PARSE_ARGUMENTS/test_cases/error_conditions/CMakeLists.txt(1 hunks)test/PARSE_ARGUMENTS/test_cases/error_conditions/invalid_combinations/CMakeLists.txt(1 hunks)test/PARSE_ARGUMENTS/test_cases/error_conditions/malformed_input/CMakeLists.txt(1 hunks)test/PARSE_ARGUMENTS/test_cases/error_conditions/missing_required/CMakeLists.txt(1 hunks)test/README.md(1 hunks)test/TEST.cmake(1 hunks)test/cache_var.cmake(1 hunks)test/test_resources/.gitignore(1 hunks)test/test_resources/README.md(1 hunks)test/test_resources/test_resources.cmake(1 hunks)test/transform_git_uri.cmake(1 hunks)
💤 Files with no reviewable changes (1)
- test/DEPENDENCY/argument_missing/.gitignore
🧰 Additional context used
🪛 LanguageTool
test/README.md
[uncategorized] ~8-~8: A comma might be missing here.
Context: ...nd only if all tests pass! If one test fails the CMlib does not work as expected! Ev...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[style] ~8-~8: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 295 characters long)
Context: ...nctionality! Whole consistency is needed!
(EN_EXCESSIVE_EXCLAMATION)
test/DEPENDENCY/README.md
[uncategorized] ~4-~4: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...DEPENDENCY Unit Tests DEPENDENCY tests does not test every possible argument combin...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[style] ~11-~11: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...e_control/ - verify that DEPENDENCY correctly manages the cache -type_check/` ...
(ADVERB_REPETITION_PREMIUM)
test/test_resources/README.md
[duplication] ~11-~11: Possible typo: you repeated a word.
Context: ..._RESOURCES_DOWNLOAD_DISABLE()` are used to to control if the download is allowed or n...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~11-~11: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal).
Context: ...NLOAD_DISABLE()` are used to to control if the download is allowed or not. - Use T...
(IF_WHETHER)
README.md
[uncategorized] ~10-~10: A determiner appears to be missing. Consider inserting it.
Context: ...stency and project regeneration times. Main features are - No other dependencies. ...
(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~51-~51: You might be missing the article “a” here.
Context: ...owever it is possible use cmakelib as submodule and use ADD_SUBDIRECTORY. However it ...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~51-~51: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...s submodule and use ADD_SUBDIRECTORY. However it is not recommanded. #### Global ins...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~54-~54: A determiner appears to be missing. Consider inserting it.
Context: ... not recommanded. #### Global install Library is stored on User computer and the glob...
(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~55-~55: Possible missing article found.
Context: ...## Global install Library is stored on User computer and the global environment var...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~58-~58: Consider a more concise word here.
Context: ...IRvariable is named as CMake requires in order to FIND_PACKAGE to find a CMLIB by aFIND...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~65-~65: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...makelib/cmakelib.git ``` To make CMLIB works add following lines to your .bashrc ``...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~65-~65: A comma might be missing here.
Context: ...b/cmakelib.git To make CMLIB works add following lines to your .bashrc bas...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~65-~65: You might be missing the article “the” here.
Context: ...akelib.git To make CMLIB works add following lines to your .bashrc bash echo "ex...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~120-~120: You might be missing the article “an” here.
Context: ...anism Cache entries are represented by ordered, nonempty set of uppercase strings call...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[uncategorized] ~123-~123: You might be missing the article “the” here.
Context: ...ry as a hash of URI, GIT_PATH etc. Cache mechanism is a persistent across CMake ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~125-~125: Possible missing article found.
Context: ...t across CMake binary dir instances. If user deletes our CMake binary dir the cache ...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~125-~125: Possible missing comma found.
Context: ...ances. If user deletes our CMake binary dir the cache will regenerate in next CMake...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~126-~126: You might be missing the article “the” here.
Context: ...binary dir the cache will regenerate in next CMake run for the same CMakeLists.txt (...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~129-~129: Possible missing comma found.
Context: ...If the CMLIB_REQUIRED_ENV_TMP_PATH is set then the cache will be stored in the di...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~140-~140: Possible missing comma found.
Context: ...ly one dependency cached at a time. If ON we cannot track the same dependency und...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~153-~153: You might be missing the article “a” here.
Context: ...REQUIRED_ENV_TMP_PATH` User can define global ENV var to specify exactly one, central...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~158-~158: You might be missing the article “an” here.
Context: ...s - Each cache entry is represented by ordered set of keywords. It's common idiom the ...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[uncategorized] ~159-~159: You might be missing the article “a” here.
Context: ...sented by ordered set of keywords. It's common idiom the the first keyword is name of ...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[duplication] ~159-~159: Possible typo: you repeated a word.
Context: ...ered set of keywords. It's common idiom the the first keyword is name of the project in...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~159-~159: You might be missing the article “the” here.
Context: ...s common idiom the the first keyword is name of the project in which the CMLIB_DEPEN...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 markdownlint-cli2 (0.17.2)
test/DEPENDENCY/README.md
13-13: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test_windows_script
- GitHub Check: test_windows
🔇 Additional comments (71)
.gitignore (1)
6-6: Verify blanket exclusion of all zip archives.
**/*.zipwill ignore every.zipin the tree, including any intentionally-checked-in sample archives.
If the intent is to skip only build/test artefacts, consider scoping the rule (e.g.test/**.zipor/build/**.zip) to avoid accidental losses.example/DEPENDENCY/cache_control_enabled/CMakeLists.txt (1)
17-20: ConfirmCMLIB_REQUIRED_ENV_REMOTE_URLexpands to a bare host URL before appending path.The macro call relies on
URI "${CMLIB_REQUIRED_ENV_REMOTE_URL}/cmakelib-test/archive/v1.0.0.zip"If
CMLIB_REQUIRED_ENV_REMOTE_URLalready ends with the repository path (or even with.git), the concatenation may yield a malformed URL such ashttps://gitlab.com/cmakelib/cmakelib.git/cmakelib-test/....Please double-check expected values in CI and local environments and adjust with
string(REGEX REPLACE ...)or a helper to strip a trailing repo segment before appending the archive path.test/DEPENDENCY/cache_control/same_file_diff_revision/CMakeLists.txt (1)
16-21: Fail-fast when required environment variable is missing
TRANSFORM_GIT_URIwill silently receive an emptyURIifCMLIB_REQUIRED_ENV_REMOTE_URLis unset, producing a confusing error later inCMLIB_DEPENDENCY.
Add an explicit guard:+if(NOT DEFINED CMLIB_REQUIRED_ENV_REMOTE_URL) + message(FATAL_ERROR "CMLIB_REQUIRED_ENV_REMOTE_URL is not defined; cannot derive remote URI") +endif()test/DEPENDENCY/cache_control/keywords_with_diff_remote/CMakeLists.txt (1)
16-21: Fail fast when the remote-URL environment variable is missing
TRANSFORM_GIT_URIwill silently produce an emptyGIT_DOWNLOAD_URIifCMLIB_REQUIRED_ENV_REMOTE_URLis not set, leading to confusing downstream errors. Add an explicit guard:+if(NOT DEFINED CMLIB_REQUIRED_ENV_REMOTE_URL OR "${CMLIB_REQUIRED_ENV_REMOTE_URL}" STREQUAL "") + message(FATAL_ERROR "CMLIB_REQUIRED_ENV_REMOTE_URL is not defined – cannot form download URI") +endif()test/DEPENDENCY/download_check/fail/unreachable_uris/http_uri_unreachable/CMakeLists.txt (1)
1-21: LGTM! Well-structured negative test case.The test correctly sets up a failure scenario with an intentionally unreachable HTTP URI. The use of
nonexistent.domain.invalidis appropriate for testing unreachable URIs, and the test structure follows good practices for negative testing.test/TEST.cmake (1)
74-78: LGTM! Well-designed utility macro.The
TEST_VAR_EQUALS_LITERALmacro is a good addition that:
- Provides a clean interface for testing variable equality with literals
- Properly handles temporary variable cleanup with
UNSET- Follows the established patterns in the file
- Simplifies test code by avoiding manual temporary variable management
test/PARSE_ARGUMENTS/test_cases/error_conditions/missing_required/CMakeLists.txt (1)
1-15: LGTM! Proper negative test case for missing required arguments.The test correctly:
- Sets up the test environment with proper CMake project structure
- Uses
CMLIB_PARSE_ARGUMENTSwith a required argument (TEST_VALUE)- Intentionally omits the required argument to trigger a
FATAL_ERROR- Includes clear documentation explaining the expected failure behavior
This aligns well with the modular test structure described in the AI summary.
test/DEPENDENCY/cache_control/keywords_same_file_diff_remote/CMakeLists.txt (1)
17-26: LGTM! Excellent improvement in URI handling.The replacement of hardcoded Git URIs with dynamic generation using
TRANSFORM_GIT_URIandTRANSFORM_GIT_URI_TO_HTTPSfunctions is a significant improvement that:
- Enhances test maintainability by centralizing URI transformations
- Provides flexibility through environment variable configuration (
CMLIB_REQUIRED_ENV_REMOTE_URL)- Supports consistent URI handling across the test suite
- Enables testing with different URI formats (Git vs HTTPS)
This change aligns well with the broader test infrastructure improvements described in the AI summary.
test/DEPENDENCY/download_check/CMakeLists.txt (1)
12-21: Download-check orchestrator looks goodThe orchestration is clean and paths are correct. No issues spotted.
test/FILE_DOWNLOAD/CMakeLists.txt (1)
15-23: Verify presence of CMLIB_REQUIRED_ENV_REMOTE_URL or add a fallbackThe calls to
TRANSFORM_GIT_URI_TO_HTTPSandTRANSFORM_GIT_URIassume
CMLIB_REQUIRED_ENV_REMOTE_URLis always defined. If it isn’t exported (e.g. running tests locally), CMake will error out.Locations to update:
- test/FILE_DOWNLOAD/CMakeLists.txt (lines 15–23)
Suggested guard (add before any use of the variable):
if(NOT DEFINED CMLIB_REQUIRED_ENV_REMOTE_URL) message(WARNING "CMLIB_REQUIRED_ENV_REMOTE_URL not set – falling back to public GitHub mirror.") set(CMLIB_REQUIRED_ENV_REMOTE_URL "https://github.com") endif()Please verify that either the variable is always initialized by your CMLIB setup or include this fallback.
test/DEPENDENCY/download_check/git_revision_validation/CMakeLists.txt (1)
16-18: LGTM – clear, self-contained test invocationsThe three
TEST_RUNcalls cover the intended scenarios and the paths are already quoted.
No issues spotted here.test/PARSE_ARGUMENTS/test_cases/error_conditions/invalid_combinations/CMakeLists.txt (1)
11-16: LGTM – negative test correctly probes invalid argument mixUsing the same identifier in
OPTIONSandONE_VALUEforces a fatal error, providing good coverage for category-collision handling.test/PARSE_ARGUMENTS/test_cases/error_conditions/malformed_input/CMakeLists.txt (1)
11-15: Negative test looks good – no issues spottedThe macro is intentionally invoked with an invalid parameter type to ensure the parser rejects malformed input. Naming, comments and scope setup are clear.
test/DEPENDENCY/cache_control/keywords/CMakeLists.txt (1)
17-22: LGTM! Good refactoring to use standardized URI transformation.The change from direct URI assignment to using the
TRANSFORM_GIT_URImacro improves consistency across the test suite and makes URI handling more maintainable.test/DEPENDENCY/download_check/http_uri_download/CMakeLists.txt (2)
26-28: LGTM! Proper validation of download success.The file existence check is a good way to verify that the HTTP download completed successfully.
15-18: No changes needed—HTTP URIs are passed through unchanged by the macro.I verified that TRANSFORM_GIT_URI_TO_HTTPS first checks for an HTTP(S) URI and immediately returns it as-is. The ZIP-archive URL in the test will be forwarded unchanged, so the conversion logic works correctly for archive downloads.
test/DEPENDENCY/type_check/type_archive/CMakeLists.txt (1)
40-46: LGTM! Proper validation of archive extraction.The verification of extracted files is thorough and appropriate for testing archive functionality.
system_modules/CMLIB_DEPENDENCY.cmake (3)
35-39: LGTM! Excellent documentation of design principles.The added comments clearly explain the two-step design pattern, which will help with maintenance and debugging.
176-180: LGTM! Much improved error messaging.The enhanced error message provides clear diagnostic information including possible causes and the problematic URI, which will significantly help with troubleshooting.
185-189: LGTM! Consistent and informative error reporting.The error message follows the same improved pattern as the directory error case, providing comprehensive troubleshooting information.
test/DEPENDENCY/download_check/fail/unreachable_uris/git_uri_unreachable_ssh/CMakeLists.txt (1)
15-22: LGTM! Well-designed failure test.The test correctly uses an unreachable domain (
.invalidTLD) and properly exercises the SSH Git URI failure path. The test structure is clean and the expected failure is clearly documented.test/DEPENDENCY/download_check/fail/unreachable_uris/git_uri_unreachable_git/CMakeLists.txt (1)
17-22: Scheme/TYPE mismatch could hide the true failure reasonThe test claims to verify “unreachable Git URI” but declares
TYPE FILEwhile the URI uses thegit://scheme.
IfCMLIB_DEPENDENCYvalidates theTYPE ↔︎ schemerelationship before attempting any network operation, the test will fail for a type-scheme mismatch rather than for an unreachable host, thereby weakening the assertion.Consider explicitly using the correct TYPE (e.g.
TYPE DIRECTORYorTYPE FILEonly after the repo is cloned locally), or add a comment clarifying that the mismatch is intentional and relied upon.
This keeps the intent of the test unambiguous to future readers.test/DEPENDENCY/type_check/type_module/CMakeLists.txt (1)
28-33: Potential double-inclusion riskYou
INCLUDE(TEST_MODULE)right after the DEPENDENCY call.
If the DEPENDENCY macro already performs ainclude()internally forTYPE MODULE, this explicit include might execute the module twice.
Verify the macro’s behaviour or guard with:if(NOT TARGET TEST_MODULE) # or another suitable check include(TEST_MODULE) endif()system_modules/CMLIB_COMPONENT.cmake (1)
142-150: LGTM - Good separation of concerns.The integration of revision management into the component download logic is well-implemented. The revision is only used when not using a local base path, which is the correct behavior.
test/cache_var.cmake (1)
1-80: Excellent utility for test isolation.This cache variable management utility is well-designed for ensuring test isolation. The implementation correctly handles both the existence state and value of cache variables.
test/DEPENDENCY/download_check/git_revision_validation/revision_specific_files/CMakeLists.txt (1)
1-62: Excellent test design for revision validation.This test effectively validates that the
GIT_REVISIONparameter correctly switches between branches by verifying branch-specific files. The approach of testing different file sets for different branches is robust.test/PARSE_ARGUMENTS/CMakeLists.txt (2)
2-22: Excellent refactoring to modular test structure.The refactoring from inline tests to modular external test scripts greatly improves maintainability and test organization. The comprehensive test coverage includes basic functionality, argument types, error conditions, complex scenarios, and cleanup behavior.
16-16: TEST.cmake dependency validatedThe included file
test/TEST.cmakeexists and defines the requiredTEST_RUNfunction, so theINCLUDE("${CMAKE_CURRENT_LIST_DIR}/../TEST.cmake")statement is correct and no further changes are needed.system_modules/CMLIB_PARSE_ARGUMENTS.cmake (3)
7-26: Excellent documentation improvements.The enhanced documentation significantly improves the usability of the
CMLIB_PARSE_ARGUMENTSmacro. The detailed parameter descriptions, behavior explanations, and default value clarifications make the macro much more accessible to users.
64-89: Great addition of cleanup macro.The
CMLIB_PARSE_ARGUMENTS_CLEANUPmacro is an excellent addition that addresses variable pollution issues, especially in macro contexts. The documentation clearly explains when and why to use it.
76-89: Cleanup macro completeness verifiedAll internal variables introduced by
CMLIB_PARSE_ARGUMENTS—namely
options, one_value_args, multi_value_args,_tmp,_tmp_PREFIX,_tmp_P_ARGN,_tmp_REQUIRED,_tmp_OPTIONS,_tmp_ONE_VALUE,_tmp_MULTI_VALUE,opt, andreq—are unset byCMLIB_PARSE_ARGUMENTS_CLEANUP. The only variables not cleared are the dynamic result variables (${_tmp_PREFIX}_${opt}), which are the intended outputs of the parser.Optional: you may add a brief comment above the cleanup macro listing each variable’s role for future maintainers.
test/DEPENDENCY/download_check/git_revision_validation/master_branch_download/CMakeLists.txt (1)
23-30: Verify empty-string handling in CMLIB_DEPENDENCYI wasn’t able to find the
CMLIB_DEPENDENCYdefinition to confirm whether passingGIT_REVISION ""still counts as specifying a revision. If the implementation treats the mere presence of the keyword (even with an empty value) as “revision provided,” your default-master branch path won’t be exercised.Please check:
- test/DEPENDENCY/download_check/git_revision_validation/master_branch_download/CMakeLists.txt – the current
CMLIB_DEPENDENCY(... GIT_REVISION "${git_revision}")invocation- The
CMLIB_DEPENDENCYmacro/function in your CMake modules to see how it handles an emptyGIT_REVISIONvalueIf it does treat empty strings as explicit, apply the suggested refactor to only append
GIT_REVISIONwhengit_revisionis non-empty.test/PARSE_ARGUMENTS/test_cases/cleanup/CMakeLists.txt (1)
20-22: Good smoke-test for memory leaks
TEST_MEMORY_MANAGEMENT()correctly exercisesCMLIB_PARSE_ARGUMENTS_CLEANUPwithout touching global state. No issues spotted.test/PARSE_ARGUMENTS/test_cases/error_conditions/CMakeLists.txt (1)
42-49: Empty-specification test never asserts anything
TEST_EMPTY_SPECIFICATIONSparses nothing and does not check outcome/error. Decide whether:
- You expect success → add assertions (e.g., variables not defined).
- You expect failure → convert to
TEST_INVALID_CMAKE_RUN.At the moment it is a no-op.
test/PARSE_ARGUMENTS/test_cases/argument_types/CMakeLists.txt (1)
20-31: Behaviour of explicitOFFoptions depends on implementationIn
TEST_OPTIONS_VALIDATIONyou treatOPT_OFF/OPT_FALSEpassed explicitly asOFFas false options.
CMLIB_PARSE_ARGUMENTSmay instead take the presence of the name inP_ARGNas enable regardless of value, mirroring CMake’s standardcmake_parse_arguments.Double-check the macro; if it follows upstream behaviour these assertions will fail.
# Expected macro behaviour if it mirrors cmake_parse_arguments: # __OPT_OFF TRUEtest/DEPENDENCY/download_check/fail/CMakeLists.txt (2)
20-77: LGTM: Comprehensive error message template setup.The template expansion logic for generating expected error messages is well-structured and covers all the necessary failure scenarios including unreachable URIs and URI type mismatches.
81-109: LGTM: Complete test coverage for failure scenarios.The test execution covers all the defined failure scenarios systematically, providing comprehensive validation of error handling for different URI schemes and type mismatches.
test/PARSE_ARGUMENTS/test_cases/cleanup/cleanup_test_macros.cmake (3)
1-41: Excellent comprehensive test documentation.The detailed documentation clearly explains the test procedure, validation criteria, and error detection mechanisms. This level of documentation is exemplary for test code.
64-109: Robust variable tracking and cleanup validation.The variable comparison logic is thorough and handles edge cases well:
- Correctly identifies new variables created by parsing
- Separates parsed variables from internal variables
- Validates cleanup effectiveness with proper error reporting
- Handles test-specific variables appropriately
The regex pattern matching for exclusions is a good approach to avoid false positives.
118-125: Excellent reusability validation.Testing that subsequent parsing operations work correctly after cleanup is crucial for validating the cleanup functionality's completeness and ensuring no state corruption occurs.
test/PARSE_ARGUMENTS/test_cases/complex_scenarios/CMakeLists.txt (5)
18-37: LGTM: Well-structured test for spaces and quotes handling.The test correctly validates that arguments containing spaces are handled properly, including multi-value arguments with mixed content. The validation logic is appropriate.
39-57: LGTM: Comprehensive special characters validation.The test covers realistic special characters that would commonly appear in file paths, email addresses, and version strings. Good coverage of edge cases.
59-79: LGTM: Stress test with long argument lists.Testing with 50 items provides good coverage for performance and memory handling. The validation of specific list positions ensures the entire list is processed correctly.
114-153: LGTM: Comprehensive variable substitution testing.The test covers both regular CMake variables and cache variables, which is essential for validating real-world usage patterns. The validation of list expansion and individual items is thorough.
155-159: LGTM: Complete test execution.All test functions are properly invoked, ensuring comprehensive coverage of complex scenarios.
test/DEPENDENCY/download_check/git_uri_download/CMakeLists.txt (3)
20-27: LGTM: Proper URI transformation setup.The transformation of git URIs to both standard and HTTPS variants ensures comprehensive testing of different git protocols.
31-56: LGTM: Comprehensive download test macro.The macro properly:
- Tests file download functionality
- Validates file existence
- Ensures proper module loading
- Executes downloaded test code
- Cleans up cache variables and files
The cleanup is particularly important for test isolation.
58-70: LGTM: Proper test isolation with cache variable management.Both test functions properly save and restore cache variable state, ensuring tests don't interfere with each other. Testing both git archive modes provides comprehensive coverage.
test/test_resources/test_resources.cmake (3)
39-54: LGTM: Robust download implementation with proper error handling.The download function properly checks the enable flag and provides clear error messages for failure scenarios. The use of CMLIB_FILE_DOWNLOAD for git repository cloning is appropriate.
95-103: LGTM: Proper file existence validation and URI generation.The function correctly validates file existence before generating file:// URIs, which prevents runtime errors in tests. The use of absolute paths ensures consistent URI generation.
64-79: LGTM: Clean cache variable management for download control.The enable/disable functions provide clean control over download behavior, which is essential for test isolation and preventing unintended downloads during test execution.
test/PARSE_ARGUMENTS/test_cases/basic_functionality/CMakeLists.txt (3)
52-75: LGTM! Good edge case coverage.The function effectively tests important edge cases including numeric prefixes and long prefix strings. The variable expansion approach for the long prefix is well-implemented.
77-109: LGTM! Comprehensive argument parsing coverage.The function effectively tests all major argument types (OPTIONS, ONE_VALUE, MULTI_VALUE) and includes proper verification of list contents. The test logic is well-structured and thorough.
113-115: LGTM! Test execution is properly structured.All test functions are called appropriately to execute the complete test suite.
test/transform_git_uri.cmake (4)
66-100: LGTM! SSH to HTTPS transformation is well-implemented.The function correctly handles SSH URI transformation and includes proper validation and error handling.
158-162: Update test case to reflect correct port handling.This test case will fail due to the port handling issue in
TRANSFORM_GIT_URI. After fixing the port handling, this test should expect the hostname without the port in the SSH URI format.The expected result should be
git@git.example.com:user/repo.git(without the port in SSH format) since SSH URIs don't typically include port numbers in the hostname part.- TEST_VAR_EQUALS_LITERAL(result "git@git.example.com:8080:user/repo.git") + TEST_VAR_EQUALS_LITERAL(result "git@git.example.com:user/repo.git")
172-226: LGTM! Comprehensive SSH to HTTPS test coverage.The test function provides excellent coverage of SSH to HTTPS transformation scenarios, including edge cases and various URI formats.
228-229: LGTM! Test execution is properly structured.Both test functions are called appropriately to execute the complete test suite.
test/DEPENDENCY/CMakeLists.txt (1)
10-14: LGTM! Good modularization of test structure.The simplified approach with dedicated test scripts (
download_check,type_check,argument_missing) improves maintainability and makes the test suite more modular. The error message regex pattern in theTEST_INVALID_CMAKE_RUNcall is appropriate.test/DEPENDENCY/cache_control/CMakeLists.txt (3)
18-25: LGTM! Dynamic URI generation improves test flexibility.The use of
transform_git_uri.cmaketo dynamically generate both HTTPS and SSH URIs from environment variables is a good improvement over hardcoded URIs. This makes tests more adaptable to different environments.
49-56: LGTM! Template system improves error message maintainability.The introduction of
VERSION_MISMATCH_TEMPLATEandCMLIB_TEMPLATE_EXPANDis a good approach to reduce duplication and improve consistency in error message definitions.
26-45: LGTM! Regex patterns improve test robustness.The use of regex patterns (e.g.,
.*) in error messages allows for more flexible matching and makes tests more resilient to minor changes in error message formatting.test/DEPENDENCY/type_check/type_directory/CMakeLists.txt (4)
25-51: LGTM! Comprehensive directory download testing.The macro effectively tests git directory downloads with proper validation of downloaded content, module execution, and cache cleanup. The file existence checks are thorough and appropriate.
53-86: LGTM! Thorough subdirectory download testing.The macro comprehensively tests git subdirectory downloads using the
GIT_PATHparameter. The file and directory existence checks are thorough and validate the expected subdirectory structure.
88-110: LGTM! Proper cache variable management.The test functions effectively use
CACHE_VAR_FORCE_SETandCACHE_VAR_RESTOREto safely manage theCMLIB_FILE_DOWNLOAD_GIT_ARCHIVE_DISABLEcache variable, ensuring isolated test execution for both enabled and disabled archive scenarios.
114-117: LGTM! Comprehensive test execution.All test functions are called appropriately to provide complete coverage of directory and subdirectory downloads with both archive enabled and disabled scenarios.
system_modules/CMLIB_FILE_DOWNLOAD.cmake (5)
137-140: Good addition of absolute path validation.The check ensures OUTPUT_PATH is absolute, preventing potential issues with relative paths. The error message clearly indicates the problematic path.
177-186: Clean unification of HTTP and FILE URI handling.Good design decision to handle both HTTP and FILE URIs through the same standard URI function, reducing code duplication.
580-593: URI type detection correctly extended for FILE URIs.The implementation follows the established pattern and correctly identifies file:// URIs.
54-101: Comprehensive documentation for FILE URI support.The documentation clearly explains the behavior and requirements for all URI types, including the new FILE support. The prerequisite checks section is particularly helpful.
456-481: Security validation for FILE URIs is properly implemented.The prerequisite checks enforce absolute paths and validate file types, which helps prevent path traversal and other security issues.
test/DEPENDENCY/download_check/fail/uri_type_mismatches/http_with_git/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/DEPENDENCY/download_check/file_uri_download/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/DEPENDENCY/download_check/fail/unreachable_uris/file_uri_unreachable/CMakeLists.txt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/FILE_DOWNLOAD/CMakeLists.txt (2)
21-35: Inconsistent URI transformation helpers – risk of silent divergence
TRANSFORM_GIT_URI_TO_HTTPS()is used forVALID_GIT_URI, but the fallback for
VALID_GIT_URI_ARCHIVErelies on a different helperTRANSFORM_GIT_URI.
If the two helpers ever diverge (e.g. one adds query-string stripping), the two
URIs will no longer be equivalent, complicating reproducibility.Unify the logic:
-TRANSFORM_GIT_URI( - URI "${CMLIB_REQUIRED_ENV_REMOTE_URL}/cmakelib-test.git" - OUTPUT_VAR VALID_GIT_URI_ARCHIVE -) +TRANSFORM_GIT_URI_TO_HTTPS( + URI "${CMLIB_REQUIRED_ENV_REMOTE_URL}/cmakelib-test.git" + OUTPUT_VAR VALID_GIT_URI_ARCHIVE +)or document the semantic difference between the two macros.
320-327: Unquoted variables can break paths containing spacesInside
GIT_DOWNLOAD_BY_ARCHIVEthe status check is fine, but other file
operations below use unquoted variables (FILE(GLOB_RECURSE glob "${output_path}/*")is OK, but elsewhereFILE(REMOVE ${tmp})is unquoted).
While not introduced in this hunk, new paths are now derived from
${output_path}– quoting them avoids latent breakage:-FILE(REMOVE ${tmp}) +FILE(REMOVE "${tmp}")Search & fix similar occurrences while you are in this file.
test/README.md (1)
2-16: Spelling, capitalisation, and minor style clean-ups-# CMAKELIB Test Suite - -Comprehensive test suite for the CMAKELIB library. - -CMlib is consistent and working if and only if all tests pass! - -If one test fails the CMlib does not work as expected! Even if the failure is not directly related to the needed functionality! Whole consistency is needed to be sure the system works as expected! - -## Known Problems - -- git archive funcionality is not tested on Github because Github does not support it. - - The gitlab [fork of cmakelib-test] repo is used instead as a resource for testing `git archive`. - - The git uri to the gitlab repo is hardcoded in the `FILE_DOWNLOAD/CMakeLists.txt` test. - - To disable the test set `_CMLIB_TEST_GIT_ARCHIVE` to `OFF` manually in the `FILE_DOWNLOAD/CMakeLists.txt` before running the test. - - **The test is enabled by default**. +# CMAKELIB Test Suite + +Comprehensive test suite for the **CMAKELIB** library. + +CMAKELIB is considered *fully functional only when **all** tests pass*. + +If **any** test fails, the library must be regarded as inconsistent – even when +the failing test appears unrelated to the feature you are interested in. + +## Known problems + +- **`git archive` functionality** is not exercised on GitHub because GitHub + does not support the command. + - A GitLab [fork of *cmakelib-test*] is used instead. + - The GitLab URI is hard-coded in `test/FILE_DOWNLOAD/CMakeLists.txt`. + - You can disable this test by setting `_CMLIB_TEST_GIT_ARCHIVE` to `OFF` + before running CTest. + - **The test is enabled by default.**Fixes:
– “funcionality” → “functionality”
– consistent “GitHub”/“GitLab” capitalisation
– uniform “CMAKELIB” spelling
– clearer wording and paragraph separation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
system_modules/CMLIB_FILE_DOWNLOAD.cmake(16 hunks)test/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/file_uri_download/CMakeLists.txt(1 hunks)test/DEPENDENCY/type_check/type_directory/CMakeLists.txt(1 hunks)test/FILE_DOWNLOAD/CMakeLists.txt(3 hunks)test/README.md(1 hunks)test/test_resources/test_resources.cmake(1 hunks)test/transform_git_uri.cmake(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- test/CMakeLists.txt
- test/DEPENDENCY/download_check/file_uri_download/CMakeLists.txt
- test/transform_git_uri.cmake
- test/test_resources/test_resources.cmake
- test/DEPENDENCY/type_check/type_directory/CMakeLists.txt
- system_modules/CMLIB_FILE_DOWNLOAD.cmake
🧰 Additional context used
🪛 LanguageTool
test/README.md
[grammar] ~1-~1: Use correct spacing
Context: # CMAKELIB Test Suite Comprehensive test suite fo...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~2-~2: Use correct spacing
Context: # CMAKELIB Test Suite Comprehensive test suite for the CMAKEL...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~4-~4: Use correct spacing
Context: ...mprehensive test suite for the CMAKELIB library. CMlib is consistent and working if and ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~6-~6: Use correct spacing
Context: ...nt and working if and only if all tests pass! If one test fails the CMlib does not wo...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~8-~8: Use a comma after introductory words or phrases
Context: ...nd only if all tests pass! If one test fails the CMlib does not work as expected! Ev...
(QB_NEW_EN_OTHER_ERROR_IDS_19)
[grammar] ~8-~8: Use correct spacing
Context: ...s needed to be sure the system works as expected! ## Known Problems - git archive funcional...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~10-~10: Use correct spacing
Context: ...the system works as expected! ## Known Problems - git archive funcionality is not tested ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~12-~12: Ensure spelling is correct
Context: ...cted! ## Known Problems - git archive funcionality is not tested on Github because Github ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~12-~12: Use proper capitalization
Context: ...t archive funcionality is not tested on Github because Github does not support it. -...
(QB_NEW_EN_OTHER_ERROR_IDS_6)
[grammar] ~12-~12: Use proper capitalization
Context: ...onality is not tested on Github because Github does not support it. - The gitlab [fo...
(QB_NEW_EN_OTHER_ERROR_IDS_6)
[grammar] ~13-~13: Use proper capitalization
Context: ...use Github does not support it. - The gitlab [fork of cmakelib-test] repo is used in...
(QB_NEW_EN_OTHER_ERROR_IDS_6)
[grammar] ~14-~14: Use proper capitalization
Context: ...g git archive. - The git uri to the gitlab repo is hardcoded in the `FILE_DOWNLOAD...
(QB_NEW_EN_OTHER_ERROR_IDS_6)
[grammar] ~15-~15: Use a comma after introductory words or phrases
Context: ...MakeLists.txttest. - To disable the test set_CMLIB_TEST_GIT_ARCHIVEtoOFF` ...
(QB_NEW_EN_OTHER_ERROR_IDS_19)
[grammar] ~16-~16: There might be a problem here.
Context: ... the test. - The test is enabled by default. [fork of cmakelib-test]: https://gitlab.com/cmakelib/cmakelib-test
(QB_NEW_EN_MERGED_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test_windows_script
🔇 Additional comments (1)
test/FILE_DOWNLOAD/CMakeLists.txt (1)
363-370: Nice conditional guard for SSH-only testThe opt-in execution of
GIT_DOWNLOAD_BY_ARCHIVE()behind
_CMLIB_TEST_GIT_ARCHIVEis clear and keeps noisy failures away.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/COMPONENT/CMakeLists.txt (2)
6-9: Fix minor typos in header commentsSpelling/grammar tweaks improve readability.
-# Some components are not able top run in script mode -# Lets skip the test then. +# Some components are not able to run in script mode. +# Let's skip the test then.
10-13: Double-check use of top-levelRETURN()
RETURN()(case-insensitive) works only in script mode; ifCMAKE_SCRIPT_MODE_FILEwere ever defined outside that context, CMake would error becausereturn()is illegal at top scope in project mode. While that scenario is unlikely, you can harden the check by additionally testingCMAKE_SCRIPT_MODE_FILE STREQUAL "${CMAKE_CURRENT_LIST_FILE}"or by switching toif (NOT CMAKE_SCRIPT_MODE_FILE)wrapping the remainder.Also, favour the conventional lowercase
return()for consistency with typical CMake style.-MESSAGE(STATUS "Skipping COMPONENT test in script mode") -RETURN() +message(STATUS "Skipping COMPONENT test in script mode") +return()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/COMPONENT/CMakeLists.txt(1 hunks)test/test_resources/test_resources.cmake(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/test_resources/test_resources.cmake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test_macos_11_script
- GitHub Check: test_windows_script
- GitHub Check: test_windows
- GitHub Check: test_macos_11
🔇 Additional comments (1)
test/COMPONENT/CMakeLists.txt (1)
15-17: Unconditional project initialisation looks goodMoving
cmake_minimum_requiredandproject()outside the conditional clarifies intent and guarantees proper policy handling in normal test mode.
koudis
force-pushed
the
jan_kubalek/component_versioning
branch
from
54ede3e to
6be9ec7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/test_resources/test_resources.cmake (1)
94-94: Fix typo in function name.The function name
TEST_RESROUCES_GET_FILE_URIhas a typo and should beTEST_RESOURCES_GET_FILE_URI. This typo appears to be propagated to other test files.-FUNCTION(TEST_RESROUCES_GET_FILE_URI relative_path output_var) +FUNCTION(TEST_RESOURCES_GET_FILE_URI relative_path output_var)system_modules/CMLIB_FILE_DOWNLOAD.cmake (1)
469-481: Add existence check before directory validation for FILE URIs.The current logic checks
IS_DIRECTORYbut doesn't first verify if the path exists. This could lead to confusing behavior sinceIS_DIRECTORYreturns false for non-existent paths.IF(NOT IS_ABSOLUTE "${file_path}") MESSAGE(FATAL_ERROR "Path specified by URI must be absolute for FILE URI_TYPE. Relative path '${uri}' provided.") ENDIF() + IF(NOT EXISTS "${file_path}") + MESSAGE(FATAL_ERROR "File specified by URI does not exist: '${uri}'") + ENDIF() IF(IS_DIRECTORY "${file_path}") MESSAGE(FATAL_ERROR "Path specified by URI must be file for FILE URI_TYPE. Directory '${uri}' provided.") ENDIF()
🧹 Nitpick comments (5)
test/test_resources/test_resources.cmake (3)
27-27: Fix incorrect cache variable description.The cache variable description doesn't match the variable name. The description should refer to "download enabled" rather than "directory".
- CACHE INTERNAL "Test resources directory" + CACHE INTERNAL "Test resources download enabled flag"
44-47: Remove unreachable RETURN() after FATAL_ERROR.The
RETURN()statement afterMESSAGE(FATAL_ERROR ...)is unreachable sinceFATAL_ERRORterminates execution.IF(NOT _TEST_RESOURCES_DOWNLOAD_ENABLED) MESSAGE(FATAL_ERROR "Test resources download disabled by _TEST_RESOURCES_DOWNLOAD_ENABLED = OFF!") - RETURN() ENDIF()
48-57: Improve error handling and validation.The function should validate the environment variable and provide clearer error messages if the download fails.
+IF(NOT DEFINED CMLIB_REQUIRED_ENV_REMOTE_URL) + MESSAGE(FATAL_ERROR "CMLIB_REQUIRED_ENV_REMOTE_URL environment variable is not defined") +ENDIF() SET(test_repo_uri "${CMLIB_REQUIRED_ENV_REMOTE_URL}/cmakelib-test.git") +_CMLIB_LIBRARY_DEBUG_MESSAGE("Downloading test resources from: ${test_repo_uri}") CMLIB_FILE_DOWNLOAD( URI_TYPE GIT URI "${test_repo_uri}" STATUS_VAR download_status OUTPUT_PATH "${_TEST_RESOURCES_DIR}" )system_modules/CMLIB_FILE_DOWNLOAD.cmake (2)
81-81: Fix typo in comment.There's a typo in "revidsion" which should be "revision".
-# from given revidsion (tag, branch, commit id, ...). +# from given revision (tag, branch, commit id, ...).
277-277: Remove trailing whitespace.There's trailing whitespace after the
ENDIF()statement.- ENDIF() + ENDIF()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
system_modules/CMLIB_FILE_DOWNLOAD.cmake(18 hunks)test/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/file_uri_directory/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/unreachable_uris/file_uri_unreachable/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/unreachable_uris/git_uri_unreachable_https/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/unreachable_uris/http_uri_unreachable/CMakeLists.txt(1 hunks)test/FILE_DOWNLOAD/CMakeLists.txt(4 hunks)test/test_resources/test_resources.cmake(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/DEPENDENCY/download_check/fail/unreachable_uris/http_uri_unreachable/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (6)
- test/CMakeLists.txt
- test/DEPENDENCY/download_check/fail/file_uri_directory/CMakeLists.txt
- test/DEPENDENCY/download_check/fail/unreachable_uris/file_uri_unreachable/CMakeLists.txt
- test/DEPENDENCY/download_check/fail/unreachable_uris/git_uri_unreachable_https/CMakeLists.txt
- test/FILE_DOWNLOAD/CMakeLists.txt
- test/DEPENDENCY/download_check/CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test_macos_11_script
- GitHub Check: test_macos_11
- GitHub Check: test_linux_script (test_fedora:41)
- GitHub Check: test_windows_script
- GitHub Check: test_linux_script (test_ubuntu:2004)
- GitHub Check: test_linux_script (test_ubuntu:2404)
- GitHub Check: test_linux_script (test_debian:bookworm)
- GitHub Check: test_linux (test_ubuntu:2404)
- GitHub Check: test_windows
- GitHub Check: test_linux (test_fedora:41)
🔇 Additional comments (3)
system_modules/CMLIB_FILE_DOWNLOAD.cmake (3)
220-225: Improve copy result handling consistency.The copy result handling logic is well-structured and provides appropriate debug messages for failure cases.
591-603: LGTM: FILE URI detection implementation.The FILE URI detection logic is correctly implemented and integrates well with the existing URI type determination.
467-491: Well-structured URI validation function.The new validation function provides comprehensive checks for both FILE and HTTP URI types with appropriate error messages.
DEPENDENCY - tests added Minor updates, forgotten paths, ...
koudis
force-pushed
the
jan_kubalek/component_versioning
branch
from
6be9ec7 to
dea71b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
system_modules/CMLIB_FILE_DOWNLOAD.cmake (2)
54-68: Documentation inconsistency persists for FILE URI_TYPE.The documentation on lines 66-68 still contains the same inconsistency flagged in previous reviews. It states that for FILE URI_TYPE, if OUTPUT_PATH represents a directory, the file will be stored under a random name, but the validation logic at lines 479-481 explicitly forbids directories for FILE URIs.
458-491: Missing existence check for FILE URI validation.The validation logic for FILE URIs checks if the path is a directory but doesn't first verify if the path exists. This could lead to confusing error messages since
IS_DIRECTORYreturns false for non-existent paths.
🧹 Nitpick comments (6)
test/README.md (3)
16-21: Add language hint & replace hard tabsMarkdown-lint flags both issues; fixing them improves readability and CI linting.
-``` +```bash git clean -xfd . - GIT_TERMINAL_PROMPT=0 cmake . + GIT_TERMINAL_PROMPT=0 cmake . git clean -xfd . - GIT_TERMINAL_PROMPT=0 cmake -P ./CMakeLists.txt + GIT_TERMINAL_PROMPT=0 cmake -P ./CMakeLists.txt--- `25-31`: **Same formatting fixes for PowerShell block** ```diff -``` +```powershell - $env:GIT_TERMINAL_PROMPT=0 + $env:GIT_TERMINAL_PROMPT=0…and replace the remaining hard tabs.
41-44: Correct spelling of “functionality” and capitalize Git/GitHub-git archive funcionality is not tested by cmakelib-test Github repository. +`git archive` functionality is not tested by the *cmakelib-test* GitHub repository.README.md (3)
51-55: Typo: “recommanded” → “recommended”-However it is not recommanded. +However, it is not recommended.
160-161: Double word-It's common idiom the the first keyword is name of the project +It's a common idiom that the first keyword is the name of the project
175-176: Spelling: “resouces” → “resources”-resouces +resources
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.md(8 hunks)system_modules/CMLIB_FILE_DOWNLOAD.cmake(18 hunks)test/CMakeLists.txt(1 hunks)test/COMPONENT/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/file_uri_directory/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/unreachable_uris/file_uri_unreachable/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/unreachable_uris/git_uri_unreachable_https/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/unreachable_uris/http_uri_unreachable/CMakeLists.txt(1 hunks)test/FILE_DOWNLOAD/CMakeLists.txt(4 hunks)test/README.md(1 hunks)test/test_resources/README.md(1 hunks)test/test_resources/test_resources.cmake(1 hunks)test/test_resources/test_resources_download.cmake(1 hunks)test/transform_git_uri.cmake(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- test/test_resources/test_resources_download.cmake
- test/DEPENDENCY/download_check/fail/unreachable_uris/http_uri_unreachable/CMakeLists.txt
- test/DEPENDENCY/download_check/fail/file_uri_directory/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (8)
- test/CMakeLists.txt
- test/COMPONENT/CMakeLists.txt
- test/DEPENDENCY/download_check/CMakeLists.txt
- test/DEPENDENCY/download_check/fail/unreachable_uris/git_uri_unreachable_https/CMakeLists.txt
- test/DEPENDENCY/download_check/fail/unreachable_uris/file_uri_unreachable/CMakeLists.txt
- test/test_resources/test_resources.cmake
- test/FILE_DOWNLOAD/CMakeLists.txt
- test/transform_git_uri.cmake
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
test/README.md
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
18-18: Hard tabs
Column: 1
(MD010, no-hard-tabs)
20-20: Hard tabs
Column: 1
(MD010, no-hard-tabs)
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
28-28: Hard tabs
Column: 1
(MD010, no-hard-tabs)
30-30: Hard tabs
Column: 1
(MD010, no-hard-tabs)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 LanguageTool
README.md
[grammar] ~51-~51: Ensure spelling is correct
Context: ...e ADD_SUBDIRECTORY. However it is not recommanded. #### Global install Library is store...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~175-~175: Ensure spelling is correct
Context: ...nctionality is required for downloading resouces from remote git repository. OFF value ha...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
test/README.md
[grammar] ~41-~41: Ensure spelling is correct
Context: .... ``` ## Known Problems - git archive funcionality is not tested by cmakelib-test Github r...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
test/test_resources/README.md
[style] ~11-~11: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal).
Context: ...NLOAD_DISABLE()` are used to to control if the download is allowed or not. - Use T...
(IF_WHETHER)
[grammar] ~13-~13: Ensure spelling is correct
Context: ...iles from downloaded repository. - Test reosurces are downloaded from the cmakelib-test...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test_windows_script
- GitHub Check: test_windows
- GitHub Check: test_macos_11_script
- GitHub Check: test_linux_script (test_ubuntu:2204)
- GitHub Check: test_linux (test_fedora:41)
- GitHub Check: test_linux (test_debian:bullseye)
- GitHub Check: test_linux (test_debian:bookworm)
- GitHub Check: test_macos_11
🔇 Additional comments (10)
system_modules/CMLIB_FILE_DOWNLOAD.cmake (10)
85-101: LGTM: Clear documentation for URI_TYPE prerequisite checks.The added documentation clearly explains the validation requirements for each URI type, which improves code maintainability and user understanding.
137-139: LGTM: Essential validation for OUTPUT_PATH.Adding the absolute path validation for OUTPUT_PATH prevents potential issues with relative paths and ensures consistent behavior across different execution contexts.
177-179: LGTM: Proper URI type handling consolidation.The logic correctly consolidates HTTP and FILE URI types to use the same download mechanism, which makes sense since both are standard URI types that can be handled by CMake's FILE(DOWNLOAD) command.
197-197: LGTM: Improved error message clarity.The updated error message is more descriptive and helps users understand what went wrong.
200-225: LGTM: Enhanced copy operation with proper error handling.The copy operation now properly captures the result variable and provides appropriate status feedback, which is essential for reliable error handling.
236-304: LGTM: Well-structured function for standard URI handling.The function properly handles both HTTP and FILE URI types with appropriate validation and error handling. The parameter changes and logic flow are sound.
375-438: LGTM: Enhanced debugging for git operations.The additional debug messages improve troubleshooting capabilities for git archive operations and extraction failures.
517-521: LGTM: Improved debug messaging for filename determination.The enhanced debug message provides better context when filename determination fails, which aids in troubleshooting.
572-603: LGTM: Proper URI type detection for FILE URIs.The logic correctly identifies file:// URIs and assigns the appropriate FILE type, completing the support for local file URIs.
640-640: LGTM: Minor documentation improvement.The documentation update is accurate and provides clearer information about the hash stripping functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
system_modules/CMLIB_FILE_DOWNLOAD.cmake (2)
66-68: Documentation inconsistency remains unresolved.The documentation still incorrectly states that for FILE URI_TYPE, if OUTPUT_PATH represents a directory, the remote file will be stored under a random generated name. However, the validation logic at lines 479-481 explicitly forbids directories for FILE URIs.
458-491: Add existence check before directory validation for FILE URIs.The current logic checks
IS_DIRECTORYbut doesn't first verify if the path exists. This could lead to confusing behavior sinceIS_DIRECTORYreturns false for non-existent paths.Apply this fix to address the issue:
IF(NOT IS_ABSOLUTE "${file_path}") MESSAGE(FATAL_ERROR "Path specified by URI must be absolute for FILE URI_TYPE. Relative path '${uri}' provided.") ENDIF() + IF(NOT EXISTS "${file_path}") + MESSAGE(FATAL_ERROR "File specified by URI does not exist: '${uri}'") + ENDIF() IF(IS_DIRECTORY "${file_path}") MESSAGE(FATAL_ERROR "Path specified by URI must be file for FILE URI_TYPE. Directory '${uri}' provided.") ENDIF()
🧹 Nitpick comments (10)
README.md (10)
50-52: Fix grammar & spelling; sentence reads awkwardlyThe second sentence is missing “to” after possible and “recommanded” is a typo. The whole paragraph can be tightened.
-It is intended that the user has only one global instance of library. However it is possible use `cmakelib` -as submodule and use `ADD_SUBDIRECTORY`. However it is not recommanded. +The library is designed to be installed once globally. +Using it as a git sub-module via `ADD_SUBDIRECTORY` is possible, but not recommended.
60-71: Shell snippet portability issues ($(PWD)vs$(pwd)) and missing home-expansion
$(PWD)is a bash variable, but in non-bash shells it may be empty;$(pwd)is POSIX-portable.
Also,echo >> .bashrcshould reference the user’s home file explicitly.-echo "export CMLIB_DIR=$(PWD)/cmakelib" >> .bashrc +# Use POSIX-portable $(pwd) and ensure ~/.bashrc is targeted +echo "export CMLIB_DIR=$(pwd)/cmakelib" >> ~/.bashrc
104-106: Spelling: “feaure” → “feature”-As optional feaure CMake-lib provides several components which extends functionality of the core library. +As an optional feature, CMake-lib provides several components that extend the core library’s functionality.
122-124: Article misuse: remove the extra “a”-Cache mechanism is a persistent across CMake binary dir instances. +The cache mechanism is persistent across CMake binary-directory instances.
140-141: Minor clarity & comma placement-If ON we cannot track the same dependency under two different `KEYWORDS` sets. +When ON, the same dependency cannot be tracked under two different `KEYWORDS` sets.
150-153: Typo: “be system” → “by” & wording-`CMLIB_REQUIRED_ENV_TMP_PATH` can be overridden be system ENV var named +`CMLIB_REQUIRED_ENV_TMP_PATH` can be overridden by the environment variable
160-161: Duplicate word “the the”-It's common idiom the the first keyword is name of the project in which +It is a common idiom that the first keyword is the name of the project in which
175-176: Spelling: “resouces” → “resources”- resources from the remote git repository. + resources from the remote git repository.
180-181: Plural & article-For test go to the [test/] and read the attached [test/README.md]. +For tests, see [test/] and its [test/README.md].
202-203: Capitalisation: “cmake” → “CMake” & clarity-In case of cmake-generation problem reset the cache and remove `CMLIB_REQUIRED_ENV_TMP_PATH` directory. +If CMake generation issues occur, clear the cache and remove the `CMLIB_REQUIRED_ENV_TMP_PATH` directory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CMLIB.cmake(3 hunks)README.md(8 hunks)system_modules/CMLIB_ARCHIVE.cmake(1 hunks)system_modules/CMLIB_CACHE.cmake(2 hunks)system_modules/CMLIB_CACHE_CONTROL.cmake(3 hunks)system_modules/CMLIB_COMPONENT.cmake(4 hunks)system_modules/CMLIB_FILE_DOWNLOAD.cmake(20 hunks)system_modules/CMLIB_REQUIRED_ENV.cmake(1 hunks)system_modules/CMLIB_TEMPLATE.cmake(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- system_modules/CMLIB_ARCHIVE.cmake
- system_modules/CMLIB_REQUIRED_ENV.cmake
- system_modules/CMLIB_CACHE_CONTROL.cmake
- system_modules/CMLIB_TEMPLATE.cmake
- system_modules/CMLIB_CACHE.cmake
- CMLIB.cmake
🚧 Files skipped from review as they are similar to previous changes (1)
- system_modules/CMLIB_COMPONENT.cmake
🧰 Additional context used
🧠 Learnings (1)
README.md (1)
Learnt from: koudis
PR: cmakelib/cmakelib#29
File: README.md:231-231
Timestamp: 2024-10-02T17:54:10.405Z
Learning: In the README.md file, '[System modules]' and '[system modules]' are intentionally used as distinct link references.
🪛 LanguageTool
README.md
[grammar] ~51-~51: Ensure spelling is correct
Context: ...e ADD_SUBDIRECTORY. However it is not recommanded. #### Global install Library is store...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~175-~175: Ensure spelling is correct
Context: ...nctionality is required for downloading resouces from remote git repository. OFF value ha...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.17.2)
README.md
220-220: Link and image reference definitions should be needed
Duplicate link or image reference definition: "system modules"
(MD053, link-image-reference-definitions)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test_linux_script (test_debian:bookworm)
- GitHub Check: test_windows_script
- GitHub Check: test_linux (test_debian:bullseye)
- GitHub Check: test_linux (test_ubuntu:2004)
- GitHub Check: test_linux (test_fedora:41)
- GitHub Check: test_windows
- GitHub Check: test_macos_11_script
- GitHub Check: test_macos_11
🔇 Additional comments (10)
README.md (1)
219-220: Duplicate link reference flagged by markdownlint
[System modules]and[system modules]resolve to the same label in a case-insensitive Markdown implementation, so most linters treat them as duplicates (MD053).
If the intentional distinction is required (per previous PR #29), consider adding an explanatory HTML comment so future linters/reviewers know the duplication is deliberate.system_modules/CMLIB_FILE_DOWNLOAD.cmake (9)
85-101: LGTM! Comprehensive documentation for URI_TYPE support.The documentation clearly explains the supported URI types and their prerequisite checks, providing good guidance for users.
137-139: LGTM! Essential validation for OUTPUT_PATH.Adding the absolute path requirement for OUTPUT_PATH improves robustness and prevents potential issues with relative paths.
177-181: LGTM! Unified handling for HTTP and FILE URI types.The refactoring to use a common
_CMLIB_FILE_DOWNLOAD_FROM_STANDARD_URIfunction for both HTTP and FILE types with URI_TYPE parameter is well-designed.
249-270: LGTM! Function signature updated correctly.The function now accepts URI_TYPE parameter which enables proper validation for different URI types.
517-517: LGTM! Enhanced debug messaging.The more descriptive debug message improves troubleshooting when filename determination fails.
571-603: LGTM! FILE URI type detection implemented correctly.The logic correctly identifies
file://URIs and assigns the FILE type, integrating well with the existing URI type determination.
220-225: LGTM! Improved error handling for copy operations.The addition of proper result checking for copy operations with debug messaging on failure enhances reliability.
375-375: LGTM! Clear debug message for git-archive initiation.The debug message helps track the download process flow.
438-438: LGTM! Enhanced error message for archive extraction.The improved debug message provides better context for troubleshooting extraction failures.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
doc/ReleaseProcess.md (4)
4-4: Correct typo and tighten wording
“abvout” → “about”; the sentence reads more naturally if you lead with “When a new version is about to be released, …”.-When new version is abvout to be released, the following steps must be taken +When a new version is about to be released, complete the following steps:
6-10: Use headings instead of bold and remove duplicated word
Markdown-lint is flagging the emphasis-as-heading pattern. Converting these to real headings improves structure, and “version version” is a copy-paste artifact.-**Open PR to release cmakelib** +### Open PR to release CMakeLib ... -- Update version version in `version.txt` +- Update the version in `version.txt`
11-18: Fix typos and improve clarity in component section
Several misspellings (“cmlib”, “boiund”, “cmaikelib”, “doe”) and a duplicated word slip through. While touching the lines, switch to a proper heading for consistency.-**Open PR for each cmlib component** +### Open PR for each CMakeLib component ... -- Update CI/CD for each respective component (cmakelib is boiund to version in each CI/CD pipeline). - Update CI/CD yamls to point to a new version of cmaikelib + • Update CI/CD for each component (each pipeline pins a specific CMakeLib version). + • Point the CI/CD YAMLs to the new CMakeLib version. ... - Increment MINOR part of the component version if the release is doe only because of cmakelib release + Increment the MINOR part of the component version if the release is triggered solely by a CMakeLib update. ... -- Update component version version in `version.txt` +- Update the component version in `version.txt`
19-26: Minor capitalization and spelling fixes in merge section
Align heading style, capitalise “GitHub”, and correct “versiontag” / “create”.-**Merge PRs** +### Merge PRs ... - - Create new release in cmakelib github with a versiontag (`v<version>`). + - Create a new release in the CMakeLib GitHub repository with the tag `v<version>`. ... - - create new release in component github with a versiontag (`v<component_version>`). + - Create a new release in the component’s GitHub repository with the tag `v<component_version>`.README.md (6)
51-52: Fix typo “recommanded” → “recommended”.Small but visible spelling error in the installation instructions.
-as submodule and use `ADD_SUBDIRECTORY`. However it is not recommanded. +as submodule and use `ADD_SUBDIRECTORY`. However it is not recommended.
104-106: Spelling: “feaure” → “feature”.Minor typo in the components section.
-As optional feaure CMake-lib provides several components which extends functionality of the core library. +As an optional feature, CMake-lib provides several components that extend the core library’s functionality.
123-124: Grammar tweak for readability.“Cache mechanism is a persistent …” → “The cache mechanism is persistent …”.
-Cache mechanism is a persistent across CMake binary dir instances. +The cache mechanism is persistent across CMake binary-dir instances.
160-161: Duplicate “the”.Remove the extra article.
-It's common idiom the the first keyword is name of the project in which +It's a common idiom that the first keyword is the name of the project in which
175-176: Spelling: “resouces” → “resources”.Also adjust wording slightly.
- resources from the remote git repository. + resources from the remote Git repository.
180-181: Pluralisation: “test” → “tests”.-For test go to the [test/] and read the attached [test/README.md]. +For tests go to [test/] and read the attached [test/README.md].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(8 hunks)doc/ReleaseProcess.md(1 hunks)version.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- version.txt
🧰 Additional context used
🧠 Learnings (1)
README.md (1)
Learnt from: koudis
PR: cmakelib/cmakelib#29
File: README.md:231-231
Timestamp: 2024-10-02T17:54:10.405Z
Learning: In the README.md file, '[System modules]' and '[system modules]' are intentionally used as distinct link references.
🪛 LanguageTool
README.md
[grammar] ~51-~51: Ensure spelling is correct
Context: ...e ADD_SUBDIRECTORY. However it is not recommanded. #### Global install Library is store...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~175-~175: Ensure spelling is correct
Context: ...nctionality is required for downloading resouces from remote git repository. OFF value ha...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
doc/ReleaseProcess.md
[grammar] ~4-~4: Ensure spelling is correct
Context: ...ib Release Process When new version is abvout to be released, the following steps mus...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~13-~13: Ensure spelling is correct
Context: ... each respective component (cmakelib is boiund to version in each CI/CD pipeline). U...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~23-~23: Ensure spelling is correct
Context: ...e new release in cmakelib github with a versiontag (v<version>). - For each component ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.17.2)
README.md
224-224: Link and image reference definitions should be needed
Duplicate link or image reference definition: "system modules"
(MD053, link-image-reference-definitions)
doc/ReleaseProcess.md
6-6: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
11-11: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
19-19: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test_windows_script
- GitHub Check: test_windows
- GitHub Check: test_linux (test_debian:bullseye)
- GitHub Check: test_linux (test_debian:bookworm)
- GitHub Check: test_macos_script
- GitHub Check: test_macos
🔇 Additional comments (1)
README.md (1)
224-224: Intentionally duplicated link reference – no action required.markdownlint flags the duplicate “[system modules]” reference, but per previous discussion (see retrieved learning) the distinct capitalisation is intentional.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/README.md (1)
2-8: Unify project name & casing (CMLIBeverywhere).
CMAKELIB(l 2) vsCMLIB(l 6-8) introduces a third variant after the earlier “CMlib / CMLib” discussion. Pick one (the project has been converging onCMLIB) and replace all occurrences to avoid brand dilution and search-inconsistencies.-# CMAKELIB Test Suite +# CMLIB Test Suite ... -CMLIB is consistent and functional only when all tests pass. +CMLIB is consistent and functional only when all tests pass. ... -Any test failure indicates CMLIB is not working as expected, +Any test failure indicates CMLIB is not working as expected,
🧹 Nitpick comments (1)
test/README.md (1)
12-20: Polish run instructions & fix OS spelling.Minor wording / formatting improvements:
- “For test go to” → “To run the tests, navigate to”.
- “Linux/Mac Os” → “Linux/macOS”.
- Tabs & spaces are mixed; stick to 4-space indents for GitHub markdown.
-For test go to the `test/` directory and run - -Linux/Mac Os: +To run the tests, navigate to the `test/` directory and execute: + +Linux/macOS:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
test/DEPENDENCY/argument_missing/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/uri_type_mismatches/file_with_git/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/fail/uri_type_mismatches/file_with_http/CMakeLists.txt(1 hunks)test/DEPENDENCY/download_check/file_uri_download/CMakeLists.txt(1 hunks)test/DEPENDENCY/type_check/type_file/CMakeLists.txt(1 hunks)test/DEPENDENCY/type_check/type_module/CMakeLists.txt(1 hunks)test/FILE_DOWNLOAD/compute_hash/CMakeLists.txt(3 hunks)test/README.md(1 hunks)test/test_resources/README.md(1 hunks)test/test_resources/test_resources.cmake(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/test_resources/README.md
🚧 Files skipped from review as they are similar to previous changes (9)
- test/DEPENDENCY/download_check/file_uri_download/CMakeLists.txt
- test/DEPENDENCY/argument_missing/CMakeLists.txt
- test/DEPENDENCY/type_check/type_module/CMakeLists.txt
- test/FILE_DOWNLOAD/compute_hash/CMakeLists.txt
- test/DEPENDENCY/download_check/fail/uri_type_mismatches/file_with_git/CMakeLists.txt
- test/DEPENDENCY/download_check/fail/uri_type_mismatches/file_with_http/CMakeLists.txt
- test/DEPENDENCY/type_check/type_file/CMakeLists.txt
- test/DEPENDENCY/download_check/fail/CMakeLists.txt
- test/test_resources/test_resources.cmake
🧰 Additional context used
🧠 Learnings (1)
test/README.md (2)
Learnt from: koudis
PR: cmakelib/cmakelib#32
File: test/FILE_DOWNLOAD/CMakeLists.txt:7-10
Timestamp: 2025-07-12T19:55:02.148Z
Learning: The _CMLIB_TEST_GIT_ARCHIVE test in test/FILE_DOWNLOAD/CMakeLists.txt requires SSH key access to git@gitlab.com:cmakelib/cmakelib-test.git for the git archive functionality test to work.
Learnt from: koudis
PR: cmakelib/cmakelib#32
File: test/DEPENDENCY/download_check/fail/unreachable_uris/file_uri_unreachable/CMakeLists.txt:15-21
Timestamp: 2025-07-12T19:59:21.224Z
Learning: In test files designed to fail (marked "TEST must FAIL"), using hard-coded unreachable file paths like "/tmp/nonexistent_directory_12345/nonexistent_file.txt" is acceptable because the probability of someone creating the same file is almost zero, according to koudis's test design philosophy.
🪛 LanguageTool
test/README.md
[grammar] ~41-~41: Ensure spelling is correct
Context: .... ``` ## Known Problems - git archive funcionality is not tested by cmakelib-test Github r...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test_windows
- GitHub Check: test_windows_script
- GitHub Check: test_macos
🔇 Additional comments (1)
test/README.md (1)
41-45: Spelling & capitalisation fixes in “Known Problems” section.• “funcionality” → “functionality”
• “git archive funcionality is not tested by cmakelib-test Github repository.” → capitalise GitHub and GitLab/URI consistently.
[ suggest_nitpick ]-## Known Problems - - git archive funcionality is not tested by cmakelib-test Github repository. +## Known Problems +- Git archive functionality is not tested by the cmakelib-test GitHub repository.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
README.md (1)
65-71: Duplicate: incorrect$PWDexpansion & grammar in Bash snippet remain unfixed.
Same issue was raised in a prior review:
$(PWD)tries to execute a non-existent command; use$PWD.- “make CMLIB works” → “make CMLIB work”.
- It’s clearer to write to
~/.bashrc.-To make CMLIB works add following lines to your .bashrc +To make CMLIB work, add the following lines to your ~/.bashrc @@ -echo "export CMLIB_DIR=$(PWD)/cmakelib" >> .bashrc -#echo "export CMLIB_REQUIRED_ENV_TMP_PATH=$(PWD)/cmakelib_cache" >> .bashrc +echo "export CMLIB_DIR=$PWD/cmakelib" >> ~/.bashrc +#echo "export CMLIB_REQUIRED_ENV_TMP_PATH=$PWD/cmakelib_cache" >> ~/.bashrc
🧹 Nitpick comments (3)
test/ARCHIVE/CMakeLists.txt (1)
50-61: Typo in helper nameTEST_EXCTRACT_WITH_SYMLINKThe function is misspelled (
TEST_EXCTRACT_WITH_SYMLINK→TEST_EXTRACT_WITH_SYMLINK).
While unchanged in this PR, fixing it now avoids confusion and aligns withTEST_EXTRACT.-function(TEST_EXCTRACT_WITH_SYMLINK archive_name) +function(TEST_EXTRACT_WITH_SYMLINK archive_name) ... -endfunction() +endfunction() ... - TEST_EXCTRACT_WITH_SYMLINK(symlink.tar) - TEST_EXCTRACT_WITH_SYMLINK(symlink.zip) + TEST_EXTRACT_WITH_SYMLINK(symlink.tar) + TEST_EXTRACT_WITH_SYMLINK(symlink.zip)README.md (2)
51-52: Spelling error: “recommanded” → “recommended”.
Minor typo; fixing it improves professionalism.-as submodule and use `INCLUDE(<submodule_path>/CMLIB.cmake)`. However it is not recommanded. +as submodule and use `INCLUDE(<submodule_path>/CMLIB.cmake)`. However, it is not recommended.
55-59: Clarify wording around global installation.
Current phrasing is awkward and may confuse readers about why the variable is needed.-Library is stored on User computer and the global environment variable `CMLIB_DIR` -must be defined. - -`CMLIB_DIR` variable is named as CMake requires in order to FIND_PACKAGE to find a CMLIB by a `FIND_PACKAGE(CMLIB REQUIRED)` +Store the library locally and define the global environment variable `CMLIB_DIR`. + +`CMLIB_DIR` is what CMake looks for when resolving `FIND_PACKAGE(CMLIB REQUIRED)`, +so it must point to the root of the cloned repository.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
test/ARCHIVE/archive/archive.tar.xzis excluded by!**/*.xztest/ARCHIVE/archive/archive.xzis excluded by!**/*.xz
📒 Files selected for processing (7)
README.md(9 hunks)example/DEPENDENCY/boost_example/CMakeLists.txt(0 hunks)example/DEPENDENCY/boost_example/main.cpp(0 hunks)example/DEPENDENCY/nlohmann_json_example/CMakeLists.txt(1 hunks)example/DEPENDENCY/nlohmann_json_example/main.cpp(1 hunks)system_modules/CMLIB_ARCHIVE.cmake(3 hunks)test/ARCHIVE/CMakeLists.txt(1 hunks)
💤 Files with no reviewable changes (2)
- example/DEPENDENCY/boost_example/CMakeLists.txt
- example/DEPENDENCY/boost_example/main.cpp
✅ Files skipped from review due to trivial changes (2)
- example/DEPENDENCY/nlohmann_json_example/main.cpp
- example/DEPENDENCY/nlohmann_json_example/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- system_modules/CMLIB_ARCHIVE.cmake
🧰 Additional context used
🧠 Learnings (2)
test/ARCHIVE/CMakeLists.txt (1)
Learnt from: koudis
PR: cmakelib/cmakelib#32
File: test/FILE_DOWNLOAD/CMakeLists.txt:7-10
Timestamp: 2025-07-12T19:55:02.179Z
Learning: The _CMLIB_TEST_GIT_ARCHIVE test in test/FILE_DOWNLOAD/CMakeLists.txt requires SSH key access to git@gitlab.com:cmakelib/cmakelib-test.git for the git archive functionality test to work.
README.md (2)
Learnt from: koudis
PR: cmakelib/cmakelib#32
File: test/FILE_DOWNLOAD/CMakeLists.txt:7-10
Timestamp: 2025-07-12T19:55:02.179Z
Learning: The _CMLIB_TEST_GIT_ARCHIVE test in test/FILE_DOWNLOAD/CMakeLists.txt requires SSH key access to git@gitlab.com:cmakelib/cmakelib-test.git for the git archive functionality test to work.
Learnt from: koudis
PR: cmakelib/cmakelib#29
File: README.md:231-231
Timestamp: 2024-10-02T17:54:10.405Z
Learning: In the README.md file, '[System modules]' and '[system modules]' are intentionally used as distinct link references.
🪛 LanguageTool
README.md
[style] ~58-~58: Consider a more concise word here.
Context: ...IRvariable is named as CMake requires in order to FIND_PACKAGE to find a CMLIB by aFIND...
(IN_ORDER_TO_PREMIUM)
🪛 markdownlint-cli2 (0.17.2)
README.md
224-224: Link and image reference definitions should be needed
Duplicate link or image reference definition: "system modules"
(MD053, link-image-reference-definitions)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test_macos_script
- GitHub Check: test_windows_script
- GitHub Check: test_linux_script (test_fedora:41)
- GitHub Check: test_linux_script (test_ubuntu:2404)
- GitHub Check: test_linux_script (test_debian:trixie)
- GitHub Check: test_linux (test_ubuntu:2004)
- GitHub Check: test_linux_script (test_debian:bullseye)
- GitHub Check: test_linux (test_ubuntu:2404)
- GitHub Check: test_linux_script (test_debian:bookworm)
- GitHub Check: test_windows
- GitHub Check: test_linux (test_debian:trixie)
- GitHub Check: test_macos
🔇 Additional comments (4)
test/ARCHIVE/CMakeLists.txt (4)
79-80: Implicit-type.xztest looks goodAdding the raw
archive.xzcase without an explicitARCHIVE_TYPEflag verifies the auto-detection path that was added in the module. 👍
82-83: Coverage for.tar.xzauto-detection addedSame remark here—the new test ensures the extractor can infer
TAR.XZfrom the extension. No issues spotted.
86-87: ExplicitTAR.XZtype test complements auto-detectionGood call to validate the explicit-type code path in addition to extension sniffing.
89-90: ExplicitXZtype test mirrors implicit caseThis completes the matrix; looks fine.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/examples.yml (2)
23-24: Strip trailing whitespace to appease YAML-lintSeveral lines contain an extra space after the comma or at EOL, triggering
YAMLlint trailing-spaceserrors.- "example/ARCHIVE", + "example/ARCHIVE",Repeat for the other flagged lines (23, 39, 43, 75, 79, 97, 111, 115).
Also applies to: 39-43, 75-79, 97-100, 111-115
18-28: Minor duplication – consider YAML anchors or a composite actionThe three jobs differ only by runner & platform-specific build flags.
Extracting shared logic would cut maintenance cost and matrix explosion,
but this is optional.Also applies to: 56-66, 92-102
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/examples.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/examples.yml
40-40: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
76-76: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
112-112: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/examples.yml
[error] 23-23: trailing spaces
(trailing-spaces)
[error] 39-39: trailing spaces
(trailing-spaces)
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 75-75: trailing spaces
(trailing-spaces)
[error] 79-79: trailing spaces
(trailing-spaces)
[error] 97-97: trailing spaces
(trailing-spaces)
[error] 111-111: trailing spaces
(trailing-spaces)
[error] 115-115: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: test_linux_script (test_fedora:41)
- GitHub Check: test_linux_script (test_fedora:42)
- GitHub Check: test_linux_script (test_ubuntu:2204)
- GitHub Check: test_windows_script
- GitHub Check: test_linux_script (test_ubuntu:2404)
- GitHub Check: test_linux (test_ubuntu:2204)
- GitHub Check: test_linux (test_ubuntu:2004)
- GitHub Check: test_linux (test_debian:trixie)
- GitHub Check: test_linux (test_debian:bullseye)
- GitHub Check: test_macos
- GitHub Check: test_windows
- GitHub Check: test_linux (test_debian:bookworm)
- GitHub Check: test_macos_script
🔇 Additional comments (1)
.github/workflows/examples.yml (1)
85-88: Build type mismatch on WindowsYou pass
-DCMLIB_DEBUG=ON(Debug flags) but compile with
--config Release, potentially hiding assertions or altering behaviour.Either switch the build to Debug:
- cmake --build . --config Release + cmake --build . --config Debugor drop the
CMLIB_DEBUGflag for Release.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/examples.yml (2)
17-17: Upgradeactions/checkoutto v4 in all jobs
actionlintflags v3 as running on an obsolete runner. v4 is the current, supported version and avoids future deprecations.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4Apply the same change at lines 17, 84, 120 and 156.
Also applies to: 84-84, 120-120, 156-156
76-82: Guard the “Install SSH key” step to prevent fork failures & secret leaks
Secrets are empty on PRs from forks, causing job failure and a potential accidental key disclosure if someone defines a public secret with the same name. Wrap each step with a conditional:- - name: Install SSH key - uses: shimataro/ssh-key-action@v2 + - name: Install SSH key + if: ${{ secrets.SSHKEY != '' }} + uses: shimataro/ssh-key-action@v2Replicate for Linux (lines 76-82), Windows (112-118) and macOS (148-154) blocks.
Also applies to: 112-118, 148-154
🧹 Nitpick comments (1)
.github/workflows/examples.yml (1)
67-67: Strip trailing whitespace to satisfy YAML-lint
Trailing spaces make the workflow fail the linting gate and add noise to diffs.Also applies to: 83-83, 87-87, 119-119, 123-123, 141-141, 155-155, 159-159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/examples.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/examples.yml
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
19-19: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
84-84: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
120-120: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
156-156: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/examples.yml
[error] 67-67: trailing spaces
(trailing-spaces)
[error] 83-83: trailing spaces
(trailing-spaces)
[error] 87-87: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
[error] 123-123: trailing spaces
(trailing-spaces)
[error] 141-141: trailing spaces
(trailing-spaces)
[error] 155-155: trailing spaces
(trailing-spaces)
[error] 159-159: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test_windows
- GitHub Check: test_macos_script
- GitHub Check: test_linux (test_fedora:41)
- GitHub Check: wait-for-tests
- GitHub Check: test_windows_script
- GitHub Check: test_linux (test_debian:trixie)
- GitHub Check: test_macos
🔇 Additional comments (1)
.github/workflows/examples.yml (1)
31-38: Verifyworkflow_id: "tests.yml"is correct for the polling logic
actions.listWorkflowRunsForReporequires the exact filename or numeric ID. If your Tests workflow file is named differently (e.g.,.github/workflows/tests.ymlvstests.yml), the poll will always return zero runs and the wait loop will time-out.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/examples.yml (2)
29-36: Guard secret-dependent “Install SSH key” stepsThese three steps still run unconditionally. On PRs from forks the
${{ secrets.* }}values are empty → workflow fails and could leak a future fork-side secret with the same name.
Wrap each step with a conditional guard exactly as suggested in the previous review to keep forks green and secrets safe.- - name: Install SSH key - uses: shimataro/ssh-key-action@v2 + - name: Install SSH key + if: ${{ secrets.SSHKEY != '' }} + uses: shimataro/ssh-key-action@v2Also applies to: 64-70, 99-105
37-40: Updateactions/checkoutto v4
actionlintcorrectly flags v3 as relying on the deprecated runner image; v4 is required for future compatibility and security updates.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4Apply to all three occurrences.
Also applies to: 72-75, 107-110
🧹 Nitpick comments (1)
.github/workflows/examples.yml (1)
18-24: Remove trailing whitespace for clean YAMLYAMLlint reports stray spaces on the highlighted lines. They don’t break execution but keeping the workflow free of formatting lint helps CI noise.
Also applies to: 20-20, 36-36, 40-40, 71-71, 75-75, 92-92, 106-106, 110-110
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/examples.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/examples.yml
37-37: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
72-72: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
107-107: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/examples.yml
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 36-36: trailing spaces
(trailing-spaces)
[error] 40-40: trailing spaces
(trailing-spaces)
[error] 71-71: trailing spaces
(trailing-spaces)
[error] 75-75: trailing spaces
(trailing-spaces)
[error] 92-92: trailing spaces
(trailing-spaces)
[error] 106-106: trailing spaces
(trailing-spaces)
[error] 110-110: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test_linux_script (test_fedora:42)
- GitHub Check: test_linux_script (test_fedora:41)
- GitHub Check: test_linux_script (test_ubuntu:2404)
- GitHub Check: test_linux_script (test_ubuntu:2004)
- GitHub Check: test_linux_script (test_debian:bullseye)
- GitHub Check: test_linux_script (test_debian:trixie)
- GitHub Check: test_linux (test_ubuntu:2204)
- GitHub Check: test_linux_script (test_debian:bookworm)
- GitHub Check: test_windows_script
- GitHub Check: test_linux (test_ubuntu:2004)
- GitHub Check: test_macos_script
- GitHub Check: test_linux (test_fedora:42)
- GitHub Check: test_windows
- GitHub Check: test_macos
koudis
force-pushed
the
jan_kubalek/component_versioning
branch
from
4f7f399 to
a7dfdaa
Compare
koudis
force-pushed
the
jan_kubalek/component_versioning
branch
from
13d4888 to
3261029
Compare
1 participant
Summary by CodeRabbit
New Features
file://) in dependency downloads with strict validation.Bug Fixes
Documentation
Tests
Refactor