Update install-cmake powershell script #3142
Conversation
WalkthroughThe pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Script as install-cmake.ps1
participant CMake as Installed CMake
User->>Script: Run install-cmake.ps1
Script->>CMake: Retrieve installed version
alt Installed version == expected version
Script->>Script: Log "Skipping install of CMake"
else Installed version != expected version
Script->>Script: Proceed with CMake installation
end
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
install-scripts/install-cmake.ps1(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (4)
install-scripts/install-cmake.ps1 (4)
4-4: LGTM: Comment clarification is appropriate.Updated comment now more accurately describes what the script does by specifying it installs the "expected" CMake version rather than just "CMake".
6-7: Updated version reflects the latest stable 3.x.x release.The version has been updated from 3.23.0 to 3.31.0, which aligns with the PR objective to restrict installation to versions less than 4.x.x.
13-18: Logical change to prevent installation of 4.x.x versions.The condition now correctly skips installation if the installed version is less than or equal to the expected version (3.31.0), which prevents installation of potentially problematic 4.x.x versions. Any installed version above 3.31.0 will be replaced with 3.31.0.
15-15: Fixed typo in the message.Corrected "instal" to "install" in the message.
|
Although, in the longer term, we should also note https://cmake.org/cmake/help/latest/release/4.0.html - #3143 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
install-scripts/install-cmake.ps1 (1)
13-13: Logic change inverts the version check condition.The change from
-ge(greater than or equal) to-le(less than or equal) is significant. This now ensures that the script skips installation if the installed version is less than or equal to the expected version, effectively preventing installation of newer versions (like 4.x.x).While this change addresses the immediate need to prevent CMake 4.x.x installations, consider if a more explicit version check might be clearer:
- if($cmake.Version -le $version) + if($cmake.Version -le $version -and $cmake.Version.Major -lt 4)This would make the intention to avoid version 4.x.x more explicit in the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
install-scripts/install-cmake.ps1(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (3)
install-scripts/install-cmake.ps1 (3)
4-7: Comment updates clearly reflect the script's purpose.The updated comments now accurately describe the script's purpose and version expectations, making it clearer to users that this script installs a specific expected version of CMake rather than any version.
15-15: Fixed typo in message.The correction from "instal" to "install" improves the message clarity.
6-7: Version update to 3.31.6 is appropriate.The updated version number (3.31.6) is still within the 3.x.x range, which aligns with the PR objective to avoid 4.x.x versions. This update provides users with a more recent compatible version.
instead or le or ge to ensure exact version.
|
Holding off the merge until we have a new code sign cert to update the signature. |
I am guessing this is un-blocked now... |
|
Correct. Script signed. |
Description
The original script expected any cmake version above 3.23.0 but there are issues with cmake versions 4.x.x (in our environment).
This change ensures that the version is set to the expected
3.31.6(latest) version (when installed in a local environment).Motivation and Context
Fix and pin install of cmake version install in a local environment.
We are yet to handle the major release changes: https://cmake.org/cmake/help/latest/release/4.0.html
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
Bug Fixes
Chores