added check compiler version#45
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new CMake compiler version validation function and updates the platform architecture detection to remove armv7l support. A new 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
buildscripts/cmake/GetPlatformInfo.cmake (1)
91-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider using FATAL_ERROR for unsupported architectures.
The current fallback to x86_64 for unknown architectures (e.g., 32-bit ARM after removing armv7l support) may cause confusing build failures downstream when x86_64-specific binaries or assumptions are used on incompatible systems. Since line 43 explicitly states only x86_64 and aarch64 are supported, consider using
FATAL_ERRORfor detected-but-unsupported architectures instead of a silent fallback.🚫 Proposed fix to fail explicitly for unsupported architectures
else() - set(ARCH_IS_X86_64 1) - message(WARNING "Architecture could not be detected. Using x86_64 as a fallback.") + message(FATAL_ERROR "Unsupported architecture: ${ARCH}. Only x86_64 and aarch64 are supported.") endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@buildscripts/cmake/GetPlatformInfo.cmake` around lines 91 - 93, The fallback that sets ARCH_IS_X86_64 and emits message(WARNING "...Using x86_64 as a fallback.") should be changed to fail fast for unknown/unsupported architectures: remove the implicit ARCH_IS_X86_64 assignment and replace the warning with a fatal error (message(FATAL_ERROR ...)) that includes the detected architecture and the supported list; target the branch that currently contains ARCH_IS_X86_64 and message(WARNING ...) so callers relying on ARCH_IS_X86_64 aren’t misled and CI/builds fail immediately when CMAKE_SYSTEM_PROCESSOR is unsupported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@buildscripts/cmake/CheckCompilerVersion.cmake`:
- Around line 4-5: The conditionals in CheckCompilerVersion.cmake use unquoted
variables (CMAKE_CXX_COMPILER_ID, ${name}, CMAKE_CXX_COMPILER_VERSION,
${min_version}); update the if() checks to quote the variable expansions so
empty or special-character values are handled safely (e.g., use quoted ${name}
and ${min_version} in the comparisons and quote
CMAKE_CXX_COMPILER_ID/CMAKE_CXX_COMPILER_VERSION where they appear). Ensure both
occurrences where CMAKE_CXX_COMPILER_ID STREQUAL ${name} and
CMAKE_CXX_COMPILER_VERSION VERSION_LESS ${min_version} are changed to use quoted
variable forms.
---
Outside diff comments:
In `@buildscripts/cmake/GetPlatformInfo.cmake`:
- Around line 91-93: The fallback that sets ARCH_IS_X86_64 and emits
message(WARNING "...Using x86_64 as a fallback.") should be changed to fail fast
for unknown/unsupported architectures: remove the implicit ARCH_IS_X86_64
assignment and replace the warning with a fatal error (message(FATAL_ERROR ...))
that includes the detected architecture and the supported list; target the
branch that currently contains ARCH_IS_X86_64 and message(WARNING ...) so
callers relying on ARCH_IS_X86_64 aren’t misled and CI/builds fail immediately
when CMAKE_SYSTEM_PROCESSOR is unsupported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: faca8663-5c7e-4cf3-aa2c-5a3132d2d48d
📒 Files selected for processing (2)
buildscripts/cmake/CheckCompilerVersion.cmakebuildscripts/cmake/GetPlatformInfo.cmake
| if (CMAKE_CXX_COMPILER_ID STREQUAL ${name}) | ||
| if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS ${min_version}) |
There was a problem hiding this comment.
Quote variables in conditionals for safety.
In CMake conditionals, it's a best practice to quote variables to handle edge cases with empty values or special characters safely.
🛡️ Proposed fix to add quotes
- if (CMAKE_CXX_COMPILER_ID STREQUAL ${name})
- if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS ${min_version})
+ if (CMAKE_CXX_COMPILER_ID STREQUAL "${name}")
+ if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS "${min_version}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@buildscripts/cmake/CheckCompilerVersion.cmake` around lines 4 - 5, The
conditionals in CheckCompilerVersion.cmake use unquoted variables
(CMAKE_CXX_COMPILER_ID, ${name}, CMAKE_CXX_COMPILER_VERSION, ${min_version});
update the if() checks to quote the variable expansions so empty or
special-character values are handled safely (e.g., use quoted ${name} and
${min_version} in the comparisons and quote
CMAKE_CXX_COMPILER_ID/CMAKE_CXX_COMPILER_VERSION where they appear). Ensure both
occurrences where CMAKE_CXX_COMPILER_ID STREQUAL ${name} and
CMAKE_CXX_COMPILER_VERSION VERSION_LESS ${min_version} are changed to use quoted
variable forms.
2 participants
No description provided.