Conversation
WalkthroughThis PR modifies the system name override handling logic in CMCONF.cmake by inverting conditional behavior for warning/error emission, relocating cache assignment to an else-branch to defer property setting until system name is determined as new, and updates corresponding test cases with revised system name placeholders (OVERRIDE_SYSTEM and ORIGINAL_SYSTEM) and cache variable type adjustments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CMCONF.cmake (1)
184-189: Documentation is inconsistent with implementation.The documentation states "the system name can be changed" when
CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED=ON, but the implementation at lines 206-214 does not change the system name—it only emits a warning and keeps the existing value.Please update the documentation to accurately describe the behavior:
- When
CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED=ON: The existingCMCONF_SYSTEM_NAMEis preserved, and a warning is emitted about the attempted re-initialization.- When
CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED=OFF: A fatal error is raised.
🤖 Fix all issues with AI agents
In @CMCONF.cmake:
- Around line 206-214: The warning message inside the
IF(CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED) branch is misleading because no override
is actually performed; update the _CMCONF_MESSAGE(...) invocation so it does not
say "Overriding ... to ..." but instead clearly states an attempted override was
detected and that the cache value CMCONF_SYSTEM_NAME remains unchanged (use
CMCONF_SYSTEM_NAME and system_name_upper to compose the message), e.g.
"Attempted to override system name from 'X' to 'Y' — override not applied;
current value remains 'X'"; keep this change in the branch where
CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED is checked and leave the ELSE() FATAL_ERROR
behavior unchanged.
In @test/test_cases/pass/system_name_override_allowed/CMakeLists.txt:
- Around line 15-17: The test's expected WARNING_MESSAGE in
TEST_RUN_AND_CHECK_OUTPUT is misleading because CMCONF.cmake preserves the
system name as 'OVERRIDE_SYSTEM' rather than changing it to 'ORIGINAL_SYSTEM';
update the expected warning text to reflect that the override was ignored (e.g.,
"Ignoring request to change system name from 'OVERRIDE_SYSTEM' to
'ORIGINAL_SYSTEM'" or "System name 'OVERRIDE_SYSTEM' preserved; ignoring
'ORIGINAL_SYSTEM'") so the test expectation matches the actual behavior of
CMCONF.cmake and the test config.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CMCONF.cmaketest/test_cases/pass/system_name_override_allowed/CMakeLists.txttest/test_cases/pass/system_name_override_allowed/test_config/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 (1)
test/test_cases/pass/system_name_override_allowed/test_config/CMakeLists.txt (1)
10-17: LGTM—test correctly validates the override behavior.The test appropriately:
- Pre-sets
CMCONF_SYSTEM_NAMEto simulate a command-line override- Enables
CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED- Calls
CMCONF_INIT_SYSTEMwith a different name- Verifies the pre-set value is preserved
The change from
CACHE INTERNALtoCACHE BOOLforCMCONF_SYSTEM_NAME_OVERRIDE_ALLOWEDis appropriate since this is a user-configurable option.
| IF(CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED) | ||
| _CMCONF_MESSAGE(WARNING "Overriding system name from '${CMCONF_SYSTEM_NAME}' to '${system_name_upper}'") | ||
| ELSE() | ||
| _CMCONF_MESSAGE(FATAL_ERROR "System name already set. Cannot change system name from '${CMCONF_SYSTEM_NAME}' to '${system_name_upper}'") | ||
| ENDIF() | ||
| ENDIF() | ||
| ELSE() | ||
| SET_PROPERTY(CACHE CMCONF_SYSTEM_NAME PROPERTY VALUE "${system_name_upper}") | ||
| ENDIF() |
There was a problem hiding this comment.
Warning message is misleading—no override actually occurs.
When CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED=ON and the names differ, the warning at line 207 says "Overriding system name from 'X' to 'Y'", but the system name is not changed—it remains X. This is confusing because:
- The word "Overriding" implies the name is being changed
- The order "from 'X' to 'Y'" suggests Y becomes the new value
Consider rewording to accurately reflect what happens:
Suggested fix
IF(CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED)
- _CMCONF_MESSAGE(WARNING "Overriding system name from '${CMCONF_SYSTEM_NAME}' to '${system_name_upper}'")
+ _CMCONF_MESSAGE(WARNING "System name already set to '${CMCONF_SYSTEM_NAME}'; ignoring attempt to initialize as '${system_name_upper}'")
ELSE()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IF(CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED) | |
| _CMCONF_MESSAGE(WARNING "Overriding system name from '${CMCONF_SYSTEM_NAME}' to '${system_name_upper}'") | |
| ELSE() | |
| _CMCONF_MESSAGE(FATAL_ERROR "System name already set. Cannot change system name from '${CMCONF_SYSTEM_NAME}' to '${system_name_upper}'") | |
| ENDIF() | |
| ENDIF() | |
| ELSE() | |
| SET_PROPERTY(CACHE CMCONF_SYSTEM_NAME PROPERTY VALUE "${system_name_upper}") | |
| ENDIF() | |
| IF(CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED) | |
| _CMCONF_MESSAGE(WARNING "System name already set to '${CMCONF_SYSTEM_NAME}'; ignoring attempt to initialize as '${system_name_upper}'") | |
| ELSE() | |
| _CMCONF_MESSAGE(FATAL_ERROR "System name already set. Cannot change system name from '${CMCONF_SYSTEM_NAME}' to '${system_name_upper}'") | |
| ENDIF() | |
| ENDIF() | |
| ELSE() | |
| SET_PROPERTY(CACHE CMCONF_SYSTEM_NAME PROPERTY VALUE "${system_name_upper}") | |
| ENDIF() |
🤖 Prompt for AI Agents
In @CMCONF.cmake around lines 206 - 214, The warning message inside the
IF(CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED) branch is misleading because no override
is actually performed; update the _CMCONF_MESSAGE(...) invocation so it does not
say "Overriding ... to ..." but instead clearly states an attempted override was
detected and that the cache value CMCONF_SYSTEM_NAME remains unchanged (use
CMCONF_SYSTEM_NAME and system_name_upper to compose the message), e.g.
"Attempted to override system name from 'X' to 'Y' — override not applied;
current value remains 'X'"; keep this change in the branch where
CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED is checked and leave the ELSE() FATAL_ERROR
behavior unchanged.
| TEST_RUN_AND_CHECK_OUTPUT("test_config" | ||
| WARNING_MESSAGE "CMCONF\\\\[FIRST_SYSTEM\\\\] - Overriding system name from 'FIRST_SYSTEM' to.*'SECOND_SYSTEM'" | ||
| WARNING_MESSAGE "CMCONF\\\\[OVERRIDE_SYSTEM\\\\] - Overriding system name from 'OVERRIDE_SYSTEM' to.*'ORIGINAL_SYSTEM'" | ||
| ) |
There was a problem hiding this comment.
Warning message expectation may be misleading.
The expected warning says "Overriding system name from 'OVERRIDE_SYSTEM' to 'ORIGINAL_SYSTEM'", but based on the implementation in CMCONF.cmake and the test config, the system name actually remains OVERRIDE_SYSTEM (it is not changed to ORIGINAL_SYSTEM). The warning message is therefore misleading—it implies a change that doesn't occur.
Consider rephrasing to something like:
"Ignoring request to change system name from 'OVERRIDE_SYSTEM' to 'ORIGINAL_SYSTEM'""System name 'OVERRIDE_SYSTEM' preserved; ignoring 'ORIGINAL_SYSTEM'"
🤖 Prompt for AI Agents
In @test/test_cases/pass/system_name_override_allowed/CMakeLists.txt around
lines 15 - 17, The test's expected WARNING_MESSAGE in TEST_RUN_AND_CHECK_OUTPUT
is misleading because CMCONF.cmake preserves the system name as
'OVERRIDE_SYSTEM' rather than changing it to 'ORIGINAL_SYSTEM'; update the
expected warning text to reflect that the override was ignored (e.g., "Ignoring
request to change system name from 'OVERRIDE_SYSTEM' to 'ORIGINAL_SYSTEM'" or
"System name 'OVERRIDE_SYSTEM' preserved; ignoring 'ORIGINAL_SYSTEM'") so the
test expectation matches the actual behavior of CMCONF.cmake and the test
config.
1 participant
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.