build(core): Lock CMake version to < 4 across all platforms to avoid third-party library compilation issues. #796
Conversation
WalkthroughThis pull request updates several installation scripts to include a new check for the CMake version by invoking a newly added script, Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 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 (
|
Bill-hbrhbr
changed the title
<4.0.0 across all platforms to avoid third-party library compilation issues.
![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 (3)
components/core/tools/scripts/lib_install/check-cmake.sh (2)
1-4: Shebang and Error Settings
The script correctly specifies the Bash interpreter and usesset -eto exit on errors. For enhanced robustness, consider addingset -o pipefailso that failures in any part of a pipeline are also caught.
10-14: Version Compatibility Check
The script checks if the major version is 4 or higher and exits with an appropriate error message. As a minor improvement, consider redirecting the error message to standard error to better signal issues. For example:- echo "CMake version $cmake_version is not supported (>= 4.0)." + echo "CMake version $cmake_version is not supported (>= 4.0)." >&2components/core/tools/scripts/lib_install/macos/install-all.sh (1)
11-15: Locking CMake Version Installation on macOS
This segment downloads a specific Homebrew formula for CMake v3.31.6 to avoid compatibility issues with yaml-cpp. This is an effective short-term solution. For better maintainability, consider defining the CMake version (e.g.CMAKE_VERSION=3.31.6) at the top of the script so that updates can be managed more easily.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/core/tools/scripts/lib_install/centos-stream-9/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/check-cmake.sh(1 hunks)components/core/tools/scripts/lib_install/macos/install-all.sh(2 hunks)components/core/tools/scripts/lib_install/ubuntu-focal/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-jammy/install-all.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (5)
components/core/tools/scripts/lib_install/check-cmake.sh (1)
6-8: CMake Version Extraction
The extraction of the CMake version usingawkandcutis straightforward and should work as intended. No further changes are required here.components/core/tools/scripts/lib_install/centos-stream-9/install-all.sh (1)
13-15: Integration of CMake Version Check
The addition of the line invoking${script_dir}/../check-cmake.shensures that any installed CMake version is validated before proceeding. Please confirm that thecheck-cmake.shscript is executable and in the expected location relative to this script.components/core/tools/scripts/lib_install/ubuntu-focal/install-all.sh (1)
14-15: Enforcing Supported CMake Version
By calling${script_dir}/../check-cmake.sh, the script now ensures that an unsupported CMake version will trigger an error. The inclusion of a TODO comment (referencing the GitHub issue) is noted; please update or remove it once the issue is resolved.components/core/tools/scripts/lib_install/ubuntu-jammy/install-all.sh (1)
14-15: CMake Version Validation Added
The new script invocation for${script_dir}/../check-cmake.shguarantees a consistent CMake version check across platforms. Ensure that the relative path remains correct as the project evolves.components/core/tools/scripts/lib_install/macos/install-all.sh (1)
41-43: Post-Installation CMake Version Check
The added invocation of${script_dir}/../check-cmake.shat the end of the script ensures that the installed CMake version is verified, maintaining consistency with other platform scripts.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com> Co-authored-by: Junhao Liao <junhao@junhao.ca>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/core/tools/scripts/lib_install/check-cmake.sh (2)
10-11: Version Extraction and Unused Variable:
The script correctly extracts the CMake version usingcmake --version. However, the variablecmake_major_version(line 11) is assigned but not used later. If the intention is to check the major version numerically, consider using it—for example:if ! [[ "$cmake_major_version" -lt 4 ]]; thenAlternatively, remove it if it’s unnecessary.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 11-11: cmake_major_version appears unused. Verify use (or export if used externally).
(SC2034)
15-18: Version Comparison Logic:
The conditional uses a string comparison ([[ "$cmake_version" < "4" ]]), which leverages the ASCII-order properties of semantic versions. For improved clarity—especially if only the major version matters—it might be preferable to compare the integer value stored incmake_major_version.components/core/tools/scripts/lib_install/macos/install-all.sh (2)
11-16: CMake Installation via Custom Formula:
The script downloads a specific CMake formula and installs CMake v3.31.6 to maintain compatibility with ANTLR and yaml-cpp. As a good-to-have enhancement, consider adding a cleanup step to remove the temporary file (/tmp/cmake.rb) after installation to avoid leaving residual files.
43-45: TODO and Script Invocation:
The new TODO (line 43) referencing GitHub issue #795 is noted—please ensure it is tracked for resolution. The use ofscript_dirto locate and invokecheck-cmake.shis well executed and enhances script portability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/core/tools/scripts/lib_install/check-cmake.sh(1 hunks)components/core/tools/scripts/lib_install/macos/install-all.sh(2 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
components/core/tools/scripts/lib_install/check-cmake.sh
[warning] 11-11: cmake_major_version appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-13, false)
🔇 Additional comments (1)
components/core/tools/scripts/lib_install/check-cmake.sh (1)
1-1: Filename Suggestion:
Consider renaming the script tocheck-cmake-version.shfor enhanced clarity since it explicitly checks the CMake version.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/core/tools/scripts/lib_install/check-cmake-version.sh (2)
3-7: Good Use of Error-Handling Flags.
The script appropriately usesset -eandset -uto exit on errors and to catch undefined variables. For enhanced robustness, consider addingset -o pipefailto ensure that failures in any part of a pipeline are detected.
9-11: Enhance JSON Parsing with jq.
Usingjqwithout the-rflag outputs JSON strings with quotes. It is recommended to usejq -rto obtain raw output, which will yield cleaner version numbers. For example:-cmake_version=$(cmake -E capabilities | jq ".version.string") -cmake_major_version=$(cmake -E capabilities | jq ".version.major") +cmake_version=$(cmake -E capabilities | jq -r ".version.string") +cmake_major_version=$(cmake -E capabilities | jq -r ".version.major")This change makes subsequent comparisons and messaging more reliable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/core/tools/scripts/lib_install/centos-stream-9/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/check-cmake-version.sh(1 hunks)components/core/tools/scripts/lib_install/macos/install-all.sh(2 hunks)components/core/tools/scripts/lib_install/ubuntu-focal/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-jammy/install-all.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- components/core/tools/scripts/lib_install/centos-stream-9/install-all.sh
- components/core/tools/scripts/lib_install/ubuntu-jammy/install-all.sh
- components/core/tools/scripts/lib_install/macos/install-all.sh
- components/core/tools/scripts/lib_install/ubuntu-focal/install-all.sh
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (1)
components/core/tools/scripts/lib_install/check-cmake-version.sh (1)
1-2: Proper Shebang Declaration.
The script begins with a correct shebang using/usr/bin/env bash, which ensures portability across systems.
components/core/tools/scripts/lib_install/check-cmake-version.sh
Outdated
Show resolved
Hide resolved
components/core/tools/scripts/lib_install/check-cmake-version.sh
Outdated
Show resolved
Hide resolved
How about
|
Bill-hbrhbr
changed the title
<4.0.0 across all platforms to avoid third-party library compilation issues.
3 participants
Description
Currently, multiple libraries that we depend on have the following specification:
where
3.x < 3.5. This raises a CMake deprecation error if we use the newest CMake version 4.0.0 (release note), which is now installed by default if we runbrew install cmakeon macos-15.Since these third-party dependencies may take some time to upgrade their CMake configurations, we are temporarily locking the CMake version to be below
4.0.0to avoid compatibility issues with the newer release.This PR should be reverted once we resolve issue #795 with PR #794.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Chores