Add zig support #11
Add zig support #11
Conversation
WalkthroughCI workflows updated to xmake 3.0.5 and now branch on Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as GH Actions runner
participant Matrix as Job matrix (lto)
participant Step as bash step
participant Xrepo as xrepo env
participant Xmake as xmake config
participant Tool as Toolchain (Zig / Clang)
Runner->>Matrix: start job with matrix.lto
Matrix->>Step: run Package step (shell: bash)
alt matrix.lto == "y"
Step->>Xrepo: xrepo env bind zig 0.15.2
Xrepo->>Xmake: invoke xmake config (zig toolchain, cross-target, preserve mode/LTO)
Xmake->>Tool: use Zig toolchain
else matrix.lto != "y"
Step->>Xmake: invoke xmake config (clang-20, preserve mode/LTO)
Xmake->>Tool: use Clang-20 toolchain
end
Note right of Xmake: subsequent build/package steps use configured toolchain
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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
🧹 Nitpick comments (2)
.github/workflows/linux.yml (1)
37-41: Linux Zig/clang split for LTO looks correct; consider DRYing Zig version
shell: bashplusif [[ "${{ matrix.lto }}" == "y" ]]is valid and ensures Zig is only used for LTO builds.- Using
xrepo env --bind="zig 0.15.2"to drivexmake config --toolchain=zigis consistent with the Zig‑based toolchain setup, while the non‑LTO path stays onclang-20.For maintainability, you might factor the Zig version (
0.15.2) into a reusable matrix value or env var shared with the macOS workflow so it’s easier to bump in one place later.Also applies to: 61-68
.github/workflows/macos.yml (1)
42-51: macOS Zig/clang branching for LTO is correct; Zig version could be centralized
- Using
shell: bashwithif [[ "${{ matrix.lto }}" == "y" ]]is appropriate.- The Zig path (
xrepo env --bind="zig 0.15.2" ... --toolchain=zig) cleanly mirrors the Linux workflow, and the else branch preserves the existing clang‑based config behavior.Similar to Linux, you might consider pulling the Zig version (
0.15.2) into a shared matrix value or env var so Linux and macOS don’t drift if you upgrade Zig later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/linux.yml(2 hunks).github/workflows/macos.yml(1 hunks).github/workflows/windows.yml(1 hunks)xmake.lua(2 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). (8)
- GitHub Check: build (releasedbg, n)
- GitHub Check: build (releasedbg, y)
- GitHub Check: build (debug, n)
- GitHub Check: build (releasedbg, y)
- GitHub Check: build (releasedbg, n)
- GitHub Check: build (debug, n)
- GitHub Check: build (releasedbg, MD, n)
- GitHub Check: build (releasedbg, MT, y)
🔇 Additional comments (1)
.github/workflows/windows.yml (1)
28-32: Windows xmake 3.0.5 bump is consistent with the rest of the repoUpdating
xmake-versionto3.0.5here matches thecompatibility.version = "3.0"policy inxmake.luaand the other CI workflows. No issues from this change alone.
| add_versions("git:21.1.4", "llvmorg-21.1.4") | ||
| add_versions("git:20.1.5", "llvmorg-20.1.5") | ||
|
|
There was a problem hiding this comment.
Guard LLVM_HOST_TRIPLE insertion to avoid nil concat when Zig is used on other platforms
Switching the 21.1.4 llvm version to the llvmorg-21.1.4 tag is fine and keeps the version mapping readable.
In the Zig‑specific block, though:
if package:toolchain("zig") then
local target
if package:is_plat("linux") then
target = "x86_64-linux-gnu"
elseif package:is_plat("macosx") then
target = "aarch64-macos-none"
end
table.insert(configs, "-DLLVM_HOST_TRIPLE=" .. target)
endIf someone ever configures the Zig toolchain on a platform other than linux or macosx, target will be nil and the concatenation will raise a runtime error. It’s safer to only append the flag when target is defined.
A minimal fix:
- if package:toolchain("zig") then
- local target
- if package:is_plat("linux") then
- target = "x86_64-linux-gnu"
- elseif package:is_plat("macosx") then
- target = "aarch64-macos-none"
- end
- table.insert(configs, "-DLLVM_HOST_TRIPLE=" .. target)
- end
+ if package:toolchain("zig") then
+ local target
+ if package:is_plat("linux") then
+ target = "x86_64-linux-gnu"
+ elseif package:is_plat("macosx") then
+ target = "aarch64-macos-none"
+ end
+ if target then
+ table.insert(configs, "-DLLVM_HOST_TRIPLE=" .. target)
+ end
+ endThis keeps current Linux/macOS behavior while avoiding a hard failure if Zig support is later enabled on another platform before adding the corresponding triple.
Also applies to: 132-140
🤖 Prompt for AI Agents
In xmake.lua around lines 29-31 (and similarly at lines 132-140), the
Zig-specific block unconditionally concatenates "-DLLVM_HOST_TRIPLE=" with a
local target variable that may remain nil on unsupported platforms, causing a
runtime concat error; update the code to only insert the
"-DLLVM_HOST_TRIPLE=..." config when target is non-nil (i.e., after computing
target, check if target then table.insert the flag), leaving existing behavior
for linux and macosx intact and avoiding insertion on other platforms.
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 (3)
.github/workflows/linux.yml(2 hunks).github/workflows/macos.yml(2 hunks)xmake.lua(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- xmake.lua
⏰ 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). (8)
- GitHub Check: build (debug, n)
- GitHub Check: build (releasedbg, y)
- GitHub Check: build (releasedbg, y)
- GitHub Check: build (debug, n)
- GitHub Check: build (releasedbg, n)
- GitHub Check: build (releasedbg, n)
- GitHub Check: build (releasedbg, MT, y)
- GitHub Check: build (releasedbg, MD, n)
🔇 Additional comments (3)
.github/workflows/linux.yml (2)
40-40: xmake version bump is straightforward.The version update from 2.9.9 to 3.0.5 is a valid upgrade. Verify that this version supports the new
--crossand conditional toolchain parameters introduced in the build steps below.
62-68: Verify cross-target glibc version compatibility.The
--cross=x86_64-linux-gnu.2.17parameter targets glibc 2.17, which is significantly older than Ubuntu 22.04's 2.35. This may restrict binary compatibility or cause runtime failures on newer systems.Verify whether:
- This glibc version is intentional for broader compatibility guarantees
- zig 0.15.2's cross-compilation toolchain requires or recommends this specific version
- The build artifacts are tested on systems with glibc 2.17 and newer
If broader compatibility is not a requirement, consider updating to a more recent glibc version (e.g., 2.29 or 2.35) to align with the runner environment.
.github/workflows/macos.yml (1)
25-30: Conditional llvm installation is logically sound.The setup correctly installs llvm@20 and lld@20 only when using the clang toolchain (lto == "n"). However, ensure the PATH export in the Package step accounts for this conditional installation (see next comment).
| - name: Package | ||
| shell: bash | ||
| run: | | ||
| export SDKROOT=$(xcrun --sdk macosx --show-sdk-path) | ||
| export PATH="/opt/homebrew/opt/llvm@20/bin:/opt/homebrew/opt/lld@20/bin:$PATH" | ||
| xmake config --yes --verbose --toolchain=clang --mode=${{ matrix.build_type }} --policies=build.optimization.lto:${{ matrix.lto }} | ||
| if [[ "${{ matrix.lto }}" == "y" ]]; then | ||
| xrepo env --verbose --yes --bind="zig 0.15.2" xmake config --yes --verbose --toolchain=zig --mode=${{ matrix.build_type }} --policies=build.optimization.lto:${{ matrix.lto }} | ||
| else | ||
| xmake config --yes --verbose --toolchain=clang --mode=${{ matrix.build_type }} --policies=build.optimization.lto:${{ matrix.lto }} | ||
| fi |
There was a problem hiding this comment.
Critical: PATH export references non-existent binaries for zig builds.
When matrix.lto == "y", the Setup llvm step skips installing llvm@20 and lld@20, but lines 48-49 unconditionally export these paths in the Package step. This breaks zig-based LTO builds.
The PATH export must be conditional to match the conditional installation:
Apply this diff to fix the issue:
- name: Package
shell: bash
run: |
export SDKROOT=$(xcrun --sdk macosx --show-sdk-path)
- export PATH="/opt/homebrew/opt/llvm@20/bin:/opt/homebrew/opt/lld@20/bin:$PATH"
if [[ "${{ matrix.lto }}" == "y" ]]; then
xrepo env --verbose --yes --bind="zig 0.15.2" xmake config --yes --verbose --toolchain=zig --mode=${{ matrix.build_type }} --policies=build.optimization.lto:${{ matrix.lto }}
else
+ export PATH="/opt/homebrew/opt/llvm@20/bin:/opt/homebrew/opt/lld@20/bin:$PATH"
xmake config --yes --verbose --toolchain=clang --mode=${{ matrix.build_type }} --policies=build.optimization.lto:${{ matrix.lto }}
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Package | |
| shell: bash | |
| run: | | |
| export SDKROOT=$(xcrun --sdk macosx --show-sdk-path) | |
| export PATH="/opt/homebrew/opt/llvm@20/bin:/opt/homebrew/opt/lld@20/bin:$PATH" | |
| xmake config --yes --verbose --toolchain=clang --mode=${{ matrix.build_type }} --policies=build.optimization.lto:${{ matrix.lto }} | |
| if [[ "${{ matrix.lto }}" == "y" ]]; then | |
| xrepo env --verbose --yes --bind="zig 0.15.2" xmake config --yes --verbose --toolchain=zig --mode=${{ matrix.build_type }} --policies=build.optimization.lto:${{ matrix.lto }} | |
| else | |
| xmake config --yes --verbose --toolchain=clang --mode=${{ matrix.build_type }} --policies=build.optimization.lto:${{ matrix.lto }} | |
| fi | |
| - name: Package | |
| shell: bash | |
| run: | | |
| export SDKROOT=$(xcrun --sdk macosx --show-sdk-path) | |
| if [[ "${{ matrix.lto }}" == "y" ]]; then | |
| xrepo env --verbose --yes --bind="zig 0.15.2" xmake config --yes --verbose --toolchain=zig --mode=${{ matrix.build_type }} --policies=build.optimization.lto:${{ matrix.lto }} | |
| else | |
| export PATH="/opt/homebrew/opt/llvm@20/bin:/opt/homebrew/opt/lld@20/bin:$PATH" | |
| xmake config --yes --verbose --toolchain=clang --mode=${{ matrix.build_type }} --policies=build.optimization.lto:${{ matrix.lto }} | |
| fi |
🤖 Prompt for AI Agents
In .github/workflows/macos.yml around lines 45 to 54 the Package step
unconditionally exports /opt/homebrew/opt/llvm@20 and /opt/homebrew/opt/lld@20
into PATH even when matrix.lto == "y" (zig-based LTO) which doesn't install
those packages; make the PATH export conditional to match the installation logic
by only exporting the llvm@20 and lld@20 paths when matrix.lto != "y" (or move
that export into the existing else branch), leaving SDKROOT exported
unconditionally.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
xmake.lua (1)
133-144: GuardLLVM_HOST_TRIPLEinsertion to avoid concat with nil on non-linux/macos Zig platformsIn this Zig-specific block:
if not package:is_plat("windows") and package:has_tool("cxx", "zig_cc") then local target if package:is_plat("linux") then target = "x86_64-linux-gnu" elseif package:is_plat("macosx") then target = "aarch64-macos-none" -- workaround opt.cxflags = "-flld" end table.insert(configs, "-DLLVM_HOST_TRIPLE=" .. target) endif someone enables the Zig toolchain on a non-
windowsplatform that is neitherlinuxnormacosx,targetstaysniland the concatenation will raise a runtime error.Only append
-DLLVM_HOST_TRIPLE=...whentargetwas actually computed:- if not package:is_plat("windows") and package:has_tool("cxx", "zig_cc") then - local target - if package:is_plat("linux") then - target = "x86_64-linux-gnu" - elseif package:is_plat("macosx") then - target = "aarch64-macos-none" - -- workaround - -- @see https://github.com/ziglang/zig/issues/18357#issuecomment-1869102870 - opt.cxflags = "-flld" - end - table.insert(configs, "-DLLVM_HOST_TRIPLE=" .. target) - end + if not package:is_plat("windows") and package:has_tool("cxx", "zig_cc") then + local target + if package:is_plat("linux") then + target = "x86_64-linux-gnu" + elseif package:is_plat("macosx") then + target = "aarch64-macos-none" + -- workaround + -- @see https://github.com/ziglang/zig/issues/18357#issuecomment-1869102870 + opt.cxflags = "-flld" + end + if target then + table.insert(configs, "-DLLVM_HOST_TRIPLE=" .. target) + end + endThis preserves current Linux/macOS behavior and avoids a hard failure if Zig is later enabled on another platform before adding the corresponding triple.
🧹 Nitpick comments (1)
xmake.lua (1)
76-187: Localoptfor CMake is fine; consider avoiding two differentoptlocalsDefining
local opt = {}here and using it foropt.targetand (conditionally)opt.cxflagspassed intocmake.installis correct. However, there is anotherlocal opt = {}later around line 231 for archive options, which shadows this name and may confuse future readers.If you want to tighten things up, consider renaming one of them (e.g.,
cmake_optvsarchive_opt) or reusing a singleopttable for both call sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
xmake.lua(3 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). (8)
- GitHub Check: build (debug, n)
- GitHub Check: build (releasedbg, n)
- GitHub Check: build (releasedbg, y)
- GitHub Check: build (releasedbg, y)
- GitHub Check: build (debug, n)
- GitHub Check: build (releasedbg, n)
- GitHub Check: build (releasedbg, MT, y)
- GitHub Check: build (releasedbg, MD, n)
🔇 Additional comments (1)
xmake.lua (1)
29-31: LLVM 21.1.4 git tag mapping looks goodUsing
add_versions("git:21.1.4", "llvmorg-21.1.4")aligns the git version with the upstream LLVM tag naming; no issues here.
Summary by CodeRabbit
Chores
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.