Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce support for overriding configuration variables with environment variables in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CMake
participant CMCONF_GET
User->>CMake: Configure project
CMake->>CMCONF_GET: Request VARIABLE_X
alt Env variable SYSTEM_VARIABLE_X is set
CMCONF_GET->>CMake: Return env variable value
else Env variable not set
CMCONF_GET->>CMake: Return config/cache variable value
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
CMCONF.cmake (2)
238-240: Document precedence and provide a quick example for ENV vs -DCurrent docs state both sources but not which wins. Code prefers ENV when set. Clarify to avoid ambiguity.
-# The variable can be specified by environment variable or by -D cmdline option as CMake cache variable. -# The variable name is constructed as SYSTEM_NAME_VARIABLE_NAME (both uppercase). +# The variable value can come either from an environment variable or from a -D command-line cache entry. +# Precedence: if both are provided, the environment variable wins. +# The variable name is constructed as SYSTEM_NAME_VARIABLE_NAME (both uppercase). +# Example: for system "TEST" and variable "VARIABLE_A", use ENV TEST_VARIABLE_A or -DTEST_VARIABLE_A=...
279-283: Optional: ignore empty ENV values and (optionally) preserve semicolonsIf an ENV var is present but empty, you may want to fall back to the configured value. Also, semicolons in ENV values become list separators in CMake; escape them if you want a plain string.
-IF(DEFINED ENV{${actual_var_name}}) - SET(${var_name} "$ENV{${actual_var_name}}" PARENT_SCOPE) -ELSE() - SET(${var_name} "${${actual_var_name}}" PARENT_SCOPE) -ENDIF() +IF(DEFINED ENV{${actual_var_name}} AND NOT "$ENV{${actual_var_name}}" STREQUAL "") + # Uncomment the next two lines if you want to preserve semicolons as literal characters: + # set(_cmconf_env_value "$ENV{${actual_var_name}}") + # string(REPLACE ";" "\\;" _cmconf_env_value "${_cmconf_env_value}") + # SET(${var_name} "${_cmconf_env_value}" PARENT_SCOPE) + SET(${var_name} "$ENV{${actual_var_name}}" PARENT_SCOPE) +ELSE() + SET(${var_name} "${${actual_var_name}}" PARENT_SCOPE) +ENDIF()test/test_cases/pass/env_variable_override/CMakeLists.txt (3)
27-31: Core behavior validated; consider hardening against false positivesThe assertions correctly verify that ENV overrides config. To avoid accidental pass if the config value already equals "env_override_value", consider also asserting the fallback value in a separate test/run (after env is unset and state is reinitialized), or explicitly checking that the template’s default differs.
I can add a companion test that validates fallback after unsetting the ENV and reinitializing state.
27-28: Document the expected ENV naming conventionAdd a brief comment stating the expected ENV key pattern (e.g., _) so future readers understand why TEST_VARIABLE_A maps to VARIABLE_A.
-SET(ENV{TEST_VARIABLE_A} "env_override_value") +# Convention: ENV override key is <SYSTEM>_<VAR>. Here: TEST_VARIABLE_A -> VARIABLE_A. +SET(ENV{TEST_VARIABLE_A} "env_override_value")
25-34: Best-effort cleanup even on assertion failure (project mode only)If a test assertion aborts, lines 33-34 won’t run. ENV is process-local, but the installed config may linger. Optionally schedule deferred cleanup when available (project mode), while keeping script-mode compatibility.
CMCONF_INIT_SYSTEM(TEST) +# In project mode, ensure cleanup even if a later assertion fails. +IF(NOT DEFINED CMAKE_SCRIPT_MODE_FILE AND COMMAND cmake_language) + cmake_language(DEFER CALL UNSET "ENV{TEST_VARIABLE_A}") + cmake_language(DEFER CALL TEST_CMCONF_UNINSTALL_CONFIG "${CMCONF_TEST_CONFIG_FILE}" "TEST") +ENDIF() + SET(ENV{TEST_VARIABLE_A} "env_override_value") CMCONF_GET(VARIABLE_A) @@ UNSET(ENV{TEST_VARIABLE_A}) TEST_CMCONF_UNINSTALL_CONFIG("${CMCONF_TEST_CONFIG_FILE}" "TEST")Note: This keeps script-mode behavior unchanged and uses DEFER only where supported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CMCONF.cmake(2 hunks)test/CMakeLists.txt(1 hunks)test/test_cases/fail/env_variable_config_required/CMakeLists.txt(1 hunks)test/test_cases/fail/env_variable_config_required/test_config/CMakeLists.txt(1 hunks)test/test_cases/pass/env_variable_fallback/CMakeLists.txt(1 hunks)test/test_cases/pass/env_variable_override/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-02T17:36:23.478Z
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmconf#1
File: test/test_cases/fail/variable_not_defined/CMakeLists.txt:17-23
Timestamp: 2025-08-02T17:36:23.478Z
Learning: The cmake_language(DEFER) construct is not available in CMake script mode, only in project mode, making it unsuitable for tests that need to run in both modes.
Applied to files:
test/CMakeLists.txttest/test_cases/fail/env_variable_config_required/test_config/CMakeLists.txttest/test_cases/fail/env_variable_config_required/CMakeLists.txttest/test_cases/pass/env_variable_fallback/CMakeLists.txttest/test_cases/pass/env_variable_override/CMakeLists.txt
🔇 Additional comments (8)
CMCONF.cmake (1)
279-283: ENV override is correctly implemented and scopedLogic is sound: ensure var exists in config, then prefer ENV if present, otherwise use cache. Matches the PR objective.
test/CMakeLists.txt (2)
21-22: Good coverage: add pass tests for ENV fallback/overrideNice additions. They exercise both absence and presence of ENV to validate precedence and backward compatibility.
28-28: Failing test inclusion looks rightThe negative test ensures undefined variables cannot be sourced solely from ENV. Aligned with intended guardrails.
test/test_cases/fail/env_variable_config_required/CMakeLists.txt (1)
23-27: Failure expectation and regex look correctInstalls config, runs the sub-test, and asserts the fatal error about undefined variable in config. Regex targets the exact CMCONF message format.
test/test_cases/pass/env_variable_fallback/CMakeLists.txt (1)
27-34: Solid fallback testExplicitly unsetting ENV then asserting the configured value verifies backward compatibility of CMCONF_GET.
test/test_cases/fail/env_variable_config_required/test_config/CMakeLists.txt (1)
16-20: Correctly simulates undefined var with ENV presentSetting TEST_UNDEFINED_VARIABLE, initializing system, then calling CMCONF_GET triggers the intended fatal path. Clean and minimal.
test/test_cases/pass/env_variable_override/CMakeLists.txt (2)
9-12: Good handling of script vs project mode; compliant with prior lessonsGuarding PROJECT/MINIMUM_REQUIRED behind CMAKE_SCRIPT_MODE_FILE avoids features unavailable in script mode (e.g., DEFER). This aligns with the retrieved learning.
1-35: Test case is already wired into the suite
Theenv_variable_overridetest is invoked intest/CMakeLists.txt:
- test/CMakeLists.txt:22
TEST_RUN("${CMAKE_CURRENT_LIST_DIR}/test_cases/pass/env_variable_override")No additional registration or discovery macros are required.
1 participant
Summary by CodeRabbit
New Features
Tests