add llvm 21.1.0 #8399
add llvm 21.1.0 #8399
Conversation
Summary of ChangesHello @waruqi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the latest LLVM version, 21.1.0, into the package management system. This update ensures that users can leverage the newest features and improvements offered by LLVM across different operating systems. Additionally, a minor adjustment was made to the macOS platform detection logic to streamline the installation process. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for LLVM version 21.1.0. The changes correctly add the new version for various platforms and build types. I've also noticed a change that enables source builds on all macOS architectures, which is a good improvement.
My main feedback is about the significant code duplication in packages/l/llvm/xmake.lua. The new version information is added in six different places. This is due to the file structure having separate logic for modern and legacy xmake versions. While this PR continues an existing pattern, it exacerbates a maintainability issue. I've left specific comments on the duplicated lines with suggestions to refactor this in the future to make the package script easier to manage.
| package:add("versions", "16.0.6", "5e1f560f75e7a4c7a6509cf7d9a28b4543e7afcb4bcf4f747e9f208f0efa6818") | ||
| package:add("versions", "17.0.6", "ce78b510603cb3b347788d2f52978e971cf5f55559151ca13a73fd400ad80c41") | ||
| package:add("versions", "18.1.1", "1e21b088b1f86aebb4a2e4ad473d1892dccab53ecbe06947f31c6fa56a078bf5") | ||
| package:add("versions", "21.1.0", "36b9a55e237b2db404aa621aacb8538b56dabc6f49b8927dc1109e8123524d5f") |
There was a problem hiding this comment.
This line adds the new version for Windows x86. However, the same version and hash are also added on line 76 for older xmake versions. This duplication makes maintenance harder and the script more error-prone. It would be better to define versions in a shared data structure (like a table) and populate them in both code paths to avoid redundancy.
| package:add("versions", "16.0.6", "7adb1a630b6cc676a4b983aca9b01e67f770556c6e960e9ee9aa7752c8beb8a3") | ||
| package:add("versions", "17.0.6", "c480a4c280234b91f7796a1b73b18134ae62fe7c88d2d0c33312d33cb2999187") | ||
| package:add("versions", "18.1.1", "7040c7a02529bc0c683896d4f851138b700d8aa8f40c5f48503b10f4cc2dc180") | ||
| package:add("versions", "21.1.0", "130d0067de849be36c0ec84c6d515bd310cab324a4cc95d8cc71a1d3c6c730f4") |
There was a problem hiding this comment.
Similar to the win32 case, this line adds the new version for Windows x64, which is duplicated on line 86. This redundancy increases the maintenance burden and risk of inconsistencies. Consolidating version definitions would improve the script's maintainability.
| package:add("versions", "16.0.6", "ce5e71081d17ce9e86d7cbcfa28c4b04b9300f8fb7e78422b1feb6bc52c3028e") | ||
| package:add("versions", "17.0.6", "58a8818c60e6627064f312dbf46c02d9949956558340938b71cf731ad8bc0813") | ||
| package:add("versions", "18.1.1", "8f34c6206be84b186b4b31f47e1b52758fa38348565953fad453d177ef34c0ad") | ||
| package:add("versions", "21.1.0", "1672e3efb4c2affd62dbbe12ea898b28a451416c7d95c1bd0190c26cbe878825") |
There was a problem hiding this comment.
This line adds the new source version, which is also duplicated on line 99. To avoid maintenance issues, it's recommended to define all versions in a single, shared data structure and reference it from both the modern on_source block and the legacy else block.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMoves Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as test.lua
participant L as line input
participant P as packages list
Note over T: process each modified line
L->>T: read line (package / +version)
alt lastEntry tokens > 1
T->>P: append new entry with version
Note right of P: new package element created
else lastEntry tokens == 1
T->>P: update last entry by attaching version
Note right of P: existing element modified
end
T->>P: continue processing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
🔇 Additional comments (4)
Comment |
waruqi
force-pushed
the
llvm
branch
2 times, most recently
from
73fb095 to
10e39cd
Compare
![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
♻️ Duplicate comments (1)
packages/l/llvm/xmake.lua (1)
38-38: Version additions are correct; duplication is a known concern.The new LLVM 21.1.0 version entries follow the established pattern and include proper checksums. The duplication between modern (
on_source) and legacy blocks has been noted in previous reviews as a maintenance concern.Also applies to: 48-48, 61-61, 76-76, 86-86, 99-99
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/l/llvm/xmake.lua(8 hunks)scripts/test.lua(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (ubuntu-latest, shared)
- GitHub Check: build (windows-2025, shared, x86, MT)
- GitHub Check: build (windows-2025, static, arm64, MT)
- GitHub Check: build (windows-2025, shared, arm64, MD)
- GitHub Check: build (windows-2025, static, x64, MD)
- GitHub Check: build (ubuntu-latest, static)
- GitHub Check: build (ubuntu-latest, shared)
- GitHub Check: build (macos-15-intel, x86_64, shared)
- GitHub Check: build (windows-11-arm, shared, arm64, MD)
- GitHub Check: build (ubuntu-latest, shared, release)
- GitHub Check: build (ubuntu-latest, static, release)
- GitHub Check: build (MINGW32, shared, i686, /mingw32)
- GitHub Check: build (MINGW64, static, x86_64, /mingw64)
- GitHub Check: build (macos-14, arm64, static)
- GitHub Check: build (windows-latest, armeabi-v7a, r27, 30)
- GitHub Check: build (ubuntu-latest, static)
- GitHub Check: build (ubuntu-24.04-arm, static, debug)
- GitHub Check: build (ubuntu-24.04-arm, static, release)
- GitHub Check: build (ubuntu-24.04-arm, shared, debug)
- GitHub Check: build (ubuntu-24.04-arm, shared, release)
🔇 Additional comments (2)
scripts/test.lua (1)
283-287: LGTM! Correct handling of multiple version additions.The conditional logic correctly handles the case where a package might already have a version string in the list. By checking token count, it prevents overwriting existing package+version entries when multiple versions are added in the same diff.
packages/l/llvm/xmake.lua (1)
135-135: LGTM! Expands macOS support to all architectures.Removing the
|x86_64restriction enables LLVM installation on all macOS architectures, including Apple Silicon (ARM64). This is a beneficial change for broader platform support.
Summary by CodeRabbit
New Features
Build Improvements
Bug Fixes