Add tests - First round #11
Conversation
This reverts commit 290f60d.
|
Warning Rate limit exceeded@koudis has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 51 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 (2)
## Walkthrough
This update introduces a comprehensive suite of tests for the CMDEF component, covering executable and library creation, environment detection, installation, and packaging. Numerous typographical corrections are made across CMake scripts and documentation. The test framework includes helper macros, cache variable management, and an INSTALL command override for validation. Minor logic corrections and clarifications are applied to core modules.
## Changes
| File(s) / Path(s) | Change Summary |
|---------------------------------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `.github/workflows/tests.yml` | Added GitHub Actions workflow to automate tests across Linux, macOS, and Windows using matrix containers and platform runners. |
| `README.md`, `doc/README.md`, `doc/CacheVariables.md`, `doc/PackageChain.md` | Documentation and README updates: clarified terminology, fixed typos, updated descriptions, and added/modified documentation files. |
| `example/*` | Fixed typos in visibility keywords and comments; added a function to print all CMake variables in the executable example. |
| `system_modules/CMDEF_ADD_EXECUTABLE.cmake` | Corrected macOS bundle flag from `MACOS_BUNDLE` to `MACOSX_BUNDLE`. |
| `system_modules/CMDEFConfig.cmake`, `FindCMDEF.cmake` | Updated included CMake file reference and minimum required CMake version; added new module inclusion. |
| `system_modules/CMDEF_ENV.cmake` | Refactored OS/arch/distro detection to use CMake built-ins instead of external commands; fixed typos and improved validation; updated function signatures and logic for architecture and distro ID/version retrieval; added Windows flags initialization call. |
| `system_modules/CMDEF_BUILD_TYPE.cmake`, `system_modules/CMDEF_RESOURCE.cmake` | Fixed minor typos in comments. |
| `system_modules/CMDEF_COMPILE_DEFINITION.cmake`, `system_modules/CMDEF_COMPILE_OPTION.cmake`, `system_modules/CMDEF_LINK_OPTION.cmake` | Fixed spelling of "language" and "VISIBILITY" in comments, argument parsing, and code; updated function signatures accordingly. |
| `system_modules/CMDEF_ADD_LIBRARY.cmake` | Clarified comments for `SOURCE_BASE_DIRECTORY` parameter. |
| `system_modules/CMDEF_PACKAGE.cmake` | Inserted explicit version validation with `CMUTIL_VERSION_CHECK` in the package creation process; renamed argument from `__ALREADY_LINKED_LIBS` to `ALREADY_LINKED_LIBS` in dependency checking; clarified warning message wording. |
| `test/`, `test/CMDEF_ADD_EXECUTABLE/`, `test/CMDEF_ADD_LIBRARY/`, `test/CMDEF_ENV/`, `test/CMDEF_INSTALL/`, `test/CMDEF_PACKAGE/` | Added extensive test suites for executables, libraries, environment, install, and package modules. Includes CMakeLists, test macros, shared source files, helper scripts, and README documentation. |
| `test/TEST.cmake`, `test/install_override.cmake`, `test/cache_var.cmake`, `test/CMDEF_INSTALL/install_test_macros.cmake`, `test/CMDEF_PACKAGE/package_test_macros.cmake` | Introduced test helper scripts: assertion macros, cache variable management, INSTALL command override for test validation, and macros for install/package property checks. |
| `test/CMDEF_ADD_EXECUTABLE/shared_sources/`, `test/CMDEF_ADD_LIBRARY/shared_sources/`, `test/CMDEF_INSTALL/shared_sources/`, `test/CMDEF_PACKAGE/shared_sources/` | Added shared C++ source and header files for use in tests, as well as CMake configuration scripts for consistent resource referencing. |
| `test/CMDEF_ADD_EXECUTABLE/test_cases/`, `test/CMDEF_ADD_LIBRARY/test_cases/`, `test/CMDEF_INSTALL/test_cases/`, `test/CMDEF_PACKAGE/test_cases/` | Added individual test case directories with CMakeLists.txt files testing various scenarios, parameter validations, error conditions, platform-specific behaviors, and property assertions for each module. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant CMake
participant CMDEF_ENV
participant CMDEF_ADD_EXECUTABLE
participant CMDEF_ADD_LIBRARY
participant CMDEF_INSTALL
participant CMDEF_PACKAGE
participant TestMacros
User->>CMake: Configure project (cmake .)
CMake->>CMDEF_ENV: Initialize environment (detect OS, arch, distro)
CMake->>CMDEF_ADD_EXECUTABLE: Add executable target(s)
CMake->>CMDEF_ADD_LIBRARY: Add library target(s)
CMake->>CMDEF_INSTALL: Install targets (with override for tests)
CMake->>CMDEF_PACKAGE: Package targets (with version check)
CMake->>TestMacros: Run test assertions and property checks
TestMacros-->>CMake: Report pass/fail for each testPossibly related PRs
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
🧹 Nitpick comments (41)
test/CMDEF_ADD_EXECUTABLE/shared_sources/src/utils.cpp (1)
8-10: Consider enhancing the test logic.While the current implementation is acceptable for basic testing, consider adding actual test logic instead of always returning
trueto make the function more meaningful for test validation.bool run_tests() { - return true; + // Add actual test logic here + // For now, return true as placeholder + return true; }test/CMDEF_ADD_LIBRARY/shared_sources/include/utils.h (1)
1-6: Add parameter names and documentation for better clarity.While the header structure is sound, consider adding parameter names to function declarations and brief documentation comments for better maintainability.
#pragma once #include <string> +/** + * @brief Gets the library name for testing purposes + * @return The library name as a string + */ std::string get_library_name(); + +/** + * @brief Validates input value + * @param value The integer value to validate + * @return True if input is valid, false otherwise + */ -bool validate_input(int value); +bool validate_input(int value);test/CMDEF_ADD_EXECUTABLE/shared_sources/include/platform_utils.h (1)
1-6: Add documentation for consistency and clarity.Similar to the previous header, this would benefit from documentation comments to describe the purpose and return values of the platform utility functions.
#pragma once #include <string> +/** + * @brief Gets the current platform name + * @return Platform name as a string + */ std::string get_platform_name(); + +/** + * @brief Checks if this is a debug build + * @return True if debug build, false otherwise + */ bool is_debug_build();test/CMDEF_ADD_LIBRARY/shared_sources/src/library.cpp (1)
4-6: Consider using '\n' instead of std::endl for better performance.While this is a test file where performance may not be critical, using
'\n'instead ofstd::endlavoids unnecessary buffer flushing.void library_function() { - std::cout << "Library function called" << std::endl; + std::cout << "Library function called\n"; }test/CMDEF_ADD_LIBRARY/README.md (1)
30-33: Clarify build invocation.Suggest specifying an out‐of‐source build for clarity, e.g.:
cmake -S . -B build cmake --build buildThis avoids in‐source build artifacts.
test/CMDEF_ADD_EXECUTABLE/shared_sources/src/platform_utils.cpp (1)
16-22: Consider more portable debug build detection.The current implementation only checks for
_DEBUG, which is MSVC-specific. Other compilers like GCC and Clang typically useNDEBUG(defined in release builds) orDEBUG(defined in debug builds).Consider this more portable approach:
bool is_debug_build() { -#ifdef _DEBUG +#if defined(_DEBUG) || defined(DEBUG) || !defined(NDEBUG) return true; #else return false; #endif }This covers MSVC (
_DEBUG), common debug builds (DEBUG), and the standard approach whereNDEBUGis undefined in debug builds.test/CMDEF_ADD_EXECUTABLE/README.md (1)
28-33: Clarify the usage instructions.The usage section could be more specific about the execution context and expected outcomes.
Consider expanding the usage section:
## Usage -Run all CMDEF_ADD_EXECUTABLE tests: +Run all CMDEF_ADD_EXECUTABLE tests from the test directory: ```bash +cd test/CMDEF_ADD_EXECUTABLE cmake .
+This will execute all test cases and report any failures or errors.
</blockquote></details> <details> <summary>test/CMDEF_ENV/macos_specific/CMakeLists.txt (2)</summary><blockquote> `14-21`: **Use a consistent skip mechanism** Instead of `MESSAGE(STATUS)` for non-macOS platforms, consider using a dedicated skip macro (e.g., `TEST_SKIP()`) to explicitly mark skipped tests in the framework. --- `21-21`: **Cleanup trailing whitespace** Remove the extra space after `ENDIF()` for consistency. </blockquote></details> <details> <summary>test/README.md (3)</summary><blockquote> `8-8`: **Clarify introductory phrase** Revise “Currently tested functionality:” to “Currently tested functionality includes:” for smoother readability. --- `19-21`: **Fix typos and hyphenation** Correct to: ```markdown Tests are designed to run on Linux-based systems. Other supported platforms have platform-specific directories.🧰 Tools
🪛 LanguageTool
[uncategorized] ~20-~20: This expression is usually spelled with a hyphen.
Context: ...lity. Tests are designed as it rund on Linux based systems. Other supported platforms have...(BASED_HYPHEN)
[uncategorized] ~20-~20: When ‘platform-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...systems. Other supported platforms have platform specific directories. ## Test Framework Tests...(SPECIFIC_HYPHEN)
29-29: Use a heading instead of bold text
Convert “Run All Tests” to a Markdown heading (e.g.,### Run All Tests) for consistency.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
29-29: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
test/CMDEF_ENV/CMakeLists.txt (3)
6-8: Specify project languages explicitly
It’s best practice to declare the languages your tests require. For example:cmake_minimum_required(VERSION 3.18) project(CMDEF_ENV_TEST LANGUAGES C)This makes it clear which languages (e.g., C or CXX) are enabled.
9-11: Improve module lookup for shared test scripts
Including../TEST.cmakevia a relative path works, but consider setting or appending toCMAKE_MODULE_PATHso thatfind_package(CTest)-style calls can locate shared helpers more robustly if the directory structure changes.
18-26: Refine OS-specific branch logic
Relying onUNIXto gate Linux tests can be ambiguous (it also matches macOS). You might prefer:if(WIN32) … elseif(APPLE) … elseif(UNIX AND NOT APPLE) message(STATUS "No Linux-specific tests available") else() message(FATAL_ERROR "Unknown OS, cannot run OS-specific tests") endif()This makes the Linux‐only branch explicit.
test/CMDEF_ADD_EXECUTABLE/test_cases/invalid_target_name_warning/CMakeLists.txt (2)
6-8: Declare C++ language for the test project
Since you’re building C++ sources (main.cpp, etc.), addLANGUAGES CXX(orenable_language(CXX)) to theproject()call to ensure the compiler is initialized:project(CMDEF_ADD_EXECUTABLE_INVALID_NAME_WARNING_TEST LANGUAGES CXX)
11-13: Centralize configuration file lookup
Using a deep relative path (../../../../CMDEFConfig.cmake) is brittle. Consider adding the top‐level test directory toCMAKE_MODULE_PATHonce and then simply:include(CMDEFConfig) include(shared_sources)This reduces path maintenance when folders shift.
test/CMDEF_ADD_EXECUTABLE/test_cases/win32_flag/CMakeLists.txt (3)
6-8: Declare C++ language for the test project
As this test compiles C++ executables, addLANGUAGES CXXto theproject()call:project(CMDEF_ADD_EXECUTABLE_WIN32_TEST LANGUAGES CXX)
23-30: Remove placeholder comment
The line# <function>()above theFUNCTION(TEST_WIN32_FLAG_BEHAVIOR)block is a leftover and doesn’t add value. You can safely delete or replace it with a descriptive header.
42-47: Add validation for branch-specific messages
You emit status messages for Windows vs. non-Windows, but the test doesn’t assert them. Consider adding aTEST_CHECK_LOG("WIN32 flag test passed")or similar to ensure the correct branch was taken.test/CMDEF_ADD_EXECUTABLE/test_cases/minimal_executable/CMakeLists.txt (2)
11-13: Consider simplifying include paths via CMAKE_MODULE_PATH.The three deep relative includes increase fragility when files move. You could add your
testdirectory (or the shared module path) toCMAKE_MODULE_PATHonce and then use simpleinclude(TEST)-style calls.
21-22: Update placeholder comment.The
# <function>()line is clearly a template marker—please replace it with# TEST_MINIMAL_EXECUTABLE_CREATION()to keep documentation in sync.test/CMDEF_ADD_LIBRARY/CMakeLists.txt (1)
7-7: Avoid hyphens in project names.Using
CMDEF-ADD-LIBRARY-TESTmay create unexpected variable or target names. Consider switching to underscores (CMDEF_ADD_LIBRARY_TEST) for consistency and safety.test/CMDEF_ADD_EXECUTABLE/test_cases/custom_output_name/CMakeLists.txt (2)
11-14: Consider centralizing include paths.As in other executable tests, these deep relative includes are fragile. Using
CMAKE_MODULE_PATHor an environment variable can reduce repetition and prevent breakage when restructuring.
21-22: Replace template placeholder.Please change
# <function>()to# TEST_CUSTOM_OUTPUT_NAME()so the comment accurately reflects the test function.test/CMDEF_ENV/windows_specific/CMakeLists.txt (1)
14-19: Inconsistent indentation.The body of the
IF(CMDEF_OS_WINDOWS)block mixes a tab and spaces. Please standardize on the project’s indentation style (e.g., two spaces).test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/CMakeLists.txt (1)
38-40: Tighten SOVERSION error matching
The test only checks for"SOVERSION". It may be too generic—consider matching the full expected message (e.g."SOVERSION must not be greater than VERSION") to avoid false positives.test/CMDEF_ADD_EXECUTABLE/test_cases/check_function/CMakeLists.txt (1)
40-43: Unset previous result before reuse
To prevent stale values, consider addingUNSET(regular_result)before callingCMDEF_ADD_EXECUTABLE_CHECKonregularcmakeexe.test/CMDEF_ADD_EXECUTABLE/CMakeLists.txt (1)
27-31: Windows-only test guard
Wrapping Windows-specific tests inIF(WIN32)properly restricts them to that platform. Consider usingCMDEF_OS_WINDOWSfor consistency across tests.test/TEST.cmake (7)
10-11: Fix typo in documentation comments.There's a spelling error in the parameter documentation.
-# var - varialbe name of value +# var - variable name of value
19-20: Fix typo in documentation comments.Another instance of the same spelling error.
-# var - varialbe name of value +# var - variable name of value
28-29: Fix typo in documentation comments.Another instance of the same spelling error.
-# var - varialbe name of value +# var - variable name of value
37-38: Fix typo in documentation comments.Another instance of the same spelling error.
-# var_a - varialbe name of first value +# var_a - variable name of first value
46-47: Fix typo in documentation comments.Another instance of the same spelling error.
-# var_a - varialbe name of first value +# var_a - variable name of first value
56-57: Fix typo in documentation comments.Another instance of the same spelling error.
-# var_a - varialbe name of first value +# var_a - variable name of first value
83-124: Consider improving error handling in TEST_RUN function.The function has good structure but could benefit from enhanced error reporting and path validation.
Consider adding more detailed error context:
IF(result_variable GREATER 0) - MESSAGE(FATAL_ERROR "Test '${test}' failed with '${result_variable}'\nError: ${error_variable}\nOutput: ${output_variable}") + MESSAGE(FATAL_ERROR "Test '${test}' failed with exit code '${result_variable}'\nWorking directory: ${working_dir}\nError output:\n${error_variable}\nStandard output:\n${output_variable}") ENDIF()test/CMDEF_ADD_LIBRARY/test_cases/posix_specific/CMakeLists.txt (2)
43-46: Improve SOVERSION validation with more specific error context.The test logic is correct but the error message could be more informative.
GET_TARGET_PROPERTY(soversion testposixsoversion-shared SOVERSION) IF(NOT soversion STREQUAL "3.2.0") - MESSAGE(FATAL_ERROR "SOVERSION should be set to '3.2.0' on POSIX systems, got: ${soversion}") + MESSAGE(FATAL_ERROR "SOVERSION property mismatch for target 'testposixsoversion-shared'. Expected: '3.2.0', Actual: '${soversion}'") ENDIF()
119-124: Consider strengthening static library PIC validation.The current logic only logs the static library PIC state but doesn't validate expected behavior.
Consider adding platform-specific expectations:
GET_TARGET_PROPERTY(static_pic testposixstaticpic-static POSITION_INDEPENDENT_CODE) - IF(static_pic) - MESSAGE(STATUS "Static library has POSITION_INDEPENDENT_CODE set (may be platform default): ${static_pic}") - ELSE() - MESSAGE(STATUS "Static library does not have POSITION_INDEPENDENT_CODE set (expected)") - ENDIF() + # On most POSIX systems, static libraries don't need PIC unless explicitly set + IF(static_pic) + MESSAGE(STATUS "Static library has POSITION_INDEPENDENT_CODE set: ${static_pic}") + # This might be expected behavior on some platforms + ELSE() + MESSAGE(STATUS "Static library does not have POSITION_INDEPENDENT_CODE set (typical for static libs)") + ENDIF()test/CMDEF_ADD_LIBRARY/test_cases/generator_expressions/CMakeLists.txt (2)
69-72: Tighten regex for generator expression matching.
The pattern".*\\$<\\$<CONFIG:DEBUG>:${CMDEF_LIBRARY_NAME_DEBUG_SUFFIX}>"double-escapes nested expressions and may fail on some CMake versions. Consider usingstring(FIND ...)or simplify to a single\$<CONFIG:Debug>:match.
96-139: Extract repetitive BUILD_INTERFACE wrapping checks.
TheTEST_BUILD_INTERFACE_WRAPPINGfunction repeats nearly identical logic for SHARED, STATIC, and INTERFACE libraries. Encapsulate the property-fetch & regex check into a helper macro to DRY up the file and make future adjustments easier.test/CMDEF_ADD_LIBRARY/test_cases/windows_specific/CMakeLists.txt (1)
82-85: Add prefix check for Windows library targets.
On Windows/MSVC, libraries should have an empty prefix. You currently verifySUFFIXonly; consider also assertingPREFIXis""for both shared and static variants to lock down naming conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
CMDEFConfig.cmake(1 hunks)FindCMDEF.cmake(1 hunks)system_modules/CMDEF_ENV.cmake(3 hunks)test/CMDEF_ADD_EXECUTABLE/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/README.md(1 hunks)test/CMDEF_ADD_EXECUTABLE/error_test_sources/invalid_syntax.cpp(1 hunks)test/CMDEF_ADD_EXECUTABLE/error_test_sources/missing_include.cpp(1 hunks)test/CMDEF_ADD_EXECUTABLE/shared_sources/include/platform_utils.h(1 hunks)test/CMDEF_ADD_EXECUTABLE/shared_sources/include/utils.h(1 hunks)test/CMDEF_ADD_EXECUTABLE/shared_sources/shared_sources.cmake(1 hunks)test/CMDEF_ADD_EXECUTABLE/shared_sources/src/main.cpp(1 hunks)test/CMDEF_ADD_EXECUTABLE/shared_sources/src/platform_utils.cpp(1 hunks)test/CMDEF_ADD_EXECUTABLE/shared_sources/src/utils.cpp(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/all_parameters/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/check_function/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/custom_output_name/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/debug_suffix_build_type/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/executable_with_includes/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/generator_expression_suffix/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/invalid_target_name_warning/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/invalid_version_error/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/macos_bundle/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/minimal_executable/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/missing_sources_error/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/missing_version_error/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/multiple_sources/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/win32_flag/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/windows_msvc_runtime/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/windows_resource_generation/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/README.md(1 hunks)test/CMDEF_ADD_LIBRARY/multiple_include_sources/include/library.h(1 hunks)test/CMDEF_ADD_LIBRARY/multiple_include_sources/include1/header1.h(1 hunks)test/CMDEF_ADD_LIBRARY/multiple_include_sources/include2/header2.h(1 hunks)test/CMDEF_ADD_LIBRARY/multiple_include_sources/multiple_include_sources.cmake(1 hunks)test/CMDEF_ADD_LIBRARY/multiple_include_sources/src/library.cpp(1 hunks)test/CMDEF_ADD_LIBRARY/multiple_include_sources/src/utils.cpp(1 hunks)test/CMDEF_ADD_LIBRARY/shared_sources/include/library.h(1 hunks)test/CMDEF_ADD_LIBRARY/shared_sources/include/utils.h(1 hunks)test/CMDEF_ADD_LIBRARY/shared_sources/shared_sources.cmake(1 hunks)test/CMDEF_ADD_LIBRARY/shared_sources/src/library.cpp(1 hunks)test/CMDEF_ADD_LIBRARY/shared_sources/src/utils.cpp(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/check_function/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/generator_expressions/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/interface_library/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/multiple_include_directories/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/invalid_type/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/missing_library_group/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/missing_sources_non_interface/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/missing_version/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/source_base_directory_non_interface/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/soversion_greater_than_version/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/posix_specific/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/soversion/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/target_properties_validation/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/version_validation/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/windows_specific/CMakeLists.txt(1 hunks)test/CMDEF_ENV/CMakeLists.txt(1 hunks)test/CMDEF_ENV/README.md(1 hunks)test/CMDEF_ENV/arch_detection/CMakeLists.txt(1 hunks)test/CMDEF_ENV/basic_init/CMakeLists.txt(1 hunks)test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt(1 hunks)test/CMDEF_ENV/macos_specific/CMakeLists.txt(1 hunks)test/CMDEF_ENV/os_detection/CMakeLists.txt(1 hunks)test/CMDEF_ENV/windows_specific/CMakeLists.txt(1 hunks)test/CMakeLists.txt(1 hunks)test/README.md(1 hunks)test/TEST.cmake(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/CMDEF_ADD_EXECUTABLE/shared_sources/src/main.cpp (4)
test/CMDEF_ADD_EXECUTABLE/shared_sources/include/utils.h (2)
get_version_string(5-5)run_tests(6-6)test/CMDEF_ADD_EXECUTABLE/shared_sources/src/utils.cpp (4)
get_version_string(4-6)get_version_string(4-4)run_tests(8-10)run_tests(8-8)test/CMDEF_ADD_EXECUTABLE/shared_sources/include/platform_utils.h (1)
get_platform_name(5-5)test/CMDEF_ADD_EXECUTABLE/shared_sources/src/platform_utils.cpp (2)
get_platform_name(4-14)get_platform_name(4-4)
🪛 LanguageTool
test/README.md
[uncategorized] ~7-~7: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...) component library. ## Test Modules Currently tested functionality: - **`CMDEF_ADD_E...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~20-~20: This expression is usually spelled with a hyphen.
Context: ...lity. Tests are designed as it rund on Linux based systems. Other supported platforms have...
(BASED_HYPHEN)
[uncategorized] ~20-~20: When ‘platform-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...systems. Other supported platforms have platform specific directories. ## Test Framework Tests...
(SPECIFIC_HYPHEN)
🪛 markdownlint-cli2 (0.17.2)
test/README.md
29-29: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
🔇 Additional comments (122)
test/CMDEF_ADD_EXECUTABLE/shared_sources/include/utils.h (1)
1-6: LGTM! Clean and well-structured header.The header follows good C++ practices with proper include guard, minimal necessary includes, and clear function declarations.
test/CMDEF_ADD_LIBRARY/shared_sources/include/library.h (1)
1-12: LGTM! Proper C/C++ interoperability implementation.The header correctly implements C linkage compatibility with proper
extern "C"blocks, making the library usable from both C and C++ code. The include guard and function declarations are well-structured.FindCMDEF.cmake (1)
12-12: LGTM! Reasonable CMake version requirement update.The version bump from 3.21 to 3.22 aligns with the broader system updates and ensures compatibility with newer CMake features used in the test infrastructure.
test/CMDEF_ENV/README.md (1)
1-8: LGTM! Clear and concise test documentation.The README provides appropriate usage instructions for the CMDEF_ENV test cases with clear and simple commands.
CMDEFConfig.cmake (1)
2-2:✅ Verification successful
LGTM! Corrected include reference.
The update from
FinDBASEDEF.cmaketoFindCMDEF.cmakecorrectly aligns the configuration with the actual module file. This ensures proper module inclusion.Verify that the old
FinDBASEDEF.cmakefile is properly handled in the codebase:
🏁 Script executed:
#!/bin/bash # Description: Verify if FinDBASEDEF.cmake file exists and check for any remaining references # Check if the old file still exists fd "FinDBASEDEF.cmake" --type f # Search for any remaining references to the old file rg -i "findbasedef" --type cmakeLength of output: 64
Confirmed removal of old FinDBASEDEF.cmake references
No instances of
FinDBASEDEF.cmakeremain in the repository—fdfinds no such file andrgreports zero matches—so the include update toFindCMDEF.cmakeis safe and complete.test/CMDEF_ADD_LIBRARY/multiple_include_sources/src/utils.cpp (1)
1-5: Trivial utility implementation is correct.The
utils_function()definition cleanly returns 42 and correctly includeslibrary.hfor its declarations. No functional or style issues detected.test/CMDEF_ADD_LIBRARY/multiple_include_sources/include1/header1.h (1)
1-4: Header guard and declaration look good.Using
#pragma onceand the prototypevoid function1();is appropriate for this test harness. No concerns.test/CMDEF_ADD_LIBRARY/multiple_include_sources/include2/header2.h (1)
1-4: Header guard and declaration look good.Using
#pragma onceand the prototypevoid function2();is appropriate for this test harness. No concerns.test/CMakeLists.txt (1)
7-14: Top-level CMake test configuration is valid.The
cmake_minimum_required,project(),find_package(), andadd_subdirectory()calls correctly set up the CMDEF test suites. No changes requested.test/CMDEF_ADD_LIBRARY/multiple_include_sources/include/library.h (1)
1-3: LGTM! Clean header implementation.The header follows best practices with
#pragma oncefor include guards and a clear function declaration.test/CMDEF_ADD_LIBRARY/multiple_include_sources/src/library.cpp (1)
1-6: LGTM! Proper implementation of the declared function.The source file correctly includes the corresponding header and implements the function with appropriate test output. The implementation is clean and follows good practices.
test/CMDEF_ADD_EXECUTABLE/shared_sources/src/utils.cpp (1)
4-6: LGTM! Appropriate test utility function.The version string function is clean and serves its purpose for testing.
test/CMDEF_ADD_LIBRARY/shared_sources/src/library.cpp (1)
8-10: LGTM!The addition function implementation is correct and appropriate for testing purposes. No issues detected.
test/CMDEF_ADD_EXECUTABLE/error_test_sources/missing_include.cpp (1)
1-6: Clarify the file's purpose based on its name.The filename suggests this tests "missing include" scenarios, but the file successfully includes
<iostream>and uses it. Consider either:
- Removing the
#include <iostream>if testing missing includes- Renaming the file to better reflect its actual purpose
Could you clarify whether this file should actually be missing an include to test error handling, or if the name should be updated to reflect its current functionality?
test/CMDEF_ADD_LIBRARY/multiple_include_sources/multiple_include_sources.cmake (1)
3-10: Configuration variables correctly defined.The source and include directory variables are well-organized and properly scoped for the multiple‐include test case. No issues found.
test/CMDEF_ADD_LIBRARY/shared_sources/src/utils.cpp (1)
1-10: LGTM! Clean utility implementation.The utility functions are well-implemented with correct logic and appropriate header includes. The
validate_inputfunction correctly validates non-negative integers, andget_library_nameprovides a clear constant return value for testing purposes.test/CMDEF_ADD_EXECUTABLE/shared_sources/src/platform_utils.cpp (1)
4-14: LGTM! Comprehensive platform detection.The platform detection logic correctly uses standard preprocessor macros and provides good coverage for major platforms with a fallback for unknown systems.
test/CMDEF_ADD_EXECUTABLE/README.md (1)
1-27: Excellent documentation structure.The README provides clear and comprehensive documentation of the test suite organization, directory structure, and test case coverage. This will be very helpful for developers working with the test framework.
test/CMDEF_ADD_EXECUTABLE/test_cases/invalid_version_error/CMakeLists.txt (1)
1-18:❓ Verification inconclusive
Verify that the invalid version format triggers the expected error.
The test setup is well-structured and follows the established patterns for the test suite. However, ensure that the version string
"invalid.version.format"actually triggers the validation error inCMDEF_ADD_EXECUTABLE.Run the following script to verify the test behavior:
🏁 Script executed:
#!/bin/bash # Description: Verify that the invalid version test actually produces the expected error. # Expected: The test should fail with a version format validation error. # Find and run this specific test case cd test/CMDEF_ADD_EXECUTABLE/test_cases/invalid_version_error cmake . 2>&1 | grep -i "version\|error\|invalid"Length of output: 109
Manual verification needed: invalid version format did not trigger an error
The automated check produced no “version” or “error” output when running CMake against this test, so it’s unclear whether
CMDEF_ADD_EXECUTABLEis validating theVERSIONfield as intended. Please manually confirm that the following case fails with a clear version‐format validation error:• File:
test/CMDEF_ADD_EXECUTABLE/test_cases/invalid_version_error/CMakeLists.txt
– CMDEF_ADD_EXECUTABLE(… VERSION "invalid.version.format")If it does not fail, the test or the validation logic should be updated to enforce and report invalid version formats.
test/CMDEF_ADD_EXECUTABLE/shared_sources/src/main.cpp (1)
1-17: LGTM! Well-structured test executable.The implementation is clean and follows good practices:
- Proper includes and standard library usage
- Clear output messages for debugging
- Correct exit code handling (0 for success, 1 for failure)
- Integration with utility functions matches the provided headers
test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/missing_version/CMakeLists.txt (1)
1-20: LGTM! Well-designed negative test case.The test correctly validates parameter enforcement by:
- Following consistent naming conventions for the test project
- Including all necessary dependencies and configuration files
- Deliberately omitting the VERSION parameter while providing other required parameters
- Clear documentation of the test purpose
This effectively tests the macro's parameter validation logic.
test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/missing_sources_non_interface/CMakeLists.txt (1)
1-19: LGTM! Correct validation test for required SOURCES parameter.The test design is sound:
- Appropriately targets SHARED library type which requires source files
- Maintains consistency with other parameter validation tests
- Clear documentation of the expected failure condition
- Proper CMake configuration and dependency management
This correctly validates that non-INTERFACE libraries must specify sources.
test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/soversion_greater_than_version/CMakeLists.txt (1)
1-27: LGTM! Excellent platform-aware validation test.The test design demonstrates good practices:
- Appropriate POSIX platform check since SOVERSION is primarily used on Unix-like systems
- Clear version relationship test (SOVERSION 2.0.0 > VERSION 1.0.0)
- Includes shared sources for proper library source file setup
- Consistent structure with other parameter validation tests
- Graceful handling of non-POSIX platforms with informative message
This effectively validates the version relationship constraints.
test/CMDEF_ENV/macos_specific/CMakeLists.txt (1)
11-12: Includes verified
The paths toTEST.cmakeandCMDEF_ENV.cmakeare correct relative to this file.test/CMDEF_ADD_EXECUTABLE/test_cases/debug_suffix_build_type/CMakeLists.txt (3)
1-5: Header comments are clear; no issues detected.
6-14: Setup and includes look correct
Project declaration and inclusion ofTEST.cmake,CMDEFConfig.cmake, and shared sources are accurate.
51-51: Function invocation approved
The call toTEST_DEBUG_SUFFIX_APPLICATION()correctly executes the defined test function.test/CMDEF_ADD_EXECUTABLE/test_cases/missing_version_error/CMakeLists.txt (3)
1-5: File header and comment structure are clear.
6-10: Project setup verified
TheCMAKE_MINIMUM_REQUIRED,PROJECT, andFIND_PACKAGEdirectives are correct.
11-13: Include paths are correct
Paths toCMDEFConfig.cmakeandshared_sources.cmakeresolve appropriately relative to this directory.test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/missing_library_group/CMakeLists.txt (3)
1-5: Header and comment block are consistent with other test files.
6-10: Project configuration is correct
CMAKE_MINIMUM_REQUIRED,PROJECT, andFIND_PACKAGEdirectives are properly specified.
11-14: Include directives validated
Inclusion ofTEST.cmake,CMDEFConfig.cmake, and shared sources is correct relative to this file’s location.test/CMDEF_ENV/CMakeLists.txt (1)
13-17: Test harness invocations look correct
TheTEST_RUNcalls for the core CMDEF_ENV modules are well organized and cover each subdirectory.test/CMDEF_ADD_EXECUTABLE/shared_sources/shared_sources.cmake (1)
10-14: Shared source paths defined correctly
The centralized variables (TEST_RESOURCES_DIR,MAIN_SOURCE_FILE, etc.) are clear and consistent, making it easy for all test cases to reference the same files.test/CMDEF_ADD_EXECUTABLE/test_cases/win32_flag/CMakeLists.txt (1)
11-14: Include paths are correct
The includes forTEST.cmake,CMDEFConfig.cmake, and the shared sources file are properly ordered and resolve to the right directories.test/CMDEF_ADD_LIBRARY/shared_sources/shared_sources.cmake (1)
10-14: Shared library paths defined correctly
Your variables (TEST_RESOURCES_DIR,LIBRARY_SOURCE_FILE, etc.) are clearly named and centralized, simplifying maintenance across all CMDEF_ADD_LIBRARY tests.test/CMDEF_ADD_EXECUTABLE/test_cases/minimal_executable/CMakeLists.txt (1)
23-36: Verify the expected OUTPUT_NAME suffix.You’re checking for
"testminimalexed"but only passedVERSION(no debug suffix documented). Confirm whether the macro really appendsd. If not, adjust the expected value to"testminimalexe".test/CMDEF_ADD_EXECUTABLE/test_cases/custom_output_name/CMakeLists.txt (1)
24-29: Ensure OUTPUT_NAME parameter matches expectations.You set
OUTPUT_NAME "custom-executable-name"but then later expect"custom-executable-named". Confirm whether the macro appends adsuffix in this context or update one of the strings for consistency.test/CMDEF_ENV/os_detection/CMakeLists.txt (1)
14-40: Verify OS-detection macros align with CMDEF_ENV definitions.You reference
CMDEF_OS_MACOSand friends—double-check that these exact variables exist inCMDEF_ENV.cmake(e.g., sometimes the alias isCMDEF_OS_DARWIN). Mismatched names will silently skip branches.test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/CMakeLists.txt (6)
9-17: Test Suite Configuration Looks Good
CMake minimum version, project declaration, package requirement, and shared include files are correctly set up for the parameter validation tests.
18-20: Verify the missing LIBRARY_GROUP error message
Ensure that the string"Key 'LIBRARY_GROUP' is not defined"exactly matches the fatal error emitted by the macro whenLIBRARY_GROUPis omitted.
22-24: Verify the missing VERSION error message
Double-check that"Key 'VERSION' is not defined"corresponds precisely to the macro’s output for a missing version parameter.
26-28: Verify the invalid type error message
Confirm that"Invalid Type 'INVALID'"matches the exact error produced when an unsupported library type is passed.
30-32: Verify SOURCE_BASE_DIRECTORY handling for non-interface libraries
The test expects"SOURCE_BASE_DIRECTORY is not supported for non INTERFACE library". Ensure the macro emits that exact phrase (including casing and spacing).
34-36: Verify missing SOURCES parameter error
Make sure"SOURCES is not defined."aligns exactly with the macro’s fatal error when sources are omitted for a non-INTERFACE library.test/CMDEF_ADD_EXECUTABLE/test_cases/macos_bundle/CMakeLists.txt (3)
6-14: macOS Bundle Test Setup Looks Good
CMake version, project declaration, package find, and shared sources are properly configured.
23-29: Invoke CMDEF_ADD_EXECUTABLE with MACOS_BUNDLE
The parameters (TARGET,SOURCES,VERSION,MACOS_BUNDLE ON) are correctly supplied to the macro.
48-49: Test Invocation
The call toTEST_MACOS_BUNDLE_BEHAVIOR()correctly triggers the function.test/CMDEF_ADD_EXECUTABLE/test_cases/windows_resource_generation/CMakeLists.txt (5)
6-14: Windows Resource Test Setup Good
CMake configuration, project declaration, package requirement, and shared sources inclusion are all in place.
23-29: Skip Logic for Non-Windows Platforms
Correctly skips the resource generation test on non-Windows hosts viaIF(NOT CMDEF_OS_WINDOWS)andRETURN().
29-33: Resource Generation Target Creation
CMDEF_ADD_EXECUTABLEis invoked with the correctTARGET,SOURCES, andVERSIONparameters.
52-54: Resource Missing Guard
Good fallback to emit a fatal error if no.rcsource is found intarget_sources.
57-58: Test Invocation
The call toTEST_WINDOWS_RESOURCE_GENERATION()correctly triggers the resource generation test.test/CMDEF_ADD_EXECUTABLE/test_cases/check_function/CMakeLists.txt (4)
23-28: Executable Creation for Check Function
CMDEF_ADD_EXECUTABLEcorrectly sets up the test executabletestcheckexe.
30-31: Control Executable Creation
ADD_EXECUTABLE(regularcmakeexe ...)provides a non-CMDEF target to validate against.
32-38: Verify CMDEF_ADD_EXECUTABLE_CHECK Behavior
The fatal error conditions ensure the macro identifies the CMDEF target and returns the correct name.
46-47: Test Invocation
TEST_EXECUTABLE_CHECK_FUNCTION()at the end properly executes the check function test.test/CMDEF_ADD_EXECUTABLE/test_cases/executable_with_includes/CMakeLists.txt (5)
6-14: Executable Include Test Setup
CMake version requirement, project name, package find, and shared sources inclusion are correct.
23-31: Include Directories Parameter Usage
INCLUDE_DIRECTORIES "${EXECUTABLE_INCLUDE_DIR}"is passed correctly toCMDEF_ADD_EXECUTABLE.
33-35: Target Existence Check
Validates thattestincludesexeis created before further checks.
37-38: Verify Include Directories in Target Property
TEST_CHECK_TARGET_PROPERTY_CONTAINScorrectly asserts that the include directory has been applied.
41-42: Test Invocation
TEST_EXECUTABLE_WITH_INCLUDES()properly invokes the test function.test/CMDEF_ADD_EXECUTABLE/test_cases/multiple_sources/CMakeLists.txt (6)
6-13: Consistent CMake project setup and includes
The CMake minimum version, project declaration, package requirement, and includes for the common test framework and shared sources are correctly specified.
23-32: Valid invocation of CMDEF_ADD_EXECUTABLE
The macro is invoked with the correct TARGET, SOURCES list, VERSION, and INCLUDE_DIRECTORIES. The syntax and indentation align with the existing test patterns.
34-37: Check for target creation
TheIF(NOT TARGET ...)guard correctly fails the test if the executable target wasn’t created.
38-38: Verify custom property on executable
TEST_CHECK_TARGET_PROPERTYasserts that theCMDEF_EXECUTABLEflag is set as expected.
40-46: Retrieve and define expected sources
The retrieval ofSOURCESand construction of theexpected_sourceslist are implemented correctly.
48-53: Validate presence of all sources
The loop checks each expected source and correctly reports a fatal error if any are missing.test/CMDEF_ADD_EXECUTABLE/test_cases/windows_msvc_runtime/CMakeLists.txt (6)
6-13: Setup CMake project and includes
CMake minimum version, project declaration, package requirement, and includes are correctly specified for Windows MSVC runtime tests.
23-27: Conditional skip on non-Windows platforms
The early return whenCMDEF_OS_WINDOWSis false prevents irrelevant execution on other OSes.
29-33: CMDEF_ADD_EXECUTABLE usage for MSVC runtime test
The executable is created correctly with the main source and version.
35-35: Retrieve MSVC runtime library property
UsingGET_TARGET_PROPERTYto fetchMSVC_RUNTIME_LIBRARYis appropriate here.
37-43: Validate static runtime selection
The checks ensureMultiThreadedprefix and absence ofDLLin static runtime modes.
45-48: Validate dynamic runtime selection
The check asserts presence ofDLLsubstring when dynamic runtime is expected.test/CMDEF_ENV/arch_detection/CMakeLists.txt (7)
6-13: Project setup and includes for environment detection test
CMake minimum version, project declaration, package dependency, and includes of the common test framework andCMDEF_ENV.cmakeare correct.
14-19: Linux architecture detection logic
The test correctly checks thatCMDEF_ARCHITECTUREis in the supported list and fails on a mismatch.
20-23: Windows and macOS architecture assertions
The tests assert architecture values for Windows and macOS. Please verify that"x86-64"and"aplsil"are the intended target values for these platforms.
24-25: Fallback error for undefined OS flag
An immediate fatal error if no OS is detected ensures test robustness.
27-29: Check distribution variables are defined
TEST_VAR_DEFINEDvalidates that the distro ID and version ID variables are present.
30-36: Non-empty Linux distribution identifiers
EnsuresCMDEF_DISTRO_IDandCMDEF_DISTRO_VERSION_IDare non-empty on Linux.
37-43: Windows/macOS distribution value assertions
The test checks expected distro values; confirm that"windows","1","macos", and"11"match the actual detection output.test/CMDEF_ADD_EXECUTABLE/CMakeLists.txt (6)
6-12: Test suite initialization
The minimum CMake version, project context, package find, and inclusion of the common test script are correct.
14-18: Basic functionality test invocations
Calls toTEST_RUNfor minimal and include/custom name tests follow the established pattern and correctly reference test cases.
19-22: Platform-specific test invocations
win32_flagandmacos_bundletests are run unconditionally, which is acceptable for their respective macros.
23-26: Debug suffix test invocations
Debug suffix tests for both build-type and generator-expression approaches are correctly included.
33-36: Parameter combination tests
multiple_sourcesandall_parameterstests are correctly scheduled.
38-56: Inconsistent test case path resolution
Error condition tests useTEST_RUN_AND_CHECK_OUTPUT("test_cases/...")withoutCMAKE_CURRENT_LIST_DIR, while other tests use full paths viaTEST_RUN("${CMAKE_CURRENT_LIST_DIR}/..."). Please verify thatTEST_RUN_AND_CHECK_OUTPUTresolves relative paths correctly, or update to include the list directory prefix for consistency.test/CMDEF_ADD_EXECUTABLE/test_cases/all_parameters/CMakeLists.txt (4)
6-13: Test case setup and includes
CMake minimum version, project declaration, package find, and includes for test framework, config, and shared sources are all correctly specified.
23-36: CMDEF_ADD_EXECUTABLE invocation with all parameters
All optional parameters (SOURCES,VERSION,OUTPUT_NAME,INCLUDE_DIRECTORIES,WIN32,MACOS_BUNDLE) are provided. The macro usage is consistent with other tests.
46-49: Executable type verification
The check forTYPE == "EXECUTABLE"is appropriate to ensure the target type is correct.
51-55: Source count validation
Verifying that at least three source files are associated with the target matches the invocation above.test/CMDEF_ENV/basic_init/CMakeLists.txt (1)
1-54: Comprehensive test coverage for CMDEF_ENV basic initialization.This test file provides excellent coverage of the CMDEF_ENV module's basic initialization functionality. The systematic testing of all expected environment variables (OS detection, architecture, naming conventions, language support, etc.) ensures the module works correctly across different platforms.
system_modules/CMDEF_ENV.cmake (3)
432-442: Improved architecture detection using built-in CMake functionality.Replacing external commands with
CMAKE_SYSTEM_PROCESSORimproves reliability and reduces external dependencies. The added validation ensures proper error handling for empty or invalid architecture strings.
467-477: Enhanced distro ID detection with built-in CMake queries.Using
CMAKE_HOST_SYSTEM_INFORMATION(RESULT _distro_id QUERY DISTRIB_ID)instead of external commands is a solid improvement for portability and reliability.
502-512:❓ Verification inconclusive
Verify the new distro version detection approach.
The switch to
CMAKE_HOST_SYSTEM_INFORMATIONfor distro version detection should be tested across different Linux distributions to ensure compatibility.
🏁 Script executed:
#!/bin/bash # Description: Check if CMAKE_HOST_SYSTEM_INFORMATION supports DISTRIB_VERSION_ID on various systems # Expected: CMake documentation should confirm DISTRIB_VERSION_ID support # Check CMake documentation for CMAKE_HOST_SYSTEM_INFORMATION queries rg -A 10 -B 5 "DISTRIB_VERSION_ID|CMAKE_HOST_SYSTEM_INFORMATION"Length of output: 3204
Verify cross-distro support for DISTRIB_VERSION_ID
Please ensure that the new use of
CMAKE_HOST_SYSTEM_INFORMATION(… QUERY DISTRIB_VERSION_ID)is supported by the versions of CMake and Linux distributions you target. In particular:• Consult your CMake reference (e.g.
cmake_host_system_informationdocs) to confirm that theDISTRIB_VERSION_IDquery key is officially supported in the minimum CMake version you require.
• Test the snippet on representative distributions (Ubuntu, Fedora, CentOS/RHEL, openSUSE, Arch, etc.) to catch any variations in howos-releasefields are exposed.
• Consider adding a fallback (parsing/etc/os-release) if older CMake versions or certain distros do not exposeDISTRIB_VERSION_IDviaCMAKE_HOST_SYSTEM_INFORMATION.test/CMDEF_ADD_LIBRARY/test_cases/version_validation/CMakeLists.txt (3)
23-71: Comprehensive testing of valid version formats.The test systematically validates multiple semantic version formats and ensures targets are created successfully. Good coverage of edge cases like patch zero and larger version numbers.
81-102: Robust version property validation with platform awareness.The test correctly verifies the VERSION property is set and includes appropriate handling for platform-dependent behavior where the property might not be set.
112-134: Proper validation of CMDEF naming conventions.The test ensures that version numbers don't appear directly in output names, confirming that CMDEF uses its own naming scheme correctly.
test/CMDEF_ADD_LIBRARY/test_cases/multiple_include_directories/CMakeLists.txt (2)
25-75: Well-designed test for multiple include directories in shared libraries.The test comprehensively validates that multiple include directories are properly set in both
INTERFACE_INCLUDE_DIRECTORIESandCMDEF_INSTALL_INCLUDE_DIRECTORIESproperties. The flexible path matching approach ensures cross-platform compatibility.
87-119: Appropriate testing for interface library include directories.The test correctly validates that interface libraries handle multiple include directories with proper
INTERFACEvisibility. The separation from shared library testing ensures both scenarios are covered.test/CMDEF_ADD_LIBRARY/test_cases/target_properties_validation/CMakeLists.txt (4)
25-65: Comprehensive INTERFACE library property validation looks solid.
All checks forINTERFACEtargets and BUILD_INTERFACE generator expressions are present and correct.
75-128: SHARED library property tests are thorough and correct.
The script verifies type, version, SOVERSION, prefix, PIC, include handling, and suffix.
137-196: STATIC library property validation is complete.
The test correctly asserts absent PIC, validates sources, output naming, and suffix.
198-200: Invocation of all property-validation functions is correct.
Ensures each validation routine is executed.test/CMDEF_ADD_LIBRARY/test_cases/soversion/CMakeLists.txt (5)
25-50: Valid SOVERSION test covers POSIX and non-POSIX cases appropriately.
Checks target creation, correct SOVERSION on POSIX, and allows omission elsewhere.
60-79: Test for SOVERSION equal to VERSION is accurate on POSIX.
Properly validates matching SOVERSION=VERSION scenario.
90-107: Static library SOVERSION-ignored test is correct.
Ensures SOVERSION is not set for static libraries.
117-133: Interface library SOVERSION-ignored test is correct.
Validates that interface libraries do not receive SOVERSION.
135-138: Test invocation for all SOVERSION cases is correctly orchestrated.test/CMDEF_ADD_LIBRARY/test_cases/check_function/CMakeLists.txt (4)
25-40: Shared-library check function test is correct.
ConfirmsCMDEF_ADD_LIBRARY_CHECKidentifies a CMDEF shared target properly.
50-65: Static-library check function test is correct.
VerifiesCMDEF_ADD_LIBRARY_CHECKreturns the expected static target name.
75-90: Interface-library check function test is correct.
Ensures the check macro recognizes an INTERFACE CMDEF target.
100-107: Regular CMake library check test is correct.
Validates the macro does not falsely identify vanilla CMake targets.test/CMDEF_ADD_LIBRARY/test_cases/interface_library/CMakeLists.txt (2)
60-82: Interface library base-directory test is solid.
ValidatesCMDEF_LIBRARY_SOURCESis set when usingSOURCE_BASE_DIRECTORY.
92-111: Header-only interface library test correctly asserts no sources.
EnsuresCMDEF_LIBRARY_SOURCESremains unset for header-only cases.test/CMDEF_ADD_LIBRARY/test_cases/posix_specific/CMakeLists.txt (2)
15-18: POSIX detection logic looks correct.The early return pattern for non-POSIX systems is appropriate and prevents unnecessary test execution.
65-83:❌ Incorrect review comment
Verify cache variable availability for library suffixes.
The test assumes CMDEF cache variables are available, but should verify they exist first.
Run this script to verify that the required cache variables are properly defined:
🏁 Script executed:
#!/bin/bash # Description: Check if CMDEF cache variables are properly defined in the codebase # Expected: Find definitions for library suffix cache variables echo "Searching for CMDEF library suffix variable definitions..." rg -A 3 -B 3 "CMDEF_LIBRARY_NAME_SUFFIX_(SHARED|STATIC)" --type cmakeLength of output: 15328
Redundant existence check for suffix variables
The
CMDEF_LIBRARY_NAME_SUFFIX_SHAREDandCMDEF_LIBRARY_NAME_SUFFIX_STATICvariables are defined and validated insystem_modules/CMDEF_ENV.cmakeand covered by the existingbasic_inittests undertest/CMDEF_ENV/basic_init. Explicitly verifying their presence in the POSIX‐specific test is therefore unnecessary.Likely an incorrect or invalid review comment.
test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt (2)
48-52: Architecture validation logic is robust.The supported architecture list and validation logic correctly handles platform-specific architectures.
335-368: Environment variable testing approach is appropriate.The test correctly validates environment variable support without modifying global state, which is a good practice for test isolation.
test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt (2)
157-195: Platform suffix integration test is well-structured.The test correctly validates that platform-specific suffixes are applied consistently across library types.
134-138: 🛠️ Refactor suggestionDebug suffix detection logic needs improvement.
The current regex pattern for detecting debug suffixes may be fragile and could produce false positives.
# The double d needs to be matched here to not confuse shareD for a debug suffix GET_TARGET_PROPERTY(release_output_name testrelease-shared OUTPUT_NAME) - IF(release_output_name MATCHES "d${CMDEF_LIBRARY_NAME_DEBUG_SUFFIX}$") + # More precise check: ensure debug suffix appears at the end before any file extension + IF(release_output_name MATCHES "${CMDEF_LIBRARY_NAME_DEBUG_SUFFIX}$") MESSAGE(FATAL_ERROR "Release library OUTPUT_NAME should NOT contain debug suffix '${CMDEF_LIBRARY_NAME_DEBUG_SUFFIX}', got: ${release_output_name}") ENDIF()Likely an incorrect or invalid review comment.
test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/invalid_type/CMakeLists.txt
Show resolved
Hide resolved
test/CMDEF_ADD_EXECUTABLE/error_test_sources/invalid_syntax.cpp
Outdated
Show resolved
Hide resolved
test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt
Show resolved
Hide resolved
test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt
Show resolved
Hide resolved
...D_LIBRARY/test_cases/parameter_validation/source_base_directory_non_interface/CMakeLists.txt
Show resolved
Hide resolved
test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/CMDEF_ADD_LIBRARY/test_cases/generator_expressions/CMakeLists.txt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
test/CMDEF_ENV/README.md (1)
17-23:⚠️ Potential issueUsage instructions incomplete
Runningcmake .only configures the project—you still need to build and invoke the tests (e.g., viactest).Apply this diff to clarify the workflow:
- Run all CMDEF_ENV tests: - ```bash - cmake . - ``` + ## Usage + + To configure, build, and run all CMDEF_ENV tests: + ```bash + cd test/CMDEF_ENV + mkdir -p build && cd build + cmake .. + cmake --build . --target test + ctest + ```
🧹 Nitpick comments (2)
test/CMDEF_ENV/README.md (2)
3-10: Link bullets to test directories
The “Functionality Covered by Tests” list is comprehensive. Consider linking each bullet to its correspondingtest/CMDEF_ENV/*subfolder for faster navigation.
11-16: Document rationale for untested scenarios
Great callout of “Not Covered” items. You may want to briefly note why these are excluded or outline plans for future coverage.🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: Possible missing preposition found.
Context: ...errides - Cross-compilation scenarios - Non-standard architectures ## Usage Run a...(AI_HYDRA_LEO_MISSING_IN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/CMDEF_ADD_EXECUTABLE/README.md(1 hunks)test/CMDEF_ADD_LIBRARY/README.md(1 hunks)test/CMDEF_ENV/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/CMDEF_ADD_LIBRARY/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/CMDEF_ADD_EXECUTABLE/README.md
🧰 Additional context used
🪛 LanguageTool
test/CMDEF_ENV/README.md
[uncategorized] ~15-~15: Possible missing preposition found.
Context: ...errides - Cross-compilation scenarios - Non-standard architectures ## Usage Run a...
(AI_HYDRA_LEO_MISSING_IN)
🔇 Additional comments (1)
test/CMDEF_ENV/README.md (1)
1-2: Header Clarity
The top‐level heading succinctly identifies the test suite.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
test/CMDEF_INSTALL/shared_sources/src/main.cpp (1)
3-5: Exit-code is being used to transmit a version – check that your harness interprets non-zero exit codes as success
main()returns the value ofget_library_version(). On many platforms any non-zero exit code is treated as a failure by CTest / CI shells, so a version like100could falsely mark the test as failed.
If the surrounding test framework really relies on the exit code, keep it; otherwise consider emitting the version to stdout/stderr and returning0.- library_function(); - return get_library_version(); + library_function(); + std::cout << get_library_version() << std::endl; + return 0; # successtest/CMDEF_INSTALL/test_cases/install_related/interface_lib/CMakeLists.txt (1)
40-43: Regex may break for paths containing “.” or “+” etc.; quote or escape${CMDEF_SOURCE_INSTALL_DIR}
IF(NOT files_calls MATCHES "FILES.*DESTINATION.*${CMDEF_SOURCE_INSTALL_DIR}")
MATCHESinterprets the RHS as a regular expression, so any meta-chars inside${CMDEF_SOURCE_INSTALL_DIR}(e.g..) change the pattern meaning.
Wrap the variable in[[ … ]]or usestring(REGEX MATCH ...)withescapebeforehand.string(REGEX REPLACE "([][+.*?^$()|\\])" "\\\\\\1" ESCAPED_SRC_DIR "${CMDEF_SOURCE_INSTALL_DIR}") if(NOT files_calls MATCHES "FILES.*DESTINATION.*${ESCAPED_SRC_DIR}")test/CMDEF_INSTALL/test_cases/install_related/multiple_calls/CMakeLists.txt (1)
49-54: Semicolon-specific regex may miss real INSTALL(TARGETS) calls
TEST_GET_INSTALL_TARGETS_CALLS()records arguments separated by semicolons on most platforms, but CMake may collapse consecutive arguments during stringification, yielding eitherTARGETS mlib1-sharedorTARGETS;mlib1-shared.
For resilience, loosen the pattern or add an alternative match:- IF(NOT targets_calls MATCHES "TARGETS;mlib1-shared") + IF(NOT targets_calls MATCHES "TARGETS[; ]mlib1-shared")Apply the same tweak for
mlib2-staticand for the export checks below.test/CMDEF_INSTALL/library_tests.cmake (1)
25-25: Hard-coded double slash in export destination
"${CMDEF_LIBRARY_INSTALL_DIR}/cmake//"produces a path ending with a redundant slash (“…/cmake//”).
While CMake tolerates this, it is brittle and can break simple string comparisons on some CI environments.- TEST_INSTALL_EXPORT_DESTINATION_EQUALS("${CMDEF_LIBRARY_INSTALL_DIR}/cmake//") + TEST_INSTALL_EXPORT_DESTINATION_EQUALS("${CMDEF_LIBRARY_INSTALL_DIR}/cmake/")test/CMDEF_INSTALL/test_cases/install_related/with_configs/CMakeLists.txt (1)
40-44: Consider assertingLIBRARY/RUNTIMEdestinations as wellFor a shared library the primary artefacts are
LIBRARY( *.so / *.dylib ) andRUNTIME
( *.dll ). Verifying only theARCHIVEdestination may miss regressions on Unix‐like
platforms where the import archive is not generated.If it is intentional to test only the archive case, please add a short comment explaining
why. Otherwise:TEST_INSTALL_TARGETS_DESTINATION_EQUALS("LIBRARY" "${CMDEF_LIBRARY_INSTALL_DIR}") TEST_INSTALL_TARGETS_DESTINATION_EQUALS("RUNTIME" "${CMDEF_BINARY_INSTALL_DIR}")test/CMDEF_INSTALL/README.md (2)
7-8: Minor wording tweak for clarity“In INSTALL command override mechanism” reads a little awkward.
Replacing the noun-use of INSTALL by theinstall()command makes the sentence clearer.-The tests use an INSTALL command override mechanism to intercept and verify ... +The tests use an `install()` command override mechanism to intercept and verify ...🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: The word ‘INSTALL’ is not a noun.
Context: ...rs. ## Test Strategy The tests use an INSTALL command override mechanism to intercept...(A_INSTALL)
76-76: Missing articleSmall grammar nit:
-Tests are executed via the main CMakeLists.txt using TEST_RUN() function: +Tests are executed via the main CMakeLists.txt using the TEST_RUN() function:🧰 Tools
🪛 LanguageTool
[uncategorized] ~76-~76: You might be missing the article “the” here.
Context: ...cuted via the main CMakeLists.txt using TEST_RUN() function: ```cmake TEST_RUN("${CMAKE...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
test/CMDEF_INSTALL/install_override.cmake (1)
201-208: Fragile regex forinstall(EXPORT …)destinationThe pattern assumes exactly two configuration names and a fixed argument order:
EXPORT;name;CONFIGURATIONS;Debug;Release;DESTINATION;pathCMake permits any number of configurations and free argument ordering, so the check can
yield false negatives.
Consider:string(REGEX MATCH "DESTINATION;([^;]+)" match "${single_call}")after isolating each call, or parse the list positionally with
list(FIND …).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
test/CMDEF_INSTALL/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/README.md(1 hunks)test/CMDEF_INSTALL/install_override.cmake(1 hunks)test/CMDEF_INSTALL/library_tests.cmake(1 hunks)test/CMDEF_INSTALL/shared_sources/include/library.h(1 hunks)test/CMDEF_INSTALL/shared_sources/shared_sources.cmake(1 hunks)test/CMDEF_INSTALL/shared_sources/src/library.cpp(1 hunks)test/CMDEF_INSTALL/shared_sources/src/main.cpp(1 hunks)test/CMDEF_INSTALL/test_cases/install_related/executable/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/install_related/include_dirs/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/install_related/interface_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/install_related/multiple_calls/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/install_related/no_config/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/install_related/shared_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/install_related/static_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/install_related/with_configs/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/install_related/with_namespace/CMakeLists.txt(1 hunks)test/CMakeLists.txt(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- test/CMDEF_INSTALL/shared_sources/include/library.h
- test/CMDEF_INSTALL/shared_sources/src/library.cpp
- test/CMDEF_INSTALL/shared_sources/shared_sources.cmake
- test/CMDEF_INSTALL/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- test/CMakeLists.txt
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/CMDEF_INSTALL/shared_sources/src/main.cpp (2)
test/CMDEF_INSTALL/shared_sources/include/library.h (2)
library_function(4-4)get_library_version(3-3)test/CMDEF_INSTALL/shared_sources/src/library.cpp (4)
library_function(7-9)library_function(7-7)get_library_version(3-5)get_library_version(3-3)
🪛 LanguageTool
test/CMDEF_INSTALL/README.md
[grammar] ~7-~7: The word ‘INSTALL’ is not a noun.
Context: ...rs. ## Test Strategy The tests use an INSTALL command override mechanism to intercept...
(A_INSTALL)
[uncategorized] ~76-~76: You might be missing the article “the” here.
Context: ...cuted via the main CMakeLists.txt using TEST_RUN() function: ```cmake TEST_RUN("${CMAKE...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (4)
test/CMDEF_INSTALL/test_cases/install_related/executable/CMakeLists.txt (1)
28-36: Looks good – tests basic executable installation pathTarget creation, versioning and the subsequent
CMDEF_INSTALLinvocation are clear and minimal.
No issues spotted.test/CMDEF_INSTALL/test_cases/install_related/with_namespace/CMakeLists.txt (1)
34-47: Namespace verification logic is clearNice coverage of both
INSTALL(TARGETS)andINSTALL(EXPORT)semantics with namespace handling.
No actionable issues.test/CMDEF_INSTALL/test_cases/install_related/shared_lib/CMakeLists.txt (1)
28-33: LGTM – shared-library creation is consistentThe shared-library test mirrors the static-library scenario and invokes
LIBRARY_TESTSon the installed artefact. Looks sound.test/CMDEF_INSTALL/install_override.cmake (1)
28-33: Shadowing the built-ininstall()command can break unrelated codeDefining a macro called
INSTALLhides the built-in command for everything that is
processed after this file is included, but not for code parsed earlier (e.g.
insidefind_package).
If any prerequisite module emits realinstall()calls before the override is loaded
they will bypass your recording logic, reducing test coverage.Mitigation options:
- Include this module before any third-party CMake code is processed in every test, or
- Rename the macro (e.g.
CMDEF_TEST_INSTALL) and wrap the originalinstall()via
cmake_language(CALL ...)to guarantee interception order-independently.
test/CMDEF_INSTALL/test_cases/install_related/include_dirs/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/CMDEF_INSTALL/test_cases/install_related/static_lib/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/CMDEF_INSTALL/test_cases/install_related/with_configs/CMakeLists.txt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
test/CMDEF_INSTALL/test_cases/static_lib/CMakeLists.txt (1)
23-23: Remove placeholder comment.
The placeholder# <function>()is not used; please remove or replace it to keep the script clean.test/CMDEF_INSTALL/test_cases/include_dirs/CMakeLists.txt (1)
23-23: Remove placeholder comment.
The placeholder# <function>()is unused; please remove it.test/CMDEF_INSTALL/test_cases/multiple_calls/CMakeLists.txt (3)
11-15: Consider centralizing include paths.
Hardcoding multiple levels of../in yourINCLUDE()directives is brittle. Introduce a variable (e.g.,TEST_ROOT_DIRor useCMAKE_SOURCE_DIR) to construct these paths more reliably.
22-23: Remove leftover placeholder comment.
The# <function>()line is a remnant placeholder and should be removed to avoid confusion.
43-46: Verify install destinations per target.
You check the install destination only once, which may miss per-target discrepancies. Add assertions for bothmlib1-sharedandmlib2-staticto ensure each has the correctARCHIVEandLIBRARYpaths.test/CMDEF_INSTALL/test_cases/with_namespace/CMakeLists.txt (2)
22-23: Remove placeholder comment.
The# <function>()line is unused—please drop it to keep the script clean.
34-35: Strengthen verification of namespace propagation.
You passNAMESPACE nlib::toCMDEF_INSTALLbut only use high-level helpers for validation. Consider callingTEST_GET_INSTALL_EXPORT_CALLS(export_calls)and inspecting the raw recorded calls to ensure the namespace flag is captured verbatim.test/CMDEF_INSTALL/test_cases/no_config/CMakeLists.txt (1)
22-23: Remove placeholder comment.
Drop the# <function>()placeholder to streamline the test script.test/CMDEF_INSTALL/test_cases/with_configs/CMakeLists.txt (1)
22-23: Remove placeholder comment.
The# <function>()line is a leftover and should be removed for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
test/CMDEF_INSTALL/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/executable/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/include_dirs/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/interface_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/multiple_calls/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/no_config/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/shared_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/static_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/with_configs/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/with_namespace/CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/CMDEF_INSTALL/CMakeLists.txt
🔇 Additional comments (3)
test/CMDEF_INSTALL/test_cases/static_lib/CMakeLists.txt (2)
11-16: Verify include paths correctness.
The relative offsets forINCLUDE("${CMAKE_CURRENT_LIST_DIR}/../../../TEST.cmake")and../../../../CMDEFConfig.cmakemay not resolve to the intended test and config scripts; please confirm these paths point to the correct locations.
35-36: Validate test target name for static library.
The invocationLIBRARY_TESTS("alib-interface")may be targeting the wrong suffix—ensure the test helper is pointing at the static library target (e.g.,"alib-static").test/CMDEF_INSTALL/test_cases/executable/CMakeLists.txt (1)
34-35: Confirm test helper usage for executables.
LIBRARY_TESTS("texe")is designed for library targets—verify if executables should use a dedicated test helper or iflibrary_tests.cmakecorrectly covers executable installation checks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/CMDEF_INSTALL/test_cases/with_configs/CMakeLists.txt (1)
47-47: Remove unintended double slash in export destination
The path${CMDEF_LIBRARY_INSTALL_DIR}/cmake//contains a superfluous “/”. This breaks strict-path equality checks.- TEST_INSTALL_EXPORT_DESTINATION_EQUALS("${CMDEF_LIBRARY_INSTALL_DIR}/cmake//") + TEST_INSTALL_EXPORT_DESTINATION_EQUALS("${CMDEF_LIBRARY_INSTALL_DIR}/cmake/")test/CMDEF_INSTALL/test_cases/interface_lib/CMakeLists.txt (1)
92-95:⚠️ Potential issueFix variable expansion in
IF()condition
files_callsis evaluated as a literal; quote the variable to match its value.- IF(NOT files_calls MATCHES "FILES.*DESTINATION.*${CMDEF_SOURCE_INSTALL_DIR}") + IF(NOT "${files_calls}" MATCHES "FILES.*DESTINATION.*${CMDEF_SOURCE_INSTALL_DIR}")
🧹 Nitpick comments (2)
test/CMDEF_INSTALL/install_test_macros.cmake (1)
35-43: Robustness: handleNOTFOUNDreturn fromget_target_propertyWhen the property is unset, CMake assigns the literal string
NOTFOUND, which bypasses the current emptiness check and produces a misleading match failure later on.- IF(NOT include_dirs) + IF(NOT include_dirs OR "${include_dirs}" STREQUAL "NOTFOUND") MESSAGE(FATAL_ERROR "Target ${target} has no ${property_name} property") ENDIF()Apply the same guard wherever
GET_TARGET_PROPERTYis used in this file.test/CMDEF_INSTALL/test_cases/shared_lib/CMakeLists.txt (1)
16-17: Duplicate include ofinstall_test_macros.cmake
install_test_macros.cmakeis included twice back-to-back.
This is harmless but noisy and can slow configure time in larger projects.-INCLUDE("${CMAKE_CURRENT_LIST_DIR}/../../install_test_macros.cmake") -INCLUDE("${CMAKE_CURRENT_LIST_DIR}/../../install_test_macros.cmake") +INCLUDE("${CMAKE_CURRENT_LIST_DIR}/../../install_test_macros.cmake")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
test/CMDEF_INSTALL/install_test_macros.cmake(1 hunks)test/CMDEF_INSTALL/test_cases/executable/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/include_dirs/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/interface_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/multiple_calls/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/no_config/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/shared_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/static_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/with_configs/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/with_namespace/CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/CMDEF_INSTALL/test_cases/no_config/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/multiple_calls/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/static_lib/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/include_dirs/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/with_namespace/CMakeLists.txt
🔇 Additional comments (1)
test/CMDEF_INSTALL/test_cases/executable/CMakeLists.txt (1)
34-37: Verify thatLIBRARY_TESTS()supports executable targets
LIBRARY_TESTS("texe")is designed for library-specific assertions; if it assumesARCHIVE/RUNTIMEartifacts it may silently skip or mis-validate executable installs.
Confirm the helper handles executable targets or introduce anEXECUTABLE_TESTS()wrapper to avoid false positives.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
test/CMDEF_PACKAGE/test_cases/executable/CMakeLists.txt (1)
70-102: Duplicate issue – same build-type restoration logicThe logic described in the previous comment repeats here. Apply the same guard/restore pattern.
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/CMakeLists.txt (2)
26-70: Build-type restoration suffers from the same bugThe
original_build_typehandling follows the same pattern as in the executable test and will always take the “restore” branch. Adopt the guarded approach suggested earlier to avoid caching an empty string.
84-111: Second occurrence of build-type restoration issueReplicates the same pattern; please update consistently.
test/CMDEF_PACKAGE/test_cases/with_configs/CMakeLists.txt (2)
28-61: SameCMAKE_BUILD_TYPErestoration problemThe guard/restore logic needs the fix already pointed out in the executable test case to prevent polluting the cache.
77-109: Repeated occurrencePlease apply the corrected pattern here as well.
🧹 Nitpick comments (19)
test/CMDEF_INSTALL/README.md (2)
3-6: Strengthen the Test Strategy description
Consider using active voice for clarity. For example:
“The tests override the INSTALL command to intercept and verify all INSTALL calls made by CMDEF_INSTALL.”🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: The word ‘INSTALL’ is not a noun.
Context: ...sts ## Test Strategy The tests use an INSTALL command override mechanism to intercept...(A_INSTALL)
37-48: Add missing article for consistency
Change “using TEST_RUN() function” to “using the TEST_RUN() function” in the Running Tests section.🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: You might be missing the article “the” here.
Context: ...cuted via the main CMakeLists.txt using TEST_RUN() function: ```cmake TEST_RUN("${CMAKE...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
test/install_override.cmake (2)
131-143: Regex may produce false positives across concatenated callsAll
INSTALL(TARGETS)calls are concatenated into one long semicolon-separated list.
STRING(REGEX MATCH "${destination_type};DESTINATION;([^;]+)" …)will happily match across call boundaries, so aDESTINATIONbelonging to another call can satisfy the check. Consider storing each call as a separate list element (e.g. prepend an item counter) or splitting${calls}by a sentinel and iterating; then run the match per entry.No change needed for immediate correctness, but test precision improves and future maintenance becomes easier.
109-116: Reset the count property withUNSET
TEST_CLEAR_INSTALL_CALLS()sets the counter to0. When the tests later query the property they cannot distinguish “was never called” from “was cleared”. Clearing to an undefined state makes assertions more robust.- SET_PROPERTY(GLOBAL PROPERTY CMDEF_TEST_INSTALL_COUNT 0) + UNSET_PROPERTY(GLOBAL PROPERTY CMDEF_TEST_INSTALL_COUNT)test/CMDEF_PACKAGE/README.md (1)
47-56: Minor wording nitpick“executed via the main CMakeLists.txt” – missing article.
-Tests are executed via the main CMakeLists.txt using TEST_RUN() function: +Tests are executed via the main CMakeLists.txt using the TEST_RUN() function:🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ...cuted via the main CMakeLists.txt using TEST_RUN() function: ```cmake TEST_RUN("${CMAKE...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
test/CMDEF_PACKAGE/test_cases/shared_lib/CMakeLists.txt (3)
11-15: Ensure include paths resolve correctly.The multiple
../../segments can be brittle across environments. Consider centralizing the base test directory in a variable to reduce duplication and improve maintainability.
25-68: Restore CMAKE_BUILD_TYPE accurately.Since
original_build_typewill always be defined (even if empty), theIF(DEFINED original_build_type)branch will always trigger. Use a non-empty check (IF(NOT "${original_build_type}" STREQUAL "")) to correctly unset the cache when the build type was not originally set.
73-108: Add PASS message for properties test.For consistency in test output, include a
MESSAGE(STATUS "Shared library package properties test passed")beforeENDFUNCTION()inTEST_SHARED_LIBRARY_PACKAGE_PROPERTIES.test/CMDEF_PACKAGE/test_cases/multiconfig_build/CMakeLists.txt (2)
25-51: Refine dependency property check.The macro retrieves dependencies via
MANUALLY_ADDED_DEPENDENCIES. To ensure compatibility, consider usingget_target_property(... PACKAGE PROPERTY DEPENDS)which directly reflects theDEPENDSargument ofadd_custom_target.
61-99: Improve build-type restoration.The restoration logic uses
IF(DEFINED original_build_type)butoriginal_build_typeis always defined. Switch toIF(NOT "${original_build_type}" STREQUAL "")to properly unset when there was no initial build type.test/CMDEF_PACKAGE/test_cases/interface_lib/CMakeLists.txt (2)
25-64: Verify interface include propagation.While the test checks install paths and metadata, adding an assertion for the
INTERFACE_INCLUDE_DIRECTORIESin the generated config file would ensure include dirs are correctly exported for interface libraries.
74-108: Consistent build-type restore.Apply the same non-empty
original_build_typecheck here as in other tests to accurately reset the cache when the build type wasn't defined initially.test/CMDEF_PACKAGE/test_cases/static_lib/CMakeLists.txt (2)
25-63: Assert absence of shared artifacts.Consider adding a test to verify that no shared library artifacts (e.g.,
.soor.dll) are produced for static library packaging, ensuring only static archives are included.
73-108: Build-type restoration refinement.Use
IF(NOT "${original_build_type}" STREQUAL "")rather thanDEFINEDto correctly restore or unsetCMAKE_BUILD_TYPEbased on its initial state.test/CMDEF_PACKAGE/test_cases/with_cpack_config/CMakeLists.txt (2)
25-65: Confirm custom CPack config usage.It would strengthen the test to verify that
CPACK_GENERATORfrom the custom config file (e.g.,"ZIP") is applied, possibly by adding a helper macro to inspect the loaded CPack variables in the test environment.
75-113: Add PASS message and refine restore logic.
- Include a
MESSAGE(STATUS "Custom CPack config properties test passed")beforeENDFUNCTION()for consistent test feedback.- Switch build-type restoration to check for a non-empty
original_build_typeto properly handle unset initial states.test/CMDEF_PACKAGE/package_test_macros.cmake (3)
56-63: File existence not re-checked beforeFILE(READ)
TEST_PACKAGE_CONFIG_FILE_CONTAINSassumes a previous call verified the file exists.
If it is invoked stand-alone (or the earlier check is removed)FILE(READ)will raise its own fatal error, giving a less explicit message.Consider an early guard to keep the macro self-contained:
MACRO(TEST_PACKAGE_CONFIG_FILE_CONTAINS target_name expected_content) SET(config_file "${CMAKE_CURRENT_BINARY_DIR}/${target_name}Config.cmake") + IF(NOT EXISTS "${config_file}") + MESSAGE(FATAL_ERROR "Package config file not found: ${config_file}") + ENDIF() FILE(READ "${config_file}" file_content)
75-82: Same robustness issue as above for the version fileReplicate the existence check here to avoid brittle call ordering.
SET(version_file "${CMAKE_CURRENT_BINARY_DIR}/${target_name}ConfigVersion.cmake") + IF(NOT EXISTS "${version_file}") + MESSAGE(FATAL_ERROR "Package version file not found: ${version_file}") + ENDIF() FILE(READ "${version_file}" file_content)
1-26: Preferfunction()overmacro()to avoid global variable pollutionAll helper blocks currently use
MACRO.
Because macros share scope with their caller, variables likefound_pos,target_found, etc., leak into the parent scope and can accidentally interfere with subsequent tests.Switching to
function()gives each helper its own local scope while preserving call syntax:-MACRO(TEST_PACKAGE_CONFIG_FILE_EXISTS target_name) +function(TEST_PACKAGE_CONFIG_FILE_EXISTS target_name) ... -ENDMACRO() +endfunction()(The same change applies to every helper.)
No behavioural change, but safer and easier to maintain.Also applies to: 28-161
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
system_modules/CMDEF_ADD_LIBRARY.cmake(1 hunks)test/CMDEF_INSTALL/README.md(1 hunks)test/CMDEF_INSTALL/shared_sources/shared_sources.cmake(1 hunks)test/CMDEF_INSTALL/test_cases/executable/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/include_dirs/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/interface_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/multiple_calls/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/no_config/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/shared_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/static_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/with_configs/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/with_namespace/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/README.md(1 hunks)test/CMDEF_PACKAGE/package_test_macros.cmake(1 hunks)test/CMDEF_PACKAGE/shared_sources/include/library.h(1 hunks)test/CMDEF_PACKAGE/shared_sources/shared_sources.cmake(1 hunks)test/CMDEF_PACKAGE/shared_sources/src/library.cpp(1 hunks)test/CMDEF_PACKAGE/shared_sources/src/main.cpp(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/executable/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/interface_lib/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/multiconfig_build/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/shared_lib/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/static_lib/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/with_configs/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/with_cpack_config/CMakeLists.txt(1 hunks)test/CMakeLists.txt(1 hunks)test/TEST.cmake(1 hunks)test/install_override.cmake(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- test/CMDEF_PACKAGE/shared_sources/include/library.h
- test/CMDEF_PACKAGE/shared_sources/src/library.cpp
- test/CMDEF_PACKAGE/shared_sources/shared_sources.cmake
- test/CMDEF_PACKAGE/CMakeLists.txt
- system_modules/CMDEF_ADD_LIBRARY.cmake
- test/CMDEF_PACKAGE/shared_sources/src/main.cpp
🚧 Files skipped from review as they are similar to previous changes (12)
- test/CMDEF_INSTALL/shared_sources/shared_sources.cmake
- test/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/static_lib/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/executable/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/with_configs/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/include_dirs/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/with_namespace/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/no_config/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/multiple_calls/CMakeLists.txt
- test/TEST.cmake
- test/CMDEF_INSTALL/test_cases/shared_lib/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/interface_lib/CMakeLists.txt
🧰 Additional context used
🪛 LanguageTool
test/CMDEF_INSTALL/README.md
[grammar] ~5-~5: The word ‘INSTALL’ is not a noun.
Context: ...sts ## Test Strategy The tests use an INSTALL command override mechanism to intercept...
(A_INSTALL)
[uncategorized] ~39-~39: You might be missing the article “the” here.
Context: ...cuted via the main CMakeLists.txt using TEST_RUN() function: ```cmake TEST_RUN("${CMAKE...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
test/CMDEF_PACKAGE/README.md
[grammar] ~5-~5: The word ‘INSTALL’ is not a noun.
Context: ...sts ## Test Strategy The tests use an INSTALL command override mechanism to intercept...
(A_INSTALL)
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ...cuted via the main CMakeLists.txt using TEST_RUN() function: ```cmake TEST_RUN("${CMAKE...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (4)
test/CMDEF_INSTALL/README.md (4)
1-2: Title looks good
The main heading is clear and follows project conventions.
7-14: Test Structure section is well-organized
The breakdown of core files intoinstall_override.cmake,shared_sources/, andtest_cases/is clear and concise.
15-28: Test Cases enumeration is comprehensive
All key scenarios are covered and numbered consistently.
29-36: Environment variable mappings are clearly documented
The CMDEF_ENV variable list accurately reflects each install destination.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
test/CMDEF_PACKAGE/test_cases/shared_lib/CMakeLists.txt (6)
23-23: Remove placeholder comment.The
# <function>()line aboveTEST_SHARED_LIBRARY_PACKAGEis a leftover template. Update it to a meaningful description or remove it.
28-29: Extract build-type setup/restore logic.The pattern of saving and forcing
CMAKE_BUILD_TYPEthen restoring it is duplicated. Introduce a helper macro (e.g.,TEST_WITH_BUILD_TYPE) to wrap this logic and reduce boilerplate.Also applies to: 49-53
64-64: Remove placeholder comment.Similarly, the
# <function>()aboveTEST_SHARED_LIBRARY_PACKAGE_PROPERTIESshould be replaced with a real description or removed.
87-94: Add success message for properties test.To mirror the first test’s feedback, include a
MESSAGE(STATUS "Shared library package properties test passed")before ending the function.
96-98: Ensure trailing newline.Add a blank line or newline at end-of-file to comply with POSIX standards and avoid CMake warnings.
7-7: Specify project languages explicitly.Consider updating
PROJECT(CMDEF_PACKAGE_SHARED_LIB_TEST)to includeLANGUAGES C CXX(or others as needed) to avoid implicit defaults.test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt (1)
135-138: Hard-coded leadingdmakes the debug-suffix test fragilePre-pending an extra
dassumes the debug suffix is literally “d”. If
CMDEF_LIBRARY_NAME_DEBUG_SUFFIXchanges (e.g. to_dbg), the test will give
false results. Use a word-boundary or simple anchor instead:- IF(release_output_name MATCHES "d${CMDEF_LIBRARY_NAME_DEBUG_SUFFIX}$") + IF(release_output_name MATCHES "${CMDEF_LIBRARY_NAME_DEBUG_SUFFIX}$")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
test/CMDEF_ADD_EXECUTABLE/test_cases/debug_suffix_build_type/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/generator_expression_suffix/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/generator_expressions/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/package_test_macros.cmake(1 hunks)test/CMDEF_PACKAGE/shared_sources/custom_cpack_config.cmake(1 hunks)test/CMDEF_PACKAGE/shared_sources/shared_sources.cmake(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/executable/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/interface_lib/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/multiconfig_build/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/shared_lib/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/static_lib/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/with_configs/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/with_cpack_config/CMakeLists.txt(1 hunks)test/cache_var.cmake(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/CMDEF_PACKAGE/shared_sources/custom_cpack_config.cmake
🚧 Files skipped from review as they are similar to previous changes (12)
- test/CMDEF_ADD_EXECUTABLE/test_cases/debug_suffix_build_type/CMakeLists.txt
- test/CMDEF_PACKAGE/shared_sources/shared_sources.cmake
- test/CMDEF_PACKAGE/test_cases/executable/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/static_lib/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/interface_lib/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/with_configs/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/multiconfig_build/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/with_cpack_config/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/test_cases/generator_expressions/CMakeLists.txt
- test/CMDEF_PACKAGE/package_test_macros.cmake
- test/CMDEF_ADD_EXECUTABLE/test_cases/generator_expression_suffix/CMakeLists.txt
🔇 Additional comments (2)
test/CMDEF_PACKAGE/test_cases/shared_lib/CMakeLists.txt (2)
9-9: Specify CMLIB version requirement.You currently call
FIND_PACKAGE(CMLIB REQUIRED)without pinning a version. To ensure the test runs against a compatible API, please add a minimum version or confirm that any installed CMLIB version will work.
73-73: Confirm group naming in second test.The
LIBRARY_GROUP shplibhere differs fromshlibin the first test. Please verify thatshplib-sharedis the intended target name.
test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt
Show resolved
Hide resolved
test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
test/CMDEF_PACKAGE/test_cases/invalid_arguments/CMakeLists.txt (1)
30-32: Duplicate: target validation check.Same fatal error message as
invalid_main_target; your coverage here is consistent.
🧹 Nitpick comments (5)
test/CMDEF_PACKAGE/test_cases/invalid_arguments/missing_main_target/CMakeLists.txt (1)
11-11: Brittle include path.
The hard-coded relative path with multiple..levels is prone to break if the directory structure changes. Consider appending the parent directory toCMAKE_MODULE_PATHand usingfind_package(CMDEF CONFIG REQUIRED)instead of a directINCLUDE.test/CMDEF_PACKAGE/test_cases/invalid_arguments/invalid_main_target/CMakeLists.txt (1)
11-11: Brittle include path.
Mirror the suggestion from the missing-main-target test: consider simplifying the path by leveragingCMAKE_MODULE_PATHandfind_packageto locateCMDEFConfig.cmake.test/CMDEF_PACKAGE/test_cases/invalid_arguments/missing_version/CMakeLists.txt (1)
11-12: Consolidate include directives.
You’re including bothCMDEFConfig.cmakeandshared_sources.cmakevia different relative-depth paths. Consider standardizing viaCMAKE_MODULE_PATHor grouping includes at the top for consistency.test/CMDEF_PACKAGE/test_cases/invalid_arguments/main_target_not_cmake_target/CMakeLists.txt (1)
11-11: Brittle include path.
Same as above: the deep relative path is fragile. Usefind_packageor adjustCMAKE_MODULE_PATHto simplify locatingCMDEFConfig.cmake.test/CMDEF_PACKAGE/CMakeLists.txt (1)
13-21: DRY the test runs.
Consider defining a list of test case names and iterating over them with aforeachloop to callTEST_RUN, reducing boilerplate and simplifying future additions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
system_modules/CMDEF_PACKAGE.cmake(1 hunks)test/CMDEF_PACKAGE/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/invalid_arguments/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/invalid_arguments/invalid_main_target/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/invalid_arguments/invalid_version_format/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/invalid_arguments/main_target_not_cmake_target/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/invalid_arguments/missing_main_target/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/invalid_arguments/missing_version/CMakeLists.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- system_modules/CMDEF_PACKAGE.cmake
🔇 Additional comments (12)
test/CMDEF_PACKAGE/test_cases/invalid_arguments/missing_version/CMakeLists.txt (1)
14-19: Verify target name.
Ensure thatCMDEF_ADD_LIBRARY(... TYPE SHARED ...)creates a target namedtestlib-shared, matching what’s passed toMAIN_TARGET.test/CMDEF_PACKAGE/CMakeLists.txt (2)
11-11: Verify include path for TEST.cmake.
Confirm thatINCLUDE("${CMAKE_CURRENT_LIST_DIR}/../TEST.cmake")correctly references the shared test utilities from this location.
21-21: Confirm invalid-arguments discovery.
Ensure thatTEST_RUN("${CMAKE_CURRENT_LIST_DIR}/test_cases/invalid_arguments")recurses into each subdirectory and executes all invalid-argument tests. If not, explicitTEST_RUNcalls per subfolder may be required.test/CMDEF_PACKAGE/test_cases/invalid_arguments/invalid_version_format/CMakeLists.txt (4)
6-14: Approve test harness setup.CMake minimum version, project declaration,
FIND_PACKAGE, and includes useCMAKE_CURRENT_LIST_DIRpaths correctly for this nested test.
15-20: Approve library definition.
CMDEF_ADD_LIBRARYinvocation withTYPE SHAREDand explicitVERSIONis well-formed and provides a valid target for packaging.
22-23: Approve install invocation.
CMDEF_INSTALLfortestlib-sharedwith custom namespace aligns with other test cases.
24-27: Verify error‐capture mechanism for invalid version.This directory invokes
CMDEF_PACKAGEdirectly without theTEST_RUN_AND_CHECK_OUTPUTwrapper used elsewhere. Confirm that your test runner auto-captures the fatal error and checks for"invalid_version_format"correctly. If not, wrap this call inTEST_RUN_AND_CHECK_OUTPUTto assert the expected failure.test/CMDEF_PACKAGE/test_cases/invalid_arguments/CMakeLists.txt (5)
9-17: Approve invalid-arguments test setup.CMake version, project name, and includes (
TEST.cmake,CMDEFConfig.cmake,shared_sources.cmake) are correctly referenced for this test suite.
18-20: Verifymissing_main_targeterror check.Using
"MAIN_TARGET"as the fatal substring may be too broad. Ensure this matches the actual error text emitted byCMDEF_PACKAGEor tighten it to a more specific phrase.
22-24: Verifymissing_versionerror check.The substring
"VERSION"might catch unintended messages. Confirm that the harness asserts the precise missing-version error.
26-28: Approveinvalid_main_targetcheck.The fatal message
"Not a valid CMake target:"aligns with the error from a nonexistent or malformed target.
34-36: Verifyinvalid_version_formaterror check.Ensure
"not in valid format"matches the actual output fromCMUTIL_VERSION_CHECK. Adjust the substring if the macro’s wording differs.
test/CMDEF_PACKAGE/test_cases/invalid_arguments/missing_main_target/CMakeLists.txt
Show resolved
Hide resolved
test/CMDEF_PACKAGE/test_cases/invalid_arguments/invalid_main_target/CMakeLists.txt
Show resolved
Hide resolved
test/CMDEF_PACKAGE/test_cases/invalid_arguments/missing_version/CMakeLists.txt
Show resolved
Hide resolved
test/CMDEF_PACKAGE/test_cases/invalid_arguments/main_target_not_cmake_target/CMakeLists.txt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/namespace_mismatch/CMakeLists.txt (1)
7-8: Add FATAL_ERROR to minimum CMake version requirementEnhance robustness by enforcing the version check:
cmake_minimum_required(VERSION 3.18 FATAL_ERROR)This ensures the configuration halts early if an unsupported CMake is used.
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/non_installed_dependency/CMakeLists.txt (1)
7-11: Enforce version and verify includesUse
cmake_minimum_required(VERSION 3.18 FATAL_ERROR)to enforce the minimum CMake version. Also confirm thatTEST.cmake,CMDEFConfig.cmake, and related modules are correctly referenced given the relative paths.test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/multiple_errors/CMakeLists.txt (1)
7-11: Standardize version enforcement and module includesAdopt
cmake_minimum_required(VERSION 3.18 FATAL_ERROR)and verify that the listed include paths for helper scripts are correct for this test’s directory depth.test/CMDEF_PACKAGE/test_cases/dependency_inclusion/transitive_dependencies/CMakeLists.txt (3)
26-83: Add explicit warning checks for transitive dependenciesCurrently the test only verifies absence of
transdep2-sharedin the config file. It should also assert that a warning about omitting a transitive dependency is emitted:# After CMDEF_PACKAGE(...) TEST_PACKAGE_WARNING_MATCH("Omitting transitive dependency 'transdep2-shared'")
93-163: Validate warning logging in deep dependency chainFor
TEST_DEEP_DEPENDENCY_CHAIN, include checks that warnings for omittingdeepdep2-sharedanddeepdep3-sharedappear, usingTEST_PACKAGE_WARNING_MATCH. This ensures the user is informed of each omitted transitive layer.
173-247: Ensure sorted order of direct dependenciesThe test asserts alphabetical ordering in
__INCLUDE_TARGETS_CONFIGS. Consider parsing the list programmatically or adding a dedicated macro to validate sort order rather than hard-coding the string.test/CMDEF_PACKAGE/test_cases/dependency_inclusion/basic_dependency_inclusion/CMakeLists.txt (1)
95-160: Inspect multiple dependencies with propertiesThe test validates multiple deps and include directories. Optionally, verify that
INSTALL_INCLUDE_DIRECTORIESvalues appear in the generated config using aTEST_PACKAGE_CONFIG_FILE_CONTAINScheck for include path entries.test/CMDEF_PACKAGE/test_cases/dependency_inclusion/dependency_exclusion/CMakeLists.txt (5)
25-25: Update placeholder comment
Replace the generic# <function>()marker with the actual function name (TEST_NON_CMDEF_EXCLUSION()) for clarity.
73-90: Verify manual exclusion with STRING(FIND)
The logic correctly asserts that non-CMDEF targets are absent. Consider simplifying by using a hypotheticalTEST_PACKAGE_CONFIG_FILE_NOT_CONTAINSmacro for negative checks if available.
100-100: Update placeholder comment
Replace the# <function>()stub withTEST_IMPORTED_TARGET_EXCLUSION()to match the defined function.
148-158: Manual exclusion check for IMPORTED targets
TheSTRING(FIND)assertions correctly ensure imported libraries are omitted. Again, if there’s a built-in negative assertion macro, it could reduce boilerplate.
169-169: Update placeholder comment
Replace# <function>()withTEST_MIXED_EXCLUSION_SCENARIOS()for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/basic_dependency_inclusion/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/dependency_exclusion/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/multiple_errors/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/namespace_mismatch/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/non_installed_dependency/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/transitive_dependencies/CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/CMakeLists.txt
🔇 Additional comments (21)
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/namespace_mismatch/CMakeLists.txt (1)
12-17: Verify relative include pathsThe relative paths to
TEST.cmake,CMDEFConfig.cmake, and other helper modules may not align with this file’s nesting level. Please confirm these include paths resolve correctly at runtime.test/CMDEF_PACKAGE/test_cases/dependency_inclusion/transitive_dependencies/CMakeLists.txt (1)
6-17: Verify include path depths and helper script availabilityThe include paths to
TEST.cmake,CMDEFConfig.cmake,install_override.cmake, and others vary in depth; please confirm they resolve correctly from this directory.test/CMDEF_PACKAGE/test_cases/dependency_inclusion/basic_dependency_inclusion/CMakeLists.txt (3)
6-13: Verify include order and relative pathsConfirm that
CMDEFConfig.cmakeandTEST.cmakeappear in the correct order and that their relative paths are accurate to this file’s location. Misordering or mispathing can lead to missing definitions.
27-85: Approve single dependency inclusion test
TEST_SINGLE_DEPENDENCY_INCLUSIONthoroughly covers install destinations, config contents, namespace checks, and link validation. The implementation looks correct and comprehensive.
169-219: Approve mixed target types test
TEST_MIXED_TARGET_TYPEScorrectly verifies the inclusion of SHARED, STATIC, and INTERFACE libraries. No further changes needed unless you want to check specific INTERFACE property exports.test/CMDEF_PACKAGE/test_cases/dependency_inclusion/dependency_exclusion/CMakeLists.txt (16)
33-38: Test setup: CMDEF dependency creation is correct
TheCMDEF_ADD_LIBRARYinvocation forcmdefdepcorrectly defines the shared dependency.
49-50: Exclude standard CMake targets creation
TheADD_LIBRARY(standard_shared|standard_static)calls properly create non-CMDEF targets for exclusion testing.
53-59: Conditional linking of Threads library
UsingFIND_PACKAGE(Threads QUIET)and guarding the link is a good pattern for optional system libraries.
62-71: Install and package only CMDEF targets
The calls toCMDEF_INSTALLandCMDEF_PACKAGEtarget only the CMDEF libraries as intended.
108-113: Test setup: CMDEF dependency creation (imported test)
Correctly definescmdefdep2as a shared dependency.
124-131: Setup imported targets and linking
ADD_LIBRARY(... IMPORTED)and subsequentTARGET_LINK_LIBRARIESlines properly introduce and link the imported libraries.
133-142: Install and package CMDEF targets for imported test
Only the CMDEF targets are installed and packaged, matching the test’s goal.
145-145: Verify inclusion of only CMDEF targets
TheTEST_PACKAGE_CONFIG_FILE_CONTAINScall correctly checks for the expected CMDEF targets.
177-189: Test setup: mixed CMDEF dependency creation
Bothincluded1(shared) andincluded2(static) CMDEF targets are defined correctly for the mixed scenario.
191-194: Setup mixed excluded targets
ADD_LIBRARY(excluded_standard)andADD_LIBRARY(excluded_imported IMPORTED)blocks are correct for exclusion testing.
196-202: Link mixed dependencies
All dependencies—both included and excluded—are linked tomixedmain-sharedas intended.
204-213: Install and package mixed scenario
TheCMDEF_INSTALLandCMDEF_PACKAGEcalls correctly set up the mixed scenario for packaging.
216-216: Verify alphabetical ordering in include list
The test expects targets sorted alphabetically; confirm that the packager consistently enforces this ordering.
220-228: Manual exclusion assertions for mixed scenario
The exclusion checks accurately detect unwanted targets in the config.
235-237: Invoke all exclusion tests
Sequential calls to each test function correctly execute the three scenarios.
6-17:✅ Verification successful
Verify include paths for test infrastructure
Ensure the relative paths correctly resolve to the intended CMake scripts (especiallyCMDEFConfig.cmakeat project root).Run this to confirm the location of
CMDEFConfig.cmakeand adjust the../../../../../if needed:
🏁 Script executed:
#!/bin/bash # Search for CMDEFConfig.cmake in the repo fd CMDEFConfig.cmakeLength of output: 40
Include paths verified
TheCMDEFConfig.cmakefile is located at the repository root, and the five-level../../../../../CMDEFConfig.cmakeinclude fromtest/CMDEF_PACKAGE/test_cases/dependency_inclusion/dependency_exclusion/CMakeLists.txtcorrectly resolves to it. No changes required.
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/namespace_mismatch/CMakeLists.txt
Show resolved
Hide resolved
...EF_PACKAGE/test_cases/dependency_inclusion/test_fail/non_installed_dependency/CMakeLists.txt
Show resolved
Hide resolved
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/multiple_errors/CMakeLists.txt
Show resolved
Hide resolved
koudis
force-pushed
the
jan_kubalek/remove_lsbrelease_again
branch
from
66e29d4 to
7354f36
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
README.md (1)
48-48: Fix macro name typo.
CMDEF_COMPIE_DEFINITIONSis misspelled. It should beCMDEF_COMPILE_DEFINITIONS.
🧹 Nitpick comments (12)
test/CMDEF_ENV/README.md (1)
19-22: Clarify test invocation steps.Currently the usage only runs
cmake ., but does not build or execute the tests. Consider adding commands to configure, build, and run (e.g.,cmake --build . && ctest -V) for a complete workflow.README.md (6)
17-18: Fix grammar in project description.Rephrase to:
It's a set of wrappers and helpers which enable easy use of existing CMake features in a more standardized manner across multiple projects.
25-27: Capitalize and add article for "Chrome".Change to:
For example – the CMake project for building the Chrome browser.
29-29: Insert missing comma in sentence.Add a comma after "By that target":
By that target, other project properties are referenced – installer name, application name, documentation, etc.
32-32: Correct typo and clarify the library target phrase.Replace with:
The library 'main target' is represented by the LIBRARY_GROUP argument with a suffix added based on the library type.
36-38: Replace hard tab with spaces in code block.In markdown fences, convert the leading tab before
FIND_PACKAGEto spaces to satisfy lint rules and ensure consistent rendering.
42-42: Streamline wording for build flags.Consider:
CMake-lib sets and maintains build and link flags as well as global definitions.test/CMDEF_INSTALL/README.md (2)
5-5: Wrap CMake commands and macros in backticks.Treat
INSTALLand other CMake terms as code:
The tests use an \INSTALL` command override mechanism to intercept…`
41-45: Include example to run all tests.Consider adding a batch invocation for all cases, for example:
cmake -S . -B build cmake --build build ctest --test-dir build --output-on-failureThis provides a complete end-to-end run.
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/basic_dependency_inclusion/CMakeLists.txt (3)
6-18: Reduce deep relative include path complexity.The multiple
INCLUDE("${CMAKE_CURRENT_LIST_DIR}/../../../../../...")calls are brittle and error-prone. Consider settingCMAKE_MODULE_PATHor defining a single root variable (e.g.set(TEST_ROOT "${CMAKE_CURRENT_LIST_DIR}/../../../..")) to centralize and simplify these paths.
6-7: Normalize CMake command casing.While CMake is case-insensitive, using the conventional lowercase style (
cmake_minimum_required,project) improves readability and consistency with most CMake codebases.
25-27: Remove placeholder comment.The
# <function>()line appears to be a leftover placeholder—please update or remove it to reflect the actual function name (TEST_SINGLE_DEPENDENCY_INCLUSION).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (107)
CMDEFConfig.cmake(1 hunks)README.md(2 hunks)system_modules/CMDEF_ADD_LIBRARY.cmake(1 hunks)system_modules/CMDEF_ENV.cmake(3 hunks)system_modules/CMDEF_PACKAGE.cmake(1 hunks)test/CMDEF_ADD_EXECUTABLE/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/README.md(1 hunks)test/CMDEF_ADD_EXECUTABLE/error_test_sources/invalid_syntax.cpp(1 hunks)test/CMDEF_ADD_EXECUTABLE/error_test_sources/missing_include.cpp(1 hunks)test/CMDEF_ADD_EXECUTABLE/shared_sources/include/platform_utils.h(1 hunks)test/CMDEF_ADD_EXECUTABLE/shared_sources/include/utils.h(1 hunks)test/CMDEF_ADD_EXECUTABLE/shared_sources/shared_sources.cmake(1 hunks)test/CMDEF_ADD_EXECUTABLE/shared_sources/src/main.cpp(1 hunks)test/CMDEF_ADD_EXECUTABLE/shared_sources/src/platform_utils.cpp(1 hunks)test/CMDEF_ADD_EXECUTABLE/shared_sources/src/utils.cpp(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/all_parameters/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/check_function/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/custom_output_name/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/debug_suffix_build_type/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/executable_with_includes/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/generator_expression_suffix/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/invalid_target_name_warning/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/invalid_version_error/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/macos_bundle/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/minimal_executable/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/missing_sources_error/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/missing_version_error/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/multiple_sources/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/win32_flag/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/windows_msvc_runtime/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/windows_resource_generation/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/README.md(1 hunks)test/CMDEF_ADD_LIBRARY/multiple_include_sources/include/library.h(1 hunks)test/CMDEF_ADD_LIBRARY/multiple_include_sources/include1/header1.h(1 hunks)test/CMDEF_ADD_LIBRARY/multiple_include_sources/include2/header2.h(1 hunks)test/CMDEF_ADD_LIBRARY/multiple_include_sources/multiple_include_sources.cmake(1 hunks)test/CMDEF_ADD_LIBRARY/multiple_include_sources/src/library.cpp(1 hunks)test/CMDEF_ADD_LIBRARY/multiple_include_sources/src/utils.cpp(1 hunks)test/CMDEF_ADD_LIBRARY/shared_sources/include/library.h(1 hunks)test/CMDEF_ADD_LIBRARY/shared_sources/include/utils.h(1 hunks)test/CMDEF_ADD_LIBRARY/shared_sources/shared_sources.cmake(1 hunks)test/CMDEF_ADD_LIBRARY/shared_sources/src/library.cpp(1 hunks)test/CMDEF_ADD_LIBRARY/shared_sources/src/utils.cpp(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/check_function/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/generator_expressions/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/interface_library/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/multiple_include_directories/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/invalid_type/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/missing_library_group/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/missing_sources_non_interface/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/missing_version/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/source_base_directory_non_interface/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/soversion_greater_than_version/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/posix_specific/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/soversion/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/target_properties_validation/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/version_validation/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/windows_specific/CMakeLists.txt(1 hunks)test/CMDEF_ENV/CMakeLists.txt(1 hunks)test/CMDEF_ENV/README.md(1 hunks)test/CMDEF_ENV/arch_detection/CMakeLists.txt(1 hunks)test/CMDEF_ENV/basic_init/CMakeLists.txt(1 hunks)test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt(1 hunks)test/CMDEF_ENV/macos_specific/CMakeLists.txt(1 hunks)test/CMDEF_ENV/os_detection/CMakeLists.txt(1 hunks)test/CMDEF_ENV/windows_specific/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/README.md(1 hunks)test/CMDEF_INSTALL/install_test_macros.cmake(1 hunks)test/CMDEF_INSTALL/library_tests.cmake(1 hunks)test/CMDEF_INSTALL/shared_sources/include/library.h(1 hunks)test/CMDEF_INSTALL/shared_sources/shared_sources.cmake(1 hunks)test/CMDEF_INSTALL/shared_sources/src/library.cpp(1 hunks)test/CMDEF_INSTALL/shared_sources/src/main.cpp(1 hunks)test/CMDEF_INSTALL/test_cases/executable/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/include_dirs/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/interface_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/multiple_calls/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/no_config/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/shared_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/static_lib/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/with_configs/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/test_cases/with_namespace/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/README.md(1 hunks)test/CMDEF_PACKAGE/package_test_macros.cmake(1 hunks)test/CMDEF_PACKAGE/shared_sources/custom_cpack_config.cmake(1 hunks)test/CMDEF_PACKAGE/shared_sources/include/library.h(1 hunks)test/CMDEF_PACKAGE/shared_sources/shared_sources.cmake(1 hunks)test/CMDEF_PACKAGE/shared_sources/src/library.cpp(1 hunks)test/CMDEF_PACKAGE/shared_sources/src/main.cpp(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/basic_dependency_inclusion/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/dependency_exclusion/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/multiple_errors/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/namespace_mismatch/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/non_installed_dependency/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/transitive_dependencies/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/executable/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/interface_lib/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/invalid_arguments/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/invalid_arguments/invalid_main_target/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/invalid_arguments/invalid_version_format/CMakeLists.txt(1 hunks)
⛔ Files not processed due to max files limit (13)
- test/CMDEF_PACKAGE/test_cases/invalid_arguments/main_target_not_cmake_target/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/invalid_arguments/missing_main_target/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/invalid_arguments/missing_version/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/multiconfig_build/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/shared_lib/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/static_lib/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/with_configs/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/with_cpack_config/CMakeLists.txt
- test/CMakeLists.txt
- test/README.md
- test/TEST.cmake
- test/cache_var.cmake
- test/install_override.cmake
✅ Files skipped from review due to trivial changes (15)
- CMDEFConfig.cmake
- test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/shared_sources/src/library.cpp
- test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/source_base_directory_non_interface/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/error_test_sources/missing_include.cpp
- test/CMDEF_PACKAGE/shared_sources/src/library.cpp
- test/CMDEF_ADD_EXECUTABLE/test_cases/invalid_target_name_warning/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/minimal_executable/CMakeLists.txt
- test/CMDEF_PACKAGE/shared_sources/include/library.h
- test/CMDEF_INSTALL/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/missing_sources_error/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/shared_sources/shared_sources.cmake
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/CMakeLists.txt
- test/CMDEF_PACKAGE/package_test_macros.cmake
- test/CMDEF_ADD_LIBRARY/test_cases/check_function/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (88)
- test/CMDEF_ADD_EXECUTABLE/shared_sources/include/utils.h
- test/CMDEF_PACKAGE/shared_sources/custom_cpack_config.cmake
- test/CMDEF_INSTALL/shared_sources/src/main.cpp
- test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/invalid_type/CMakeLists.txt
- test/CMDEF_PACKAGE/shared_sources/src/main.cpp
- test/CMDEF_ADD_LIBRARY/multiple_include_sources/src/library.cpp
- test/CMDEF_INSTALL/shared_sources/include/library.h
- test/CMDEF_ADD_LIBRARY/multiple_include_sources/include/library.h
- system_modules/CMDEF_PACKAGE.cmake
- test/CMDEF_INSTALL/shared_sources/shared_sources.cmake
- test/CMDEF_ADD_EXECUTABLE/shared_sources/src/utils.cpp
- test/CMDEF_ADD_EXECUTABLE/shared_sources/include/platform_utils.h
- test/CMDEF_ADD_LIBRARY/multiple_include_sources/include2/header2.h
- test/CMDEF_ADD_LIBRARY/shared_sources/include/library.h
- test/CMDEF_ADD_LIBRARY/multiple_include_sources/multiple_include_sources.cmake
- test/CMDEF_ADD_LIBRARY/shared_sources/src/utils.cpp
- test/CMDEF_ADD_LIBRARY/multiple_include_sources/include1/header1.h
- test/CMDEF_INSTALL/shared_sources/src/library.cpp
- test/CMDEF_ADD_EXECUTABLE/error_test_sources/invalid_syntax.cpp
- test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/soversion_greater_than_version/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/shared_sources/include/utils.h
- test/CMDEF_PACKAGE/README.md
- test/CMDEF_ADD_EXECUTABLE/test_cases/debug_suffix_build_type/CMakeLists.txt
- test/CMDEF_ENV/macos_specific/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/win32_flag/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/missing_library_group/CMakeLists.txt
- test/CMDEF_INSTALL/library_tests.cmake
- test/CMDEF_ADD_EXECUTABLE/test_cases/generator_expression_suffix/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/multiple_errors/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/invalid_version_error/CMakeLists.txt
- test/CMDEF_PACKAGE/shared_sources/shared_sources.cmake
- test/CMDEF_PACKAGE/test_cases/invalid_arguments/invalid_main_target/CMakeLists.txt
- system_modules/CMDEF_ADD_LIBRARY.cmake
- test/CMDEF_ADD_EXECUTABLE/shared_sources/src/platform_utils.cpp
- test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/missing_version/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/custom_output_name/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/invalid_arguments/invalid_version_format/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/executable_with_includes/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/missing_version_error/CMakeLists.txt
- test/CMDEF_PACKAGE/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/windows_resource_generation/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/missing_sources_non_interface/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/invalid_arguments/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/shared_sources/src/main.cpp
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/non_installed_dependency/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/shared_sources/shared_sources.cmake
- test/CMDEF_ADD_EXECUTABLE/README.md
- test/CMDEF_ENV/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/all_parameters/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/CMakeLists.txt
- test/CMDEF_ENV/arch_detection/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/test_cases/parameter_validation/CMakeLists.txt
- test/CMDEF_ENV/windows_specific/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/multiple_sources/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/check_function/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/README.md
- test/CMDEF_ADD_LIBRARY/test_cases/posix_specific/CMakeLists.txt
- test/CMDEF_ENV/os_detection/CMakeLists.txt
- test/CMDEF_ENV/basic_init/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/windows_msvc_runtime/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/executable/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/test_cases/generator_expressions/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/test_cases/soversion/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/multiple_calls/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/macos_bundle/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/multiple_include_sources/src/utils.cpp
- test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/test_cases/version_validation/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/interface_lib/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/test_cases/multiple_include_directories/CMakeLists.txt
- system_modules/CMDEF_ENV.cmake
- test/CMDEF_ADD_LIBRARY/test_cases/interface_library/CMakeLists.txt
- test/CMDEF_INSTALL/install_test_macros.cmake
- test/CMDEF_ADD_LIBRARY/test_cases/target_properties_validation/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/transitive_dependencies/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/include_dirs/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/no_config/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/namespace_mismatch/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/with_configs/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/test_cases/windows_specific/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/dependency_exclusion/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/static_lib/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/with_namespace/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/executable/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/shared_lib/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/interface_lib/CMakeLists.txt
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~18-~18: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...ore standardised manner akross multiple project. ### Definition of Main target Each C...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[grammar] ~25-~25: The proper noun “Chrome” (= software from Google) needs to be capitalized.
Context: ...or example - CMake project for building chrome browser. Browser is represented by the ...
(GOOGLE_PRODUCTS)
[uncategorized] ~25-~25: A determiner appears to be missing. Consider inserting it.
Context: ...ke project for building chrome browser. Browser is represented by the executable called...
(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~29-~29: A comma might be missing here.
Context: ...e own target in CMake project. By that target other project properties are referenced...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~30-~30: You might be missing the article “the” here.
Context: ...tion etc. This type of target is called 'main target' (the 'object' for which the...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
test/CMDEF_ENV/README.md
[uncategorized] ~15-~15: Possible missing preposition found.
Context: ...errides - Cross-compilation scenarios - Non-standard architectures ## Usage Run a...
(AI_HYDRA_LEO_MISSING_IN)
test/CMDEF_INSTALL/README.md
[grammar] ~5-~5: The word ‘INSTALL’ is not a noun.
Context: ...sts ## Test Strategy The tests use an INSTALL command override mechanism to intercept...
(A_INSTALL)
🪛 markdownlint-cli2 (0.17.2)
README.md
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (5)
test/CMDEF_ENV/README.md (1)
6-7: Verify the architecture identifier "aplsil".The list includes "aplsil", which may be a typo or an unconventional architecture label. Please confirm or correct this term to match the intended target (e.g., "appleSilicon" or "aarch64").
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/basic_dependency_inclusion/CMakeLists.txt (4)
27-85: Comprehensive single dependency inclusion test is well-structured.
TEST_SINGLE_DEPENDENCY_INCLUSIONcleanly sets up a shared library dependency, installs, packages, and validates each step—including install status, namespace consistency, and link libraries. All assertions look correct.
95-160: Multiple dependencies test properly validates properties.
TEST_MULTIPLE_DEPENDENCIES_WITH_PROPERTIESaccurately covers shared and static dependencies with include-directory properties, verifying installation flags, namespace propagation, and linkage. No issues found.
170-218: Mixed target types inclusion test is solid.
TEST_MIXED_TARGET_TYPEScorrectly handles SHARED, STATIC, and INTERFACE dependencies and verifies their inclusion in the package config. Good coverage and checks.
221-224: Standard invocation of test functions at the end — no changes needed.
Remove explicit need of uname and lsb_release file
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
doc/CacheVariables.md (3)
34-34: Use consistent descriptive phrasing
The entry currently uses an imperative tone (“Indicate if…”), whereas other descriptions are noun- or clause-based. Consider rephrasing for consistency and clarity.Proposed diff:
- Indicate if the CMAKE_BUILD_TYPE is overridden by CMDEF library. + Whether the CMAKE_BUILD_TYPE is overridden by the CMDEF library.
37-37: Insert missing comma for readability
Add a comma after the introductory phrase to improve flow.Proposed diff:
- to the predefined value stored in CMDEF_BUILD_TYPE_DEFAULT variable. In this case the variable is set to ON. + to the predefined value stored in CMDEF_BUILD_TYPE_DEFAULT variable. In this case, the variable is set to ON.
46-46: Fix typo in “build”
Correct “byuld” to “build” to eliminate the typo.Proposed diff:
- List of the supported byuld types. CMAKE_BUILD_TYPE must hold exactly one value from this list. + List of the supported build types. CMAKE_BUILD_TYPE must hold exactly one value from this list.example/executable/CMakeLists.txt (2)
17-17: GuardCMDEF_CLEANUP()to avoid calling on missing package.Wrap the cleanup call in a check for
CMLIB_FOUNDto prevent errors whenfind_packageis optional:- CMDEF_CLEANUP() + if(CMLIB_FOUND) + CMDEF_CLEANUP() + endif()
40-40: Clarify compiler-specific flag.The
/wd4996switch only applies to MSVC. Update the comment for accuracy:- ALL /wd4996 # Disable deprecation warning on Windows OS + ALL /wd4996 # Disable deprecation warning on MSVC (Windows)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
FindCMDEF.cmake(2 hunks)doc/CacheVariables.md(2 hunks)example/executable/CMakeLists.txt(5 hunks)example/library-interface/CMakeLists.txt(1 hunks)example/library-interface/main.cpp(1 hunks)example/library/CMakeLists.txt(1 hunks)system_modules/CMDEF_BUILD_TYPE.cmake(2 hunks)system_modules/CMDEF_COMPILE_DEFINITION.cmake(4 hunks)system_modules/CMDEF_COMPILE_OPTION.cmake(4 hunks)system_modules/CMDEF_ENV.cmake(6 hunks)system_modules/CMDEF_LINK_OPTION.cmake(4 hunks)system_modules/CMDEF_RESOURCE.cmake(2 hunks)
✅ Files skipped from review due to trivial changes (6)
- example/library-interface/main.cpp
- example/library/CMakeLists.txt
- example/library-interface/CMakeLists.txt
- system_modules/CMDEF_RESOURCE.cmake
- system_modules/CMDEF_BUILD_TYPE.cmake
- system_modules/CMDEF_COMPILE_OPTION.cmake
🚧 Files skipped from review as they are similar to previous changes (2)
- FindCMDEF.cmake
- system_modules/CMDEF_ENV.cmake
🧰 Additional context used
🪛 LanguageTool
doc/CacheVariables.md
[uncategorized] ~36-~36: Possible missing comma found.
Context: ...rary. - If the CMAKE_BUILD_TYPE is not set the CMDEF try to set the CMAKE_BUILD_TY...
(AI_HYDRA_LEO_MISSING_COMMA)
[formatting] ~37-~37: Consider inserting a comma after an introductory phrase for better readability.
Context: ...d in CMDEF_BUILD_TYPE_DEFAULT variable. In this case the variable is set to ON. - If the CMA...
(IN_THAT_CASE_COMMA)
🔇 Additional comments (5)
system_modules/CMDEF_COMPILE_DEFINITION.cmake (2)
24-26: Typo fix looks good.
The “language” spelling correction in the comment improves clarity without changing behaviour.
80-82: Another clear typo fix – good catch.
Consistent wording in the docs is always welcome.system_modules/CMDEF_LINK_OPTION.cmake (1)
24-25: Typo corrections acknowledged.
“language” wording is now consistent – nice.Also applies to: 81-82
example/executable/CMakeLists.txt (2)
57-57: CorrectedVISIBILITY PUBLICkeyword.The typo fix ensures the macro now recognizes the visibility scope. Implementation looks good.
70-70: CorrectedVISIBILITY PRIVATEkeyword.The typo fix aligns the target’s compile definitions with the intended private scope. LGTM.
…ain' into jan_kubalek/remove_lsbrelease_again
koudis
force-pushed
the
jan_kubalek/remove_lsbrelease_again
branch
from
b45fa86 to
3a7563a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
README.md (8)
18-18: Fix spelling and pluralization
Replace “akross” with “across” and change “project” to “projects” for correct spelling and plurality.
25-27: Improve example phrasing and capitalization
Consider rewriting to:For example, a CMake project for building the Chrome browser.
Ensure “Chrome” is capitalized and add the comma and article for clarity.
29-29: Add missing comma or rephrase
Insert a comma after “By that target,” or rephrase the sentence to improve readability.
31-31: Correct typo “wift”
Replace “wift” with “with” and consider rephrasing for clarity, e.g.:…represented by the LIBRARY_GROUP argument with a suffix based on the library type.
36-38: Normalize code block formatting
Remove the leading tab in the code fence to maintain consistent formatting:FIND_PACKAGE(CMLIB COMPONENTS CMDEF)
42-42: Refine phrase “global wide definitions”
Consider simplifying to “global definitions” or “globally scoped definitions” for conciseness.
50-50: Capitalize and refine directory reference
Change to:Examples can be found in the [example] directory.
Capitalize “Examples” and use “in” for proper preposition.
59-59: Use “in” instead of “at” for module reference
Replace “found at the appropriate module” with “found in the appropriate module” for accuracy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(4 hunks)doc/PackageChain.md(2 hunks)doc/README.md(1 hunks)system_modules/CMDEF_LINK_OPTION.cmake(4 hunks)test/CMDEF_PACKAGE/test_cases/executable/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/interface_lib/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/shared_lib/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/static_lib/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/with_cpack_config/CMakeLists.txt(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- doc/README.md
- doc/PackageChain.md
🚧 Files skipped from review as they are similar to previous changes (6)
- system_modules/CMDEF_LINK_OPTION.cmake
- test/CMDEF_PACKAGE/test_cases/shared_lib/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/with_cpack_config/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/interface_lib/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/executable/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/static_lib/CMakeLists.txt
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~25-~25: The proper noun “Chrome” (= software from Google) needs to be capitalized.
Context: ...or example - CMake project for building chrome browser. Browser is represented by the ...
(GOOGLE_PRODUCTS)
[uncategorized] ~29-~29: A comma might be missing here.
Context: ...e own target in CMake project. By that target other project properties are referenced...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🔇 Additional comments (1)
README.md (1)
2-4: Update component name and alias
The updated title and alias accurately reflect the new branding for the CMake-lib Definition Framework component.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/tests.yml (1)
59-60:actions/checkout@v2is outdatedYou’re still on v2 for the Windows leg while the other jobs use v3.
Align to v3 unless you have a hard requirement for the old Node-12 runner.
🧹 Nitpick comments (3)
.github/workflows/tests.yml (3)
16-25: Indentation inside thematrix.imageblock is offYAML-lint warns (
expected 8 but found 10). While it still parses, the uneven indent hurts readability and can trip auto-formatters.- image: [ - "test_debian:bullseye", - "test_debian:trixie", - "test_debian:bookworm", - "test_ubuntu:2004", - "test_ubuntu:2204", - "test_ubuntu:2404", - "test_fedora:41", - "test_fedora:42" - ] + image: [ + "test_debian:bullseye", + "test_debian:trixie", + "test_debian:bookworm", + "test_ubuntu:2004", + "test_ubuntu:2204", + "test_ubuntu:2404", + "test_fedora:41", + "test_fedora:42" + ]
42-44: Job name vs runner mismatchThe job is named
test_macos_11but runs onmacos-latest(currently macOS-14).
If you really need Big Sur, setruns-on: macos-11; otherwise rename the job to avoid confusion in CI dashboards.
10-11: Trailing whitespace and missing terminating newlineYAML-lint flags:
- line 11 & 41 – trailing spaces
- EOF – no newline
Tiny but noisy in diffs; trim & add a final newline.
- workflow_dispatch: - + workflow_dispatch: ... - cd test/ && cmake . - + cd test/ && cmake . ... - cd test/ && cmake . \ No newline at end of file + + cd test/ && cmake . +Also applies to: 40-41, 68-68
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/tests.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMakeLists.txt:9-11
Timestamp: 2025-06-17T22:15:13.853Z
Learning: The cmakelib-component-cmdef project uses a custom testing framework with TEST.cmake and custom macros like TEST_RUN, TEST_RUN_AND_CHECK_OUTPUT rather than CTest. CTest integration is not needed or used in this project.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_INSTALL/test_cases/install_related/include_dirs/CMakeLists.txt:0-0
Timestamp: 2025-06-16T19:47:01.662Z
Learning: In the CMDEF_INSTALL system, export destination paths intentionally contain double slashes like `${CMDEF_LIBRARY_INSTALL_DIR}/cmake//` - this is the actual format produced by the implementation, not a formatting error in tests.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:14-17
Timestamp: 2025-06-08T19:13:14.984Z
Learning: For CMake test cases that verify error conditions (like missing required parameters), use TEST_RUN_AND_CHECK_OUTPUT or similar test harness macros instead of directly calling the function being tested. Direct calls will abort CMake without proper test assertion, while test harnesses can capture and validate expected error messages.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:11-12
Timestamp: 2025-06-16T19:45:39.391Z
Learning: In CMDEF test suites, individual error test cases (like missing_target_error) should NOT include TEST.cmake. They are designed to be executed by main test orchestrators that use TEST_RUN_AND_CHECK_OUTPUT to capture and validate the expected fatal errors. Individual test cases should simply trigger the error condition.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt:215-218
Timestamp: 2025-06-17T21:44:57.056Z
Learning: In CMDEF_ENV cache variable validation tests, hardcoded platform-specific values (like Windows architecture "x86-64" and version "1") are intentional and used to validate expected cache variable values in the test environment, rather than testing dynamic detection capabilities which are covered by other test cases.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/install_override.cmake:29-52
Timestamp: 2025-06-16T19:50:42.083Z
Learning: When overriding built-in CMake commands (like INSTALL) for testing purposes, MACRO() must be used instead of function() to properly shadow the original command. This is a specific case where the macro form is technically required despite general preferences for functions to avoid variable leakage.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_PACKAGE/package_test_macros.cmake:92-96
Timestamp: 2025-06-16T20:53:43.698Z
Learning: The CMDEF_PACKAGE system creates a custom target named "PACKAGE" (uppercase) in multiconfig builds, which should not exist in single-config builds. This is different from CMake's built-in CPack "package" target (lowercase).
.github/workflows/tests.yml (3)
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:11-12
Timestamp: 2025-06-16T19:45:39.391Z
Learning: In CMDEF test suites, individual error test cases (like missing_target_error) should NOT include TEST.cmake. They are designed to be executed by main test orchestrators that use TEST_RUN_AND_CHECK_OUTPUT to capture and validate the expected fatal errors. Individual test cases should simply trigger the error condition.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMakeLists.txt:9-11
Timestamp: 2025-06-17T22:15:13.853Z
Learning: The cmakelib-component-cmdef project uses a custom testing framework with TEST.cmake and custom macros like TEST_RUN, TEST_RUN_AND_CHECK_OUTPUT rather than CTest. CTest integration is not needed or used in this project.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:14-17
Timestamp: 2025-06-08T19:13:14.984Z
Learning: For CMake test cases that verify error conditions (like missing required parameters), use TEST_RUN_AND_CHECK_OUTPUT or similar test harness macros instead of directly calling the function being tested. Direct calls will abort CMake without proper test assertion, while test harnesses can capture and validate expected error messages.
🪛 actionlint (1.7.7)
.github/workflows/tests.yml
31-31: 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)
45-45: 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)
59-59: the runner of "actions/checkout@v2" 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/tests.yml
[error] 11-11: trailing spaces
(trailing-spaces)
[warning] 25-25: wrong indentation: expected 8 but found 10
(indentation)
[warning] 29-29: wrong indentation: expected 6 but found 8
(indentation)
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 68-68: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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
- GitHub Check: test_macos_11
koudis
force-pushed
the
jan_kubalek/remove_lsbrelease_again
branch
from
3e9820c to
adc73c3
Compare
koudis
force-pushed
the
jan_kubalek/remove_lsbrelease_again
branch
from
adc73c3 to
51accc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (11)
system_modules/CMDEF_ENV.cmake (2)
125-125: Syntax fix already addressed.This MESSAGE command syntax fix was already flagged in previous reviews and has been properly corrected.
384-384: Syntax fix already addressed.This MESSAGE command syntax fix was already flagged in previous reviews and has been properly corrected.
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/namespace_mismatch/CMakeLists.txt (2)
4-6: Guard fail-only test execution behindCMDEF_TEST_FAIL_CASESas previously discussedThe script states that it “should only be run when
CMDEF_TEST_FAIL_CASESis set” yet there is no such guard; the test is invoked unconditionally later.
This is the same concern raised in an earlier review.
Executing the file unconditionally will break regular (non-fail) test runs.
12-17: IncludingTEST.cmakecontradicts the established fail-case patternPer project guidance, individual fail test cases should not include
TEST.cmake; orchestration macros capture the fatal error instead.
Keeping this include may trigger unintended helpers or assertions before the deliberate failure occurs.README.md (4)
4-4: Use correct Apple OS branding (repeat).
Mac OS→macOS
This was called out in a previous review but is still unresolved.
38-40: Remove hard-tab and align code fence (repeat).Line 39 starts with a hard tab, triggering MD010. Replace the tab with spaces so the code fence and its contents are left-aligned.
- FIND_PACKAGE(CMLIB COMPONENTS CMDEF) +FIND_PACKAGE(CMLIB COMPONENTS CMDEF)
27-29: Capitalize “Chrome” and improve phrasing (repeat).-For example - CMake project for building chrome browser. -Browser is represented by the executable called `chrome`. +For example, a CMake project for building the Chrome browser. +The browser is represented by the executable `chrome`.
52-52: Grammar: capitalise and use correct preposition (repeat).
examples→Examples,at→in-examples can be found at [example] directory. +Examples can be found in the [example] directory.test/TEST.cmake (3)
59-62: Fix inconsistent error message in TEST_VAR_NOT_EQUAL macro.The error message incorrectly states "is not equal to" when it should state "is equal to" for the NOT_EQUAL test case.
- IF(("${${var_a}}" STREQUAL "${${var_b}}")) - MESSAGE(FATAL_ERROR "Variable ${var_a}(${${var_a}}) is not equal to ${var_b}(${${var_b}})") - ENDIF() + IF(("${${var_a}}" STREQUAL "${${var_b}}")) + MESSAGE(FATAL_ERROR "Variable ${var_a}(${${var_a}}) is equal to ${var_b}(${${var_b}})") + ENDIF()
133-176: Enhance TEST_RUN_AND_CHECK_OUTPUT function robustness.The function has several areas for improvement in error handling and logic clarity.
157-175: Unchecked execution result can mask failuresIf neither
WARNING_MESSAGEnorFATAL_ERROR_MESSAGEis supplied the function never inspectsresult, potentially letting a failing test pass silently. Add a default "success expected" branch.
🧹 Nitpick comments (10)
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/namespace_mismatch/CMakeLists.txt (1)
32-33:CACHE_VAR_FORCE_SETis not balanced when the expected fatal error aborts early
CACHE_VAR_RESTORE()is never reached ifCMDEF_PACKAGEfatals, leaving the globalCMAKE_BUILD_TYPEcache mutated for subsequent tests.Consider pushing the current value and restoring it unconditionally via
cmake_language(PUSH/POP)or acmake_language(CALL)wrapper that executes even on fatal error.Also applies to: 69-69
.github/workflows/tests.yml (3)
62-62: Update checkout action to v3 for consistencyThe Windows job uses an outdated
actions/checkout@v2while the Linux and macOS jobs use v3.- - uses: actions/checkout@v2 + - uses: actions/checkout@v3
14-14: Fix YAML formatting issuesThe workflow file has several formatting issues that should be addressed:
- Trailing spaces on lines 14 and 44
- Missing newline at end of file
Remove trailing spaces after
CMLIB_VERSION: 1.0.0and after line 43, and add a newline at the end of the file.Also applies to: 44-44, 71-71
28-28: Fix YAML indentationThe matrix configuration has incorrect indentation.
- ] + ]And for the container configuration:
- image: ghcr.io/cmakelib/${{ matrix.image }} + image: ghcr.io/cmakelib/${{ matrix.image }}Also applies to: 32-32
README.md (5)
75-75: Fix article misuse and tighten wording.-Every function has a comprehensive documentation written as part of the function definition. +Every function includes comprehensive documentation written within its definition.
44-45: Minor wording – “global-wide” → “global”.-CMake-lib sets and maintains build and link flags and global wide definitions. +CMake-lib sets and maintains build and link flags and global definitions.
8-9: Improve grammar in project overview.-CMake-lib provides consistent settings for build environment. -It simplifies and maintains built option, libraries and executables. +CMake-lib provides consistent settings for the build environment. +It simplifies and maintains build options, libraries, and executables.
34-34: Clarify sentence on “main target” representation.-The library 'main target' is represented by LIBRARY_GROUP argument with suffix added according to a library type created. +The library’s main target is represented by the `LIBRARY_GROUP` argument, with a suffix that reflects the library type.
81-82: Style – drop leading article and simplify.-- The uppercase letters are used for all keywords and global variables +- Uppercase letters are used for all keywords and global variablestest/TEST.cmake (1)
10-10: Fix typo: "varialbe" → "variable"This typo appears in multiple comment blocks throughout the file.
-# var - varialbe name of value +# var - variable name of valueAlso applies to: 19-19, 28-28, 37-37, 46-46, 56-56
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.github/workflows/tests.yml(1 hunks)README.md(5 hunks)doc/CacheVariables.md(4 hunks)system_modules/CMDEF_ENV.cmake(10 hunks)system_modules/CMDEF_PACKAGE.cmake(4 hunks)test/CMDEF_ADD_EXECUTABLE/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/shared_sources/shared_sources.cmake(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/win32_flag/CMakeLists.txt(1 hunks)test/CMDEF_ADD_EXECUTABLE/test_cases/windows_resource_generation/CMakeLists.txt(1 hunks)test/CMDEF_ADD_LIBRARY/README.md(1 hunks)test/CMDEF_ADD_LIBRARY/test_cases/windows_specific/CMakeLists.txt(1 hunks)test/CMDEF_ENV/CMakeLists.txt(1 hunks)test/CMDEF_ENV/windows_specific/CMakeLists.txt(1 hunks)test/CMDEF_INSTALL/README.md(1 hunks)test/CMDEF_INSTALL/test_cases/shared_lib/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/README.md(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/basic_dependency_inclusion/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/dependency_exclusion/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/multiple_errors/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/namespace_mismatch/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/non_installed_dependency/CMakeLists.txt(1 hunks)test/CMDEF_PACKAGE/test_cases/dependency_inclusion/transitive_dependency_warning/CMakeLists.txt(1 hunks)test/CMakeLists.txt(1 hunks)test/README.md(1 hunks)test/TEST.cmake(1 hunks)version.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/CMDEF_PACKAGE/README.md
🚧 Files skipped from review as they are similar to previous changes (21)
- version.txt
- test/CMDEF_INSTALL/README.md
- doc/CacheVariables.md
- test/CMDEF_ENV/CMakeLists.txt
- test/CMDEF_ENV/windows_specific/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/shared_sources/shared_sources.cmake
- test/CMDEF_ADD_EXECUTABLE/test_cases/win32_flag/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/README.md
- test/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/test_cases/windows_resource_generation/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/multiple_errors/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/transitive_dependency_warning/CMakeLists.txt
- test/CMDEF_ADD_EXECUTABLE/CMakeLists.txt
- system_modules/CMDEF_PACKAGE.cmake
- test/README.md
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/non_installed_dependency/CMakeLists.txt
- test/CMDEF_ADD_LIBRARY/test_cases/windows_specific/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/basic_dependency_inclusion/CMakeLists.txt
- test/CMDEF_INSTALL/test_cases/shared_lib/CMakeLists.txt
- test/CMDEF_PACKAGE/test_cases/dependency_inclusion/dependency_exclusion/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMakeLists.txt:9-11
Timestamp: 2025-06-17T22:15:13.853Z
Learning: The cmakelib-component-cmdef project uses a custom testing framework with TEST.cmake and custom macros like TEST_RUN, TEST_RUN_AND_CHECK_OUTPUT rather than CTest. CTest integration is not needed or used in this project.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_INSTALL/test_cases/install_related/include_dirs/CMakeLists.txt:0-0
Timestamp: 2025-06-16T19:47:01.662Z
Learning: In the CMDEF_INSTALL system, export destination paths intentionally contain double slashes like `${CMDEF_LIBRARY_INSTALL_DIR}/cmake//` - this is the actual format produced by the implementation, not a formatting error in tests.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:14-17
Timestamp: 2025-06-08T19:13:14.984Z
Learning: For CMake test cases that verify error conditions (like missing required parameters), use TEST_RUN_AND_CHECK_OUTPUT or similar test harness macros instead of directly calling the function being tested. Direct calls will abort CMake without proper test assertion, while test harnesses can capture and validate expected error messages.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:11-12
Timestamp: 2025-06-16T19:45:39.391Z
Learning: In CMDEF test suites, individual error test cases (like missing_target_error) should NOT include TEST.cmake. They are designed to be executed by main test orchestrators that use TEST_RUN_AND_CHECK_OUTPUT to capture and validate the expected fatal errors. Individual test cases should simply trigger the error condition.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt:215-218
Timestamp: 2025-06-17T21:44:57.056Z
Learning: In CMDEF_ENV cache variable validation tests, hardcoded platform-specific values (like Windows architecture "x86-64" and version "1") are intentional and used to validate expected cache variable values in the test environment, rather than testing dynamic detection capabilities which are covered by other test cases.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/install_override.cmake:29-52
Timestamp: 2025-06-16T19:50:42.083Z
Learning: When overriding built-in CMake commands (like INSTALL) for testing purposes, MACRO() must be used instead of function() to properly shadow the original command. This is a specific case where the macro form is technically required despite general preferences for functions to avoid variable leakage.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_PACKAGE/package_test_macros.cmake:92-96
Timestamp: 2025-06-16T20:53:43.698Z
Learning: The CMDEF_PACKAGE system creates a custom target named "PACKAGE" (uppercase) in multiconfig builds, which should not exist in single-config builds. This is different from CMake's built-in CPack "package" target (lowercase).
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/namespace_mismatch/CMakeLists.txt (7)
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:11-12
Timestamp: 2025-06-16T19:45:39.391Z
Learning: In CMDEF test suites, individual error test cases (like missing_target_error) should NOT include TEST.cmake. They are designed to be executed by main test orchestrators that use TEST_RUN_AND_CHECK_OUTPUT to capture and validate the expected fatal errors. Individual test cases should simply trigger the error condition.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_INSTALL/test_cases/install_related/include_dirs/CMakeLists.txt:0-0
Timestamp: 2025-06-16T19:47:01.662Z
Learning: In the CMDEF_INSTALL system, export destination paths intentionally contain double slashes like `${CMDEF_LIBRARY_INSTALL_DIR}/cmake//` - this is the actual format produced by the implementation, not a formatting error in tests.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMakeLists.txt:9-11
Timestamp: 2025-06-17T22:15:13.853Z
Learning: The cmakelib-component-cmdef project uses a custom testing framework with TEST.cmake and custom macros like TEST_RUN, TEST_RUN_AND_CHECK_OUTPUT rather than CTest. CTest integration is not needed or used in this project.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_PACKAGE/package_test_macros.cmake:92-96
Timestamp: 2025-06-16T20:53:43.698Z
Learning: The CMDEF_PACKAGE system creates a custom target named "PACKAGE" (uppercase) in multiconfig builds, which should not exist in single-config builds. This is different from CMake's built-in CPack "package" target (lowercase).
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:14-17
Timestamp: 2025-06-08T19:13:14.984Z
Learning: For CMake test cases that verify error conditions (like missing required parameters), use TEST_RUN_AND_CHECK_OUTPUT or similar test harness macros instead of directly calling the function being tested. Direct calls will abort CMake without proper test assertion, while test harnesses can capture and validate expected error messages.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/install_override.cmake:29-52
Timestamp: 2025-06-16T19:50:42.083Z
Learning: When overriding built-in CMake commands (like INSTALL) for testing purposes, MACRO() must be used instead of function() to properly shadow the original command. This is a specific case where the macro form is technically required despite general preferences for functions to avoid variable leakage.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt:215-218
Timestamp: 2025-06-17T21:44:57.056Z
Learning: In CMDEF_ENV cache variable validation tests, hardcoded platform-specific values (like Windows architecture "x86-64" and version "1") are intentional and used to validate expected cache variable values in the test environment, rather than testing dynamic detection capabilities which are covered by other test cases.
system_modules/CMDEF_ENV.cmake (9)
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt:215-218
Timestamp: 2025-06-17T21:44:57.056Z
Learning: In CMDEF_ENV cache variable validation tests, hardcoded platform-specific values (like Windows architecture "x86-64" and version "1") are intentional and used to validate expected cache variable values in the test environment, rather than testing dynamic detection capabilities which are covered by other test cases.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_INSTALL/test_cases/install_related/include_dirs/CMakeLists.txt:0-0
Timestamp: 2025-06-16T19:47:01.662Z
Learning: In the CMDEF_INSTALL system, export destination paths intentionally contain double slashes like `${CMDEF_LIBRARY_INSTALL_DIR}/cmake//` - this is the actual format produced by the implementation, not a formatting error in tests.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_PACKAGE/package_test_macros.cmake:92-96
Timestamp: 2025-06-16T20:53:43.698Z
Learning: The CMDEF_PACKAGE system creates a custom target named "PACKAGE" (uppercase) in multiconfig builds, which should not exist in single-config builds. This is different from CMake's built-in CPack "package" target (lowercase).
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt:138-141
Timestamp: 2025-06-16T19:45:58.536Z
Learning: The CMDEF project intentionally only supports Apple Silicon Macs ("aplsil" architecture) and does not support Intel Macs. The hardcoded architecture checks in tests are deliberate design decisions.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:11-12
Timestamp: 2025-06-16T19:45:39.391Z
Learning: In CMDEF test suites, individual error test cases (like missing_target_error) should NOT include TEST.cmake. They are designed to be executed by main test orchestrators that use TEST_RUN_AND_CHECK_OUTPUT to capture and validate the expected fatal errors. Individual test cases should simply trigger the error condition.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMakeLists.txt:9-11
Timestamp: 2025-06-17T22:15:13.853Z
Learning: The cmakelib-component-cmdef project uses a custom testing framework with TEST.cmake and custom macros like TEST_RUN, TEST_RUN_AND_CHECK_OUTPUT rather than CTest. CTest integration is not needed or used in this project.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/install_override.cmake:29-52
Timestamp: 2025-06-16T19:50:42.083Z
Learning: When overriding built-in CMake commands (like INSTALL) for testing purposes, MACRO() must be used instead of function() to properly shadow the original command. This is a specific case where the macro form is technically required despite general preferences for functions to avoid variable leakage.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:14-17
Timestamp: 2025-06-08T19:13:14.984Z
Learning: For CMake test cases that verify error conditions (like missing required parameters), use TEST_RUN_AND_CHECK_OUTPUT or similar test harness macros instead of directly calling the function being tested. Direct calls will abort CMake without proper test assertion, while test harnesses can capture and validate expected error messages.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt:120-122
Timestamp: 2025-06-16T20:53:24.161Z
Learning: The CACHE_VAR_FORCE_SET macro in cache_var.cmake is designed to handle multiple calls safely - it only saves the original cache variable state on the first call and preserves that saved state through subsequent calls, allowing proper restoration to the pre-test value.
test/TEST.cmake (10)
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMakeLists.txt:9-11
Timestamp: 2025-06-17T22:15:13.853Z
Learning: The cmakelib-component-cmdef project uses a custom testing framework with TEST.cmake and custom macros like TEST_RUN, TEST_RUN_AND_CHECK_OUTPUT rather than CTest. CTest integration is not needed or used in this project.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:14-17
Timestamp: 2025-06-08T19:13:14.984Z
Learning: For CMake test cases that verify error conditions (like missing required parameters), use TEST_RUN_AND_CHECK_OUTPUT or similar test harness macros instead of directly calling the function being tested. Direct calls will abort CMake without proper test assertion, while test harnesses can capture and validate expected error messages.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:11-12
Timestamp: 2025-06-16T19:45:39.391Z
Learning: In CMDEF test suites, individual error test cases (like missing_target_error) should NOT include TEST.cmake. They are designed to be executed by main test orchestrators that use TEST_RUN_AND_CHECK_OUTPUT to capture and validate the expected fatal errors. Individual test cases should simply trigger the error condition.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/install_override.cmake:29-52
Timestamp: 2025-06-16T19:50:42.083Z
Learning: When overriding built-in CMake commands (like INSTALL) for testing purposes, MACRO() must be used instead of function() to properly shadow the original command. This is a specific case where the macro form is technically required despite general preferences for functions to avoid variable leakage.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_INSTALL/test_cases/install_related/include_dirs/CMakeLists.txt:0-0
Timestamp: 2025-06-16T19:47:01.662Z
Learning: In the CMDEF_INSTALL system, export destination paths intentionally contain double slashes like `${CMDEF_LIBRARY_INSTALL_DIR}/cmake//` - this is the actual format produced by the implementation, not a formatting error in tests.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt:120-122
Timestamp: 2025-06-16T20:53:24.161Z
Learning: The CACHE_VAR_FORCE_SET macro in cache_var.cmake is designed to handle multiple calls safely - it only saves the original cache variable state on the first call and preserves that saved state through subsequent calls, allowing proper restoration to the pre-test value.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_INSTALL/test_cases/interface_lib/CMakeLists.txt:93-95
Timestamp: 2025-06-16T19:51:02.357Z
Learning: In modern CMake, the IF command automatically dereferences variables in conditions. For example, `IF(variable_name MATCHES pattern)` works correctly without needing explicit variable expansion syntax like `IF("${variable_name}" MATCHES pattern)`. The explicit syntax is not required for basic variable dereferencing in IF conditions.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt:215-218
Timestamp: 2025-06-17T21:44:57.056Z
Learning: In CMDEF_ENV cache variable validation tests, hardcoded platform-specific values (like Windows architecture "x86-64" and version "1") are intentional and used to validate expected cache variable values in the test environment, rather than testing dynamic detection capabilities which are covered by other test cases.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_PACKAGE/package_test_macros.cmake:92-96
Timestamp: 2025-06-16T20:53:43.698Z
Learning: The CMDEF_PACKAGE system creates a custom target named "PACKAGE" (uppercase) in multiconfig builds, which should not exist in single-config builds. This is different from CMake's built-in CPack "package" target (lowercase).
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/cache_var.cmake:77-79
Timestamp: 2025-06-16T20:53:31.710Z
Learning: CMake does not have an `UNSET_PROPERTY` command. The only way to clear a property in CMake is using `SET_PROPERTY(GLOBAL PROPERTY <prop>)` without a value, which sets the property to an empty value rather than removing it completely.
.github/workflows/tests.yml (3)
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:11-12
Timestamp: 2025-06-16T19:45:39.391Z
Learning: In CMDEF test suites, individual error test cases (like missing_target_error) should NOT include TEST.cmake. They are designed to be executed by main test orchestrators that use TEST_RUN_AND_CHECK_OUTPUT to capture and validate the expected fatal errors. Individual test cases should simply trigger the error condition.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMakeLists.txt:9-11
Timestamp: 2025-06-17T22:15:13.853Z
Learning: The cmakelib-component-cmdef project uses a custom testing framework with TEST.cmake and custom macros like TEST_RUN, TEST_RUN_AND_CHECK_OUTPUT rather than CTest. CTest integration is not needed or used in this project.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:14-17
Timestamp: 2025-06-08T19:13:14.984Z
Learning: For CMake test cases that verify error conditions (like missing required parameters), use TEST_RUN_AND_CHECK_OUTPUT or similar test harness macros instead of directly calling the function being tested. Direct calls will abort CMake without proper test assertion, while test harnesses can capture and validate expected error messages.
README.md (8)
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMakeLists.txt:9-11
Timestamp: 2025-06-17T22:15:13.853Z
Learning: The cmakelib-component-cmdef project uses a custom testing framework with TEST.cmake and custom macros like TEST_RUN, TEST_RUN_AND_CHECK_OUTPUT rather than CTest. CTest integration is not needed or used in this project.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_PACKAGE/package_test_macros.cmake:92-96
Timestamp: 2025-06-16T20:53:43.698Z
Learning: The CMDEF_PACKAGE system creates a custom target named "PACKAGE" (uppercase) in multiconfig builds, which should not exist in single-config builds. This is different from CMake's built-in CPack "package" target (lowercase).
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_INSTALL/test_cases/install_related/include_dirs/CMakeLists.txt:0-0
Timestamp: 2025-06-16T19:47:01.662Z
Learning: In the CMDEF_INSTALL system, export destination paths intentionally contain double slashes like `${CMDEF_LIBRARY_INSTALL_DIR}/cmake//` - this is the actual format produced by the implementation, not a formatting error in tests.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt:215-218
Timestamp: 2025-06-17T21:44:57.056Z
Learning: In CMDEF_ENV cache variable validation tests, hardcoded platform-specific values (like Windows architecture "x86-64" and version "1") are intentional and used to validate expected cache variable values in the test environment, rather than testing dynamic detection capabilities which are covered by other test cases.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/install_override.cmake:29-52
Timestamp: 2025-06-16T19:50:42.083Z
Learning: When overriding built-in CMake commands (like INSTALL) for testing purposes, MACRO() must be used instead of function() to properly shadow the original command. This is a specific case where the macro form is technically required despite general preferences for functions to avoid variable leakage.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:11-12
Timestamp: 2025-06-16T19:45:39.391Z
Learning: In CMDEF test suites, individual error test cases (like missing_target_error) should NOT include TEST.cmake. They are designed to be executed by main test orchestrators that use TEST_RUN_AND_CHECK_OUTPUT to capture and validate the expected fatal errors. Individual test cases should simply trigger the error condition.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:14-17
Timestamp: 2025-06-08T19:13:14.984Z
Learning: For CMake test cases that verify error conditions (like missing required parameters), use TEST_RUN_AND_CHECK_OUTPUT or similar test harness macros instead of directly calling the function being tested. Direct calls will abort CMake without proper test assertion, while test harnesses can capture and validate expected error messages.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt:120-122
Timestamp: 2025-06-16T20:53:24.161Z
Learning: The CACHE_VAR_FORCE_SET macro in cache_var.cmake is designed to handle multiple calls safely - it only saves the original cache variable state on the first call and preserves that saved state through subsequent calls, allowing proper restoration to the pre-test value.
🪛 actionlint (1.7.7)
.github/workflows/tests.yml
34-34: 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)
48-48: 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)
62-62: the runner of "actions/checkout@v2" 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/tests.yml
[error] 14-14: trailing spaces
(trailing-spaces)
[warning] 28-28: wrong indentation: expected 8 but found 10
(indentation)
[warning] 32-32: wrong indentation: expected 6 but found 8
(indentation)
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 71-71: no new line character at the end of file
(new-line-at-end-of-file)
🪛 markdownlint-cli2 (0.17.2)
README.md
39-39: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (12)
system_modules/CMDEF_ENV.cmake (11)
19-19: Minor documentation improvement - looks good.The typo correction clarifies that "aplsil" refers to Apple Silicon architecture.
42-42: Good addition of Windows-specific initialization.Adding the call to
_CMDEF_ENV_SET_WINDOWS_FLAGS()ensures Windows-specific flags are properly initialized during environment setup.
207-211: Excellent architectural improvement - replacing external calls with CMake built-ins.The direct use of
CMAKE_SYSTEM_NAMEwith proper validation is more reliable than external command calls. The added validation ensures the variable is defined before use.
288-288: Improved cache variable description.The updated description is more precise and clearer about the variable's purpose.
411-411: Improved cache variable description.The updated description is clearer and more specific about the copyright being added to binaries.
436-436: Good architectural improvement - using CMake built-in for system processor.Replacing external command calls with
CMAKE_SYSTEM_PROCESSORis more reliable and portable.
439-444: Excellent addition of robust error handling.The validation ensures that the architecture detection doesn't silently fail with empty or invalid values. The error message is clear and actionable.
471-471: Excellent architectural improvement - using CMake built-in for distribution detection.Replacing external
lsb_releasecommand withCMAKE_HOST_SYSTEM_INFORMATIONis more reliable and doesn't depend on external tools being available.
474-478: Excellent addition of robust error handling.The validation ensures that distribution ID detection doesn't silently fail with empty or invalid values. The error message provides clear guidance.
506-506: Good architectural improvement - using CMake built-in for version detection.Replacing external command with
CMAKE_HOST_SYSTEM_INFORMATIONfor version ID detection is more reliable and portable.
509-513: Excellent addition of robust error handling.The validation ensures that version ID detection doesn't silently fail with empty or invalid values. The error message is clear and references the os-release file appropriately.
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/namespace_mismatch/CMakeLists.txt (1)
54-60: Validate thatCMDEF_NAMESPACEproperty exists before comparisonIf either install call fails to attach the property,
main_nsordep_nswill be empty, making the equality check unreliable.
A quick guard avoids false negatives and yields clearer diagnostics.-GET_TARGET_PROPERTY(main_ns nsmain-shared CMDEF_NAMESPACE) -GET_TARGET_PROPERTY(dep_ns nsdep-shared CMDEF_NAMESPACE) +GET_TARGET_PROPERTY(main_ns nsmain-shared CMDEF_NAMESPACE) +GET_TARGET_PROPERTY(dep_ns nsdep-shared CMDEF_NAMESPACE) + +if(NOT main_ns OR NOT dep_ns) + message(FATAL_ERROR + "Test setup error: CMDEF_NAMESPACE property missing " + "(main='${main_ns}', dep='${dep_ns}')") +endif()
test/CMDEF_PACKAGE/test_cases/dependency_inclusion/test_fail/namespace_mismatch/CMakeLists.txt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
version.txt (1)
1-1: Confirm version bump is propagated everywhere (CMake package config, docs, tags, PyPI).Bumping to
1.0.0is a breaking-change signal; double-check that:
• any hard-coded version strings in CMake scripts orsetup.pyare updated,
• release notes / CHANGELOG reflect the milestone,
• CI/CD pipeline creates a corresponding Git tag and package.A quick
rg '0\.2\.0'over the repo should come back empty after this PR is merged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
version.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMakeLists.txt:9-11
Timestamp: 2025-06-17T22:15:13.853Z
Learning: The cmakelib-component-cmdef project uses a custom testing framework with TEST.cmake and custom macros like TEST_RUN, TEST_RUN_AND_CHECK_OUTPUT rather than CTest. CTest integration is not needed or used in this project.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_INSTALL/test_cases/install_related/include_dirs/CMakeLists.txt:0-0
Timestamp: 2025-06-16T19:47:01.662Z
Learning: In the CMDEF_INSTALL system, export destination paths intentionally contain double slashes like `${CMDEF_LIBRARY_INSTALL_DIR}/cmake//` - this is the actual format produced by the implementation, not a formatting error in tests.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:14-17
Timestamp: 2025-06-08T19:13:14.984Z
Learning: For CMake test cases that verify error conditions (like missing required parameters), use TEST_RUN_AND_CHECK_OUTPUT or similar test harness macros instead of directly calling the function being tested. Direct calls will abort CMake without proper test assertion, while test harnesses can capture and validate expected error messages.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:11-12
Timestamp: 2025-06-16T19:45:39.391Z
Learning: In CMDEF test suites, individual error test cases (like missing_target_error) should NOT include TEST.cmake. They are designed to be executed by main test orchestrators that use TEST_RUN_AND_CHECK_OUTPUT to capture and validate the expected fatal errors. Individual test cases should simply trigger the error condition.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt:215-218
Timestamp: 2025-06-17T21:44:57.056Z
Learning: In CMDEF_ENV cache variable validation tests, hardcoded platform-specific values (like Windows architecture "x86-64" and version "1") are intentional and used to validate expected cache variable values in the test environment, rather than testing dynamic detection capabilities which are covered by other test cases.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/install_override.cmake:29-52
Timestamp: 2025-06-16T19:50:42.083Z
Learning: When overriding built-in CMake commands (like INSTALL) for testing purposes, MACRO() must be used instead of function() to properly shadow the original command. This is a specific case where the macro form is technically required despite general preferences for functions to avoid variable leakage.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_PACKAGE/package_test_macros.cmake:92-96
Timestamp: 2025-06-16T20:53:43.698Z
Learning: The CMDEF_PACKAGE system creates a custom target named "PACKAGE" (uppercase) in multiconfig builds, which should not exist in single-config builds. This is different from CMake's built-in CPack "package" target (lowercase).
de58bc9 to
316dabc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/tests.yml (2)
34-36: Unify and moderniseactions/checkoutversion
actions/checkout@v3is flagged as “runner too old” and the Windows job still uses v2.
Bump all three occurrences to the currentv4for consistency and improved performance/features.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4 … - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 … - - uses: actions/checkout@v2 + - uses: actions/checkout@v4Also applies to: 48-50, 62-62
12-14: Minor linting nits: trailing spaces & missing newlineYAML-lint reports trailing spaces (lines 14, 44) and missing final newline (EOF).
Cleaning these up avoids noisy warnings in CI.Also applies to: 44-44, 71-71
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/tests.yml(1 hunks)example/executable/CMakeLists.txt(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- example/executable/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMakeLists.txt:9-11
Timestamp: 2025-06-17T22:15:13.853Z
Learning: The cmakelib-component-cmdef project uses a custom testing framework with TEST.cmake and custom macros like TEST_RUN, TEST_RUN_AND_CHECK_OUTPUT rather than CTest. CTest integration is not needed or used in this project.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_INSTALL/test_cases/install_related/include_dirs/CMakeLists.txt:0-0
Timestamp: 2025-06-16T19:47:01.662Z
Learning: In the CMDEF_INSTALL system, export destination paths intentionally contain double slashes like `${CMDEF_LIBRARY_INSTALL_DIR}/cmake//` - this is the actual format produced by the implementation, not a formatting error in tests.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:14-17
Timestamp: 2025-06-08T19:13:14.984Z
Learning: For CMake test cases that verify error conditions (like missing required parameters), use TEST_RUN_AND_CHECK_OUTPUT or similar test harness macros instead of directly calling the function being tested. Direct calls will abort CMake without proper test assertion, while test harnesses can capture and validate expected error messages.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:11-12
Timestamp: 2025-06-16T19:45:39.391Z
Learning: In CMDEF test suites, individual error test cases (like missing_target_error) should NOT include TEST.cmake. They are designed to be executed by main test orchestrators that use TEST_RUN_AND_CHECK_OUTPUT to capture and validate the expected fatal errors. Individual test cases should simply trigger the error condition.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt:215-218
Timestamp: 2025-06-17T21:44:57.056Z
Learning: In CMDEF_ENV cache variable validation tests, hardcoded platform-specific values (like Windows architecture "x86-64" and version "1") are intentional and used to validate expected cache variable values in the test environment, rather than testing dynamic detection capabilities which are covered by other test cases.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/install_override.cmake:29-52
Timestamp: 2025-06-16T19:50:42.083Z
Learning: When overriding built-in CMake commands (like INSTALL) for testing purposes, MACRO() must be used instead of function() to properly shadow the original command. This is a specific case where the macro form is technically required despite general preferences for functions to avoid variable leakage.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_PACKAGE/package_test_macros.cmake:92-96
Timestamp: 2025-06-16T20:53:43.698Z
Learning: The CMDEF_PACKAGE system creates a custom target named "PACKAGE" (uppercase) in multiconfig builds, which should not exist in single-config builds. This is different from CMake's built-in CPack "package" target (lowercase).
.github/workflows/tests.yml (3)
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:11-12
Timestamp: 2025-06-16T19:45:39.391Z
Learning: In CMDEF test suites, individual error test cases (like missing_target_error) should NOT include TEST.cmake. They are designed to be executed by main test orchestrators that use TEST_RUN_AND_CHECK_OUTPUT to capture and validate the expected fatal errors. Individual test cases should simply trigger the error condition.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMakeLists.txt:9-11
Timestamp: 2025-06-17T22:15:13.853Z
Learning: The cmakelib-component-cmdef project uses a custom testing framework with TEST.cmake and custom macros like TEST_RUN, TEST_RUN_AND_CHECK_OUTPUT rather than CTest. CTest integration is not needed or used in this project.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:14-17
Timestamp: 2025-06-08T19:13:14.984Z
Learning: For CMake test cases that verify error conditions (like missing required parameters), use TEST_RUN_AND_CHECK_OUTPUT or similar test harness macros instead of directly calling the function being tested. Direct calls will abort CMake without proper test assertion, while test harnesses can capture and validate expected error messages.
🪛 actionlint (1.7.7)
.github/workflows/tests.yml
34-34: 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)
48-48: 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)
62-62: the runner of "actions/checkout@v2" 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/tests.yml
[error] 14-14: trailing spaces
(trailing-spaces)
[warning] 28-28: wrong indentation: expected 8 but found 10
(indentation)
[warning] 32-32: wrong indentation: expected 6 but found 8
(indentation)
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 71-71: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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
koudis
force-pushed
the
jan_kubalek/remove_lsbrelease_again
branch
2 times, most recently
from
abef9f5 to
316dabc
Compare
Summary by CodeRabbit
Bug Fixes
MACOSX_BUNDLEoption when adding executables on macOS platforms.Style
Documentation
Chores