Conversation
WalkthroughUpdates CI workflows to use cmakelib v1.3.1; refactors CMDEF_ENV OS/architecture detection to use host info and a uname fallback on Linux; adjusts tests for CMake script mode and centralizes OS-specific cache-variable tests; bumps package version to 1.0.3. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as CMake (invoker)
participant E as CMDEF_ENV.cmake
participant H as Host OS
participant U as uname
rect rgba(230,245,255,0.6)
Note over C,E: OS detection path
C->>E: call _CMDEF_ENV_SET_OS()
alt CMAKE_SYSTEM_NAME defined
E->>E: _system_name = CMAKE_SYSTEM_NAME
else
E->>H: CMAKE_HOST_SYSTEM_INFORMATION QUERY OS_NAME
H-->>E: _system_name
end
E->>E: normalize _system_name -> _os_name (macos/linux/windows)
E-->>C: OS identified
end
rect rgba(235,255,235,0.6)
Note over C,E: Arch detection (Linux)
C->>E: call _CMDEF_ENV_GET_ARCH()
alt CMAKE_SYSTEM_PROCESSOR defined
E->>E: _arch = CMAKE_SYSTEM_PROCESSOR
else
E->>U: execute `uname -m`
U-->>E: machine string
alt success
E->>E: trim/map -> normalized _arch
else failure
E-->>C: FATAL_ERROR (unable to determine arch)
end
end
E-->>C: arch (normalized)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Version 1.0.2
koudis
force-pushed
the
BAF-1090/local_repository
branch
from
3749136 to
dcb31d4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt (2)
41-46: Avoid masking Linux arch detection issues: remove “aplsil” from Linux supported set.Including "aplsil" (Apple Silicon) in the Linux-specific supported_archs allows a misdetected macOS arch to pass the Linux test. Per prior learnings, Apple Silicon is intentionally macOS-only in this project. Restrict Linux to Linux-appropriate arches.
Apply:
- SET(supported_archs "x86-64" "x86" "aarch64" "aplsil") + SET(supported_archs "x86-64" "x86" "aarch64")
328-333: Mirror the Linux fix: remove “aplsil” from generic supported_archs in ENV test.Even in this generic check, allowing “aplsil” on non-mac platforms weakens the test. Keep the list platform-appropriate or split by CMDEF_OS_*.
Apply:
- SET(supported_archs "x86-64" "x86" "aarch64" "aplsil") + IF(CMDEF_OS_MACOS) + SET(supported_archs "aplsil") + ELSEIF(CMDEF_OS_LINUX) + SET(supported_archs "x86-64" "x86" "aarch64") + ELSEIF(CMDEF_OS_WINDOWS) + SET(supported_archs "x86-64" "x86") + ENDIF()
🧹 Nitpick comments (5)
.github/workflows/tests.yml (1)
13-13: Confirm CMLIB v1.3.1 tag exists; consider--depth 1for faster clones
- ✅ Tag v1.3.1 was found upstream (
999fed7427e239a407ae3e27764f73f7607bdb53), so the version bump intests.ymlis safe.- ⚡ To speed up CI and save bandwidth, update your
git cloneinvocations to use shallow clones:In
.github/workflows/tests.yml(and any other workflows cloning cmakelib), change:- git clone --branch "v${CMLIB_VERSION}" https://github.com/cmakelib/cmakelib.git + git clone --depth 1 --branch "v${CMLIB_VERSION}" https://github.com/cmakelib/cmakelib.gitAnd for PowerShell:
- git clone --branch "v$Env:CMLIB_VERSION" https://github.com/cmakelib/cmakelib.git + git clone --depth 1 --branch "v$Env:CMLIB_VERSION" https://github.com/cmakelib/cmakelib.gitsystem_modules/CMDEF_ENV.cmake (2)
214-225: Provide a final platform-variable fallback if mapping from system name fails.If OS_NAME is unexpected (e.g., a distro variant string), the mapping leaves os_name empty. A safe fallback using CMake’s built-in WIN32/APPLE/UNIX flags avoids a hard fail.
Insert after the existing Darwin/Linux/Windows mapping:
ELSEIF("${_os_name}" STREQUAL "Windows") SET(os_name "windows") ENDIF() + # Fallback by standard platform flags if not resolved above + IF("${os_name}" STREQUAL "") + IF(WIN32) + SET(os_name "windows") + ELSEIF(APPLE) + SET(os_name "macos") + ELSEIF(UNIX) + SET(os_name "linux") + ENDIF() + ENDIF()
439-453: Linux arch detection: trim output reliably and harden uname discovery.Good improvement by preferring CMAKE_SYSTEM_PROCESSOR. A couple of tweaks improve resilience:
- Use OUTPUT_STRIP_TRAILING_WHITESPACE instead of manual regex trimming.
- Fail fast if uname is not found (rare but clearer error).
- Avoid passing NO_CACHE to FIND_PROGRAM (not a documented option).
Apply:
- SET(_arch) - IF(DEFINED CMAKE_SYSTEM_PROCESSOR) - SET(_arch "${CMAKE_SYSTEM_PROCESSOR}") - ELSE() - FIND_PROGRAM(uname_exe uname NO_CACHE REQUIRED) - EXECUTE_PROCESS( - COMMAND ${uname_exe} -m - OUTPUT_VARIABLE _arch - RESULT_VARIABLE result - ) - STRING(REGEX REPLACE "\n" "" _arch "${_arch}") - IF(NOT result EQUAL 0) - MESSAGE(FATAL_ERROR "Cannot determine system architecture. uname -m failed") - ENDIF() - ENDIF() + SET(_arch) + IF(DEFINED CMAKE_SYSTEM_PROCESSOR AND NOT "${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "") + SET(_arch "${CMAKE_SYSTEM_PROCESSOR}") + ELSE() + FIND_PROGRAM(UNAME_EXE uname) + IF(NOT UNAME_EXE) + MESSAGE(FATAL_ERROR "Cannot determine system architecture: 'uname' not found") + ENDIF() + EXECUTE_PROCESS( + COMMAND "${UNAME_EXE}" -m + OUTPUT_VARIABLE _arch + OUTPUT_STRIP_TRAILING_WHITESPACE + RESULT_VARIABLE _uname_res + ) + IF(NOT _uname_res EQUAL 0 OR "${_arch}" STREQUAL "") + MESSAGE(FATAL_ERROR "Cannot determine system architecture. 'uname -m' failed") + ENDIF() + ENDIF()Additionally (outside this hunk), consider normalizing common synonyms before the supported-list check:
# Map common synonyms if(_arch_normalized STREQUAL "amd64") set(_arch_normalized "x86-64") elseif(_arch_normalized STREQUAL "arm64") set(_arch_normalized "aarch64") endif()test/CMDEF_ENV/CMakeLists.txt (1)
19-36: Capture test output for better diagnostics when script-mode checks fail.The execute_process calls work, but failures would be easier to debug if we included stdout/stderr in the fatal message and stripped trailing whitespace.
Apply:
EXECUTE_PROCESS( COMMAND "${CMAKE_COMMAND}" -P "${CMAKE_CURRENT_LIST_DIR}/os_detection/CMakeLists.txt" WORKING_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}/os_detection" - RESULT_VARIABLE result_var + RESULT_VARIABLE result_var + OUTPUT_VARIABLE out_os + ERROR_VARIABLE err_os + OUTPUT_STRIP_TRAILING_WHITESPACE + ERROR_STRIP_TRAILING_WHITESPACE ) IF(NOT result_var EQUAL 0) - MESSAGE(FATAL_ERROR "os_detection test did not pass in script mode!") + MESSAGE(FATAL_ERROR "os_detection (script mode) failed. stdout='${out_os}' stderr='${err_os}'") ENDIF() EXECUTE_PROCESS( COMMAND "${CMAKE_COMMAND}" -P "${CMAKE_CURRENT_LIST_DIR}/arch_detection/CMakeLists.txt" WORKING_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}/arch_detection" - RESULT_VARIABLE result_var + RESULT_VARIABLE result_var + OUTPUT_VARIABLE out_arch + ERROR_VARIABLE err_arch + OUTPUT_STRIP_TRAILING_WHITESPACE + ERROR_STRIP_TRAILING_WHITESPACE ) IF(NOT result_var EQUAL 0) - MESSAGE(FATAL_ERROR "arch_detection test did not pass in script mode!") + MESSAGE(FATAL_ERROR "arch_detection (script mode) failed. stdout='${out_arch}' stderr='${err_arch}'") ENDIF()test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt (1)
310-343: “Environment variable support” test doesn’t validate overrides.This function only reasserts that variables are defined and in a set; it doesn’t actually set ENV and re-initialize detection. As a result, it can’t catch regressions in env override logic.
Two options:
- Spin a sub-process using TEST_RUN to invoke a tiny CMake script that sets ENV and includes CMDEF_ENV, then assert the overridden values in that sub-process.
- Or add a small re-init hook in CMDEF_ENV (if available) that re-reads ENV, then call it here.
Happy to draft the sub-test script and harness if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.github/workflows/tests.yml(1 hunks)system_modules/CMDEF_ENV.cmake(2 hunks)test/CMDEF_ENV/CMakeLists.txt(1 hunks)test/CMDEF_ENV/arch_detection/CMakeLists.txt(1 hunks)test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt(2 hunks)test/CMDEF_ENV/os_detection/CMakeLists.txt(1 hunks)version.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
📚 Learning: 2025-06-17T22:15:13.853Z
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.
Applied to files:
test/CMDEF_ENV/arch_detection/CMakeLists.txttest/CMDEF_ENV/CMakeLists.txttest/CMDEF_ENV/os_detection/CMakeLists.txttest/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
📚 Learning: 2025-06-17T21:44:57.056Z
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.
Applied to files:
test/CMDEF_ENV/arch_detection/CMakeLists.txttest/CMDEF_ENV/CMakeLists.txttest/CMDEF_ENV/os_detection/CMakeLists.txttest/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
📚 Learning: 2025-06-08T19:13:14.984Z
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.
Applied to files:
test/CMDEF_ENV/arch_detection/CMakeLists.txttest/CMDEF_ENV/CMakeLists.txttest/CMDEF_ENV/os_detection/CMakeLists.txttest/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
📚 Learning: 2025-06-16T19:45:58.536Z
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.
Applied to files:
test/CMDEF_ENV/arch_detection/CMakeLists.txttest/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
📚 Learning: 2025-06-16T19:45:39.391Z
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.
Applied to files:
test/CMDEF_ENV/arch_detection/CMakeLists.txttest/CMDEF_ENV/CMakeLists.txttest/CMDEF_ENV/os_detection/CMakeLists.txt
📚 Learning: 2025-06-16T19:47:01.662Z
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.
Applied to files:
test/CMDEF_ENV/CMakeLists.txttest/CMDEF_ENV/os_detection/CMakeLists.txttest/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
📚 Learning: 2025-06-16T20:53:24.161Z
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.
Applied to files:
test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
🪛 LanguageTool
version.txt
[grammar] ~1-~1: Hier könnte ein Fehler sein.
Context: version=1.0.3
(QB_NEW_DE)
⏰ 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
🔇 Additional comments (11)
version.txt (1)
1-1: No further version updates requiredI ran the grep checks against the entire repo. The only
1.0.3pin is in version.txt, and all remaining occurrences of1.0.0live in:
- test-case CMakeLists (sample fixtures exercising the macro)
- documentation examples in doc/PackageChain.md
There are no hard-coded version strings in packaging or in a static CMDEFConfig.cmake—those are generated at install time from version.txt. The tests and docs intentionally use “1.0.0” as illustrative examples, so they do not need to track the bump to 1.0.3.
All metadata stays in sync. Merging as-is.
system_modules/CMDEF_ENV.cmake (1)
207-214: Manual verification required: CMake script-mode OS detectionCMake isn’t available in our sandbox environment, so the proposed change can’t be automatically exercised here. Please verify on a clean machine with CMake installed by running:
•
cmake -P test/CMDEF_ENV/os_detection/CMakeLists.txt
•cmake -P test/CMDEF_ENV/arch_detection/CMakeLists.txtEnsure that in script mode or when
CMAKE_SYSTEM_NAMEis undefined or"Generic",_os_namecorrectly falls back to the host OS name as per the diff.test/CMDEF_ENV/arch_detection/CMakeLists.txt (1)
6-9: Guarding project setup in script mode is correct.This makes the test cmake -P friendly and aligns with the new script-mode checks added in the parent test driver.
test/CMDEF_ENV/os_detection/CMakeLists.txt (4)
16-23: OS expectations match CMDEF_ENV; LGTM.The asserted CMDEF_OS_* and name variables for Linux are consistent with the CMDEF_ENV contract. Once the CYGWIN guard is added, this block looks solid.
24-31: Windows branch assertions look consistent.POSIX=false on Windows aligns with prior semantics in this repo. No changes needed.
32-40: macOS branch assertions look consistent with project policy.The macOS expectations (POSIX=true, name triplet) align with CMDEF_ENV. No changes needed.
6-9: Ensure CMLIB is locatable in script-mode before invoking find_packageThe os_detection script (test/CMDEF_ENV/os_detection/CMakeLists.txt) calls
FIND_PACKAGE(CMLIB REQUIRED)immediately after the script-mode guard—yet it never loads CMDEFConfig or adjusts CMAKE_PREFIX_PATH/CMAKE_MODULE_PATH before that call. When you run
cmake -P os_detection/CMakeLists.txtvia EXECUTE_PROCESS, no module path is established, so CMLIB may not be found and the test will fail in script mode.
Please address the following:
- In test/CMDEF_ENV/os_detection/CMakeLists.txt, move or duplicate the logic that sets up module/search paths (e.g. include CMDEFConfig.cmake or manually append to CMAKE_PREFIX_PATH) so that FIND_PACKAGE(CMLIB) runs after paths are configured.
- Alternatively, modify the EXECUTE_PROCESS invocation in test/CMDEF_ENV/CMakeLists.txt to pass
-D CMAKE_PREFIX_PATH=<path-to-CMLIB>(or-D CMLIB_DIR) so that the script-mode CMake invocation can locate CMLIB.- Verify that after your changes, both the normal TEST_RUN and the
cmake -Pinvocation succeed in locating and loading CMLIB.⛔ Skipped due to learnings
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_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.test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt (4)
9-10: Doc nit: Nice cross-platform scope clarification.The added note clarifies the test’s focus on variables shared across platforms. Looks good.
182-244: Windows cache validations look consistent with previous intent.The fixed arch “x86-64” and version “1” are aligned with the established, intentional test environment assumptions captured in prior learnings. No changes needed.
252-301: Common cache validations are solid and focused.Good coverage on language extensions and shared naming conventions. No action needed.
121-169: No changes needed—tests intentionally pin static macOS version “11”The cache‐variable validation suite purposely hardcodes platform‐specific values (including
CMDEF_DISTRO_VERSION_ID "11") to ensure that any change in the system logic is immediately caught and explicitly updated. This aligns with existing patterns (e.g. Windows version “1”, Apple-Silicon “aplsil”) and is by design.• File:
test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
• Lines: 121–169Please disregard the earlier suggestion to parameterize or gate the macOS version check—static version pins are deliberate.
Likely an incorrect or invalid review comment.
koudis
force-pushed
the
BAF-1090/local_repository
branch
from
94c10c1 to
6e3e6a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt (1)
345-353: Great: centralized OS dispatch now fails fast on unsupported/undetected OS.This resolves the earlier feedback about silent skips on unknown platforms.
🧹 Nitpick comments (7)
.github/workflows/examples.yml (2)
39-41: Shallow-clone cmakelib to speed up CI (~2-5x faster clone).We only need the tagged snapshot; add --depth 1 to reduce network and time.
- git clone --branch "v${CMLIB_VERSION}" https://github.com/cmakelib/cmakelib.git + git clone --depth 1 --branch "v${CMLIB_VERSION}" https://github.com/cmakelib/cmakelib.git- git clone --branch "v${CMLIB_VERSION}" https://github.com/cmakelib/cmakelib.git + git clone --depth 1 --branch "v${CMLIB_VERSION}" https://github.com/cmakelib/cmakelib.gitAlso applies to: 89-91
64-66: Normalize PowerShell env-var casing and use Join-Path for robust Windows paths.PowerShell is case-insensitive, but aligning with tests.yml and using Join-Path avoids forward-slash path issues and handles spaces.
- git clone --branch "v$env:CMLIB_VERSION" https://github.com/cmakelib/cmakelib.git - $env:CMLIB_DIR = "$(pwd)/cmakelib" + git clone --depth 1 --branch "v$Env:CMLIB_VERSION" https://github.com/cmakelib/cmakelib.git + $Env:CMLIB_DIR = (Join-Path -Path (Get-Location).Path -ChildPath 'cmakelib').github/workflows/tests.yml (2)
41-43: Use shallow clones for cmakelib across all OS runners.Small optimization to reduce CI time and bandwidth.
- git clone --branch "v${CMLIB_VERSION}" https://github.com/cmakelib/cmakelib.git + git clone --depth 1 --branch "v${CMLIB_VERSION}" https://github.com/cmakelib/cmakelib.git- git clone --branch "v${CMLIB_VERSION}" https://github.com/cmakelib/cmakelib.git + git clone --depth 1 --branch "v${CMLIB_VERSION}" https://github.com/cmakelib/cmakelib.git- git clone --branch "v$Env:CMLIB_VERSION" https://github.com/cmakelib/cmakelib.git + git clone --depth 1 --branch "v$Env:CMLIB_VERSION" https://github.com/cmakelib/cmakelib.gitAlso applies to: 55-57, 69-71
69-71: Minor consistency nit: keep $Env casing uniform across workflows.Tests already use $Env; Examples uses $env in one place. Consider aligning to $Env in both.
test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt (3)
9-10: Fix misleading comment about scope of validations.This file validates both per-OS and cross-platform variables; the current note suggests only cross-platform checks.
-# It tests only variables which are shared across all platforms. -# +# This file validates both per-OS and cross-platform CMDEF_ENV cache variables. +# Cross-platform checks are centralized in TEST_COMMON_CACHE_VARIABLES().
41-47: Tighten Linux architecture allow-list (avoid mac-only 'aplsil').Per project learnings, "aplsil" is macOS-only by design. Allowing it on Linux could hide a misclassification.
- SET(supported_archs "x86-64" "x86" "aarch64" "aplsil") + # Linux-supported architectures + SET(supported_archs "x86-64" "x86" "aarch64")If cross-compilation or env overrides are intended to set non-Linux values, consider asserting that overrides are exercised in TEST_ENVIRONMENT_VARIABLES instead. (Reference: retrieved learning about Apple Silicon being mac-only.)
313-317: Remove unused locals saved for restoration (not used).These variables are saved but never used or restored in this function.
- # Save current values - SET(original_arch "${CMDEF_ARCHITECTURE}") - SET(original_distro_id "${CMDEF_DISTRO_ID}") - SET(original_distro_version "${CMDEF_DISTRO_VERSION_ID}") + # Intentionally not mutating environment or cache state in this test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/examples.yml(1 hunks).github/workflows/tests.yml(1 hunks)test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
📚 Learning: 2025-06-17T21:44:57.056Z
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.
Applied to files:
test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
📚 Learning: 2025-08-22T21:46:21.663Z
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#14
File: test/CMDEF_ENV/os_detection/CMakeLists.txt:16-24
Timestamp: 2025-08-22T21:46:21.663Z
Learning: The cmakelib-component-cmdef project intentionally does not support Cygwin platform. OS detection logic using IF(UNIX AND NOT APPLE) is acceptable without Cygwin guards since Cygwin is out of scope.
Applied to files:
test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
📚 Learning: 2025-06-08T19:13:14.984Z
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.
Applied to files:
test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
📚 Learning: 2025-06-16T19:45:58.536Z
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.
Applied to files:
test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
📚 Learning: 2025-06-16T19:47:01.662Z
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.
Applied to files:
test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
📚 Learning: 2025-06-17T22:15:13.853Z
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.
Applied to files:
test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
📚 Learning: 2025-06-16T20:53:24.161Z
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.
Applied to files:
test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test_windows
🔇 Additional comments (2)
.github/workflows/examples.yml (1)
13-13: Version bump to cmakelib v1.3.1 — looks good.Consistent with the Tests workflow; no functional concerns here.
.github/workflows/tests.yml (1)
13-13: Version bump to cmakelib v1.3.1 — looks good.Matches the Examples workflow; keeps CI in sync with the new tag.
1 participant
Summary by CodeRabbit