Skip to content

update #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
koudis merged 1 commit into from
Jan 10, 2026
Merged

update #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions CMCONF.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,14 +203,15 @@ FUNCTION(CMCONF_INIT_SYSTEM system_name)
_CMCONF_CHECK_AND_NORMALIZE_SYSTEM_NAME("${system_name}" system_name_upper)
IF(CMCONF_SYSTEM_NAME)
IF(NOT CMCONF_SYSTEM_NAME STREQUAL "${system_name_upper}")
IF(NOT CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED)
_CMCONF_MESSAGE(FATAL_ERROR "System name already set. Cannot change system name from '${CMCONF_SYSTEM_NAME}' to '${system_name_upper}'")
ELSE()
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()
Comment on lines +206 to 214
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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:

  1. The word "Overriding" implies the name is being changed
  2. 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.

Suggested change
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.

SET_PROPERTY(CACHE CMCONF_SYSTEM_NAME PROPERTY VALUE "${system_name_upper}")

IF(NOT DEFINED CMAKE_FIND_PACKAGE_NAME)
IF(CMCONF_UNINSTALL AND CMCONF_INSTALL_AS_SYMLINK)
Expand Down
2 changes: 1 addition & 1 deletion test/test_cases/pass/system_name_override_allowed/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ FIND_PACKAGE(CMLIB REQUIRED)
INCLUDE("${CMAKE_CURRENT_LIST_DIR}/../../../TEST.cmake")

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'"
)
Comment on lines 15 to 17
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


11 changes: 5 additions & 6 deletions test/test_cases/pass/system_name_override_allowed/test_config/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,12 @@ FIND_PACKAGE(CMLIB REQUIRED)

INCLUDE("${CMAKE_CURRENT_LIST_DIR}/../../../../../CMCONF.cmake")

SET(CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED ON CACHE INTERNAL "" FORCE)
SET(CMCONF_SYSTEM_NAME_OVERRIDE_ALLOWED ON CACHE BOOL "" FORCE)
SET(CMCONF_SYSTEM_NAME "OVERRIDE_SYSTEM" CACHE STRING "" FORCE)

CMCONF_INIT_SYSTEM("FIRST_SYSTEM")
CMCONF_INIT_SYSTEM("ORIGINAL_SYSTEM")

CMCONF_INIT_SYSTEM("SECOND_SYSTEM")

IF(NOT CMCONF_SYSTEM_NAME STREQUAL "SECOND_SYSTEM")
MESSAGE(FATAL_ERROR "System name should be SECOND_SYSTEM but is '${CMCONF_SYSTEM_NAME}'")
IF(NOT CMCONF_SYSTEM_NAME STREQUAL "OVERRIDE_SYSTEM")
MESSAGE(FATAL_ERROR "System name should be OVERRIDE_SYSTEM but is '${CMCONF_SYSTEM_NAME}'")
ENDIF()