Remove explicit need of uname and lsb_release file #10
Conversation
WalkthroughThe pull request introduces several changes across multiple CMake-related files. Key modifications include updating the minimum required CMake version from 3.21 to 3.22 in Changes
Sequence DiagramsequenceDiagram
participant CMake
participant CMDEF_ENV
participant System
CMake->>CMDEF_ENV: Request system information
CMDEF_ENV->>System: Use CMAKE_HOST_SYSTEM_INFORMATION
System-->>CMDEF_ENV: Return system details
CMDEF_ENV->>CMDEF_ENV: Normalize and validate system info
CMDEF_ENV-->>CMake: Provide system configuration
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 1
🧹 Nitpick comments (1)
system_modules/CMDEF_ENV.cmake (1)
Line range hint
432-510: Great improvements to system information detection!The changes successfully:
- Remove dependencies on external commands (
unameandlsb_release)- Use CMake's built-in
CMAKE_HOST_SYSTEM_INFORMATIONcommand consistently- Implement consistent string normalization patterns
- Add robust error handling with descriptive messages
Consider extracting the common string normalization pattern into a helper function to reduce code duplication, as it's used in three places:
STRING(REGEX REPLACE "[^a-zA-Z0-9.]" "-" _mapped "${_value}") STRING(TOLOWER "${_mapped}" _normalized)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
FindCMDEF.cmake(1 hunks)system_modules/CMDEF_ENV.cmake(3 hunks)
🔇 Additional comments (3)
FindCMDEF.cmake (1)
12-12: LGTM! Version bump is necessary.The increase in minimum CMake version from 3.21 to 3.22 is required to support the new
CMAKE_HOST_SYSTEM_INFORMATIONqueries (DISTRIB_IDandDISTRIB_VERSION_ID) that replace the external command dependencies.system_modules/CMDEF_ENV.cmake (2)
432-440: LGTM! Improved system architecture detection.The changes effectively:
- Remove dependency on external
unamecommand- Add proper validation for empty/invalid architecture strings
- Normalize architecture string consistently
467-475: LGTM! Improved distribution ID detection.The changes effectively:
- Remove dependency on external
lsb_releasecommand- Add proper validation for empty/invalid distribution IDs
- Normalize distribution ID string consistently
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
system_modules/CMDEF_ENV.cmake (4)
432-440: LGTM! Clean replacement of uname dependency.The use of
CMAKE_HOST_SYSTEM_INFORMATIONwith proper string normalization is a good approach. The error handling is thorough and provides clear guidance.Consider adding quotes around the error message components for better readability:
- MESSAGE(FATAL_ERROR "Cannot determine system architecture." - " It seems the system has system arch set to empty or invalid string." - " Consult os-release file." + MESSAGE(FATAL_ERROR "Cannot determine system architecture." + " It seems the system has system arch set to empty or invalid string." + " Please consult 'os-release' file."
467-475: LGTM! Clean replacement of lsb_release dependency.The implementation follows the same pattern as
_CMDEF_ENV_GET_ARCH, maintaining consistency in the codebase.Fix the error message formatting for consistency with other messages:
- MESSAGE(FATAL_ERROR "Cannot determine Distro ID." - "It seems the system has Distro ID set to empty or invalid string." - " Consult os-release file." + MESSAGE(FATAL_ERROR "Cannot determine Distro ID." + " It seems the system has Distro ID set to empty or invalid string." + " Please consult 'os-release' file."
502-510: LGTM! Clean replacement of lsb_release dependency.The implementation maintains consistency with the other system information retrieval functions.
Fix the error message formatting for consistency:
- MESSAGE(FATAL_ERROR "Cannot determine Distro Version ID." - " It seems the system has Distro Version ID set to empty or invalid string." - " Consult os-release file." + MESSAGE(FATAL_ERROR "Cannot determine Distro Version ID." + " It seems the system has Distro Version ID set to empty or invalid string." + " Please consult 'os-release' file."
Line range hint
432-510: Consider centralizing error messages for better maintainability.The three functions have similar error messages. Consider defining these at the top of the file as variables for better maintainability and consistency.
Example implementation:
# At the top of the file SET(_CMDEF_ENV_ERROR_MESSAGE_PREFIX "Cannot determine") SET(_CMDEF_ENV_ERROR_MESSAGE_SUFFIX "Please consult 'os-release' file.") # In the functions MESSAGE(FATAL_ERROR "${_CMDEF_ENV_ERROR_MESSAGE_PREFIX} system architecture." " It seems the system has system arch set to empty or invalid string." " ${_CMDEF_ENV_ERROR_MESSAGE_SUFFIX}")
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
system_modules/CMDEF_ENV.cmake (3)
431-439: Add documentation for the string normalization process.The architecture detection logic looks good, but could benefit from a comment explaining the string normalization steps (regex replacement and lowercase conversion) for better maintainability.
Add this comment before line 432:
+ # Normalize architecture string: + # 1. Replace non-alphanumeric chars with hyphens + # 2. Convert to lowercase for consistency
466-474: Extract common string normalization logic.The string normalization and error handling logic is duplicated across functions. Consider extracting it into a helper function for better maintainability.
Create a new helper function:
## Helper # # Normalize system information string and validate result # # <function>( # <output_var_name> # Output variable name # <input_string> # Input string to normalize # <info_type> # Type of info (for error message) # ) # FUNCTION(_CMDEF_ENV_NORMALIZE_SYSTEM_INFO output input info_type) STRING(REGEX REPLACE "[^a-zA-Z0-9.]" "-" _mapped "${input}") STRING(TOLOWER "${_mapped}" _normalized) IF(NOT _normalized) MESSAGE(FATAL_ERROR "Cannot determine ${info_type}." " It seems the system has ${info_type} set to empty or invalid string." " Consult os-release file." ) ENDIF() SET(${output} "${_normalized}" PARENT_SCOPE) ENDFUNCTION()Then update the functions to use it:
FUNCTION(_CMDEF_ENV_GET_DISTRO_ID distro_id) # ... existing OS checks ... IF(CMDEF_OS_LINUX) CMAKE_HOST_SYSTEM_INFORMATION(RESULT _distro_id QUERY DISTRIB_ID) - STRING(REGEX REPLACE "[^a-zA-Z0-9.]" "-" _distro_id_mapped "${_distro_id}") - STRING(TOLOWER "${_distro_id_mapped}" _distro_id_normalized) - IF(NOT _distro_id_normalized) - MESSAGE(FATAL_ERROR "Cannot determine Distro ID." - "It seems the system has Distro ID set to empty or invalid string." - " Consult os-release file." - ) - ENDIF() - SET(${distro_id} "${_distro_id_normalized}" PARENT_SCOPE) + _CMDEF_ENV_NORMALIZE_SYSTEM_INFO(${distro_id} "${_distro_id}" "Distro ID") RETURN() ENDIF() # ... existing error message ... ENDFUNCTION()
505-508: Fix error message formatting inconsistency.The error message has inconsistent spacing in the concatenation.
- MESSAGE(FATAL_ERROR "Cannot determine Distro Version ID." - " It seems the system has Distro Version ID set to empty or invalid string." + MESSAGE(FATAL_ERROR "Cannot determine Distro Version ID. " + "It seems the system has Distro Version ID set to empty or invalid string."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
FindCMDEF.cmake(2 hunks)system_modules/CMDEF_ENV.cmake(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- FindCMDEF.cmake
🔇 Additional comments (2)
system_modules/CMDEF_ENV.cmake (2)
501-509: Use the suggested helper function here as well.This function has the same string normalization and validation logic as the other functions. It would benefit from using the suggested
_CMDEF_ENV_NORMALIZE_SYSTEM_INFOhelper function.
Line range hint
431-509: Overall changes look good! 👍The changes successfully replace external command dependencies with CMake's built-in
CMAKE_HOST_SYSTEM_INFORMATIONcommand, making the code more portable and reliable. The implementation includes proper error handling and string normalization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
example/executable/CMakeLists.txt (1)
99-106: Consider moving dump_cmake_variables to a debug utilities module.The
dump_cmake_variablesfunction is a useful debugging tool. Consider moving it to a separate debug utilities module (e.g., CMDEF_DEBUG.cmake) to make it reusable across different CMake files.doc/CacheVariables.md (1)
34-37: Add a comma after the introductory phrase.For better readability, add a comma after "In this case":
-In this case the variable is set to ON. +In this case, the variable is set to ON.🧰 Tools
🪛 LanguageTool
[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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
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 (7)
- system_modules/CMDEF_RESOURCE.cmake
- example/library-interface/main.cpp
- system_modules/CMDEF_COMPILE_OPTION.cmake
- example/library/CMakeLists.txt
- system_modules/CMDEF_BUILD_TYPE.cmake
- system_modules/CMDEF_LINK_OPTION.cmake
- example/library-interface/CMakeLists.txt
🧰 Additional context used
🪛 LanguageTool
doc/CacheVariables.md
[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 (4)
system_modules/CMDEF_ENV.cmake (3)
431-439: LGTM! Improved system architecture detection.The switch to
CMAKE_HOST_SYSTEM_INFORMATIONfor architecture detection is a good improvement that removes the external dependency onuname. The added validation for empty/invalid architecture strings enhances error handling.
466-474: LGTM! Improved distribution ID detection.The use of
CMAKE_HOST_SYSTEM_INFORMATIONfor distribution ID detection is consistent with the architecture detection changes. The validation and error handling are well-implemented.
501-509: LGTM! Improved distribution version detection.The implementation follows the same pattern as architecture and distribution ID detection, providing consistent error handling and validation.
system_modules/CMDEF_COMPILE_DEFINITION.cmake (1)
94-94: Breaking Change: VISIBILITY parameter is now required.The VISIBILITY parameter has been made mandatory in CMDEF_COMPILE_DEFINITIONS_TARGET. While this enforces better practice by requiring explicit visibility specification, it may break existing code that doesn't provide this parameter.
Run this script to find potentially affected code:
Also applies to: 104-104, 106-106
Summary by CodeRabbit
Chores
Bug Fixes