build: use pixi for managing build toolchains#322
Conversation
|
Warning Rate limit exceeded@16bit-ykiko has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request replaces Docker-based and legacy CMake LLVM provisioning with a Pixi-managed workspace, adds Python/Bash/PowerShell tooling to build, install, prune, and upload LLVM artifacts, refactors CMake to use a toolchain file and optional ThinLTO, and updates GitHub Actions to run LLVM builds and artifact workflows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant GH_Actions as "GitHub Actions (build-llvm)"
participant Pixi as "Pixi (setup)"
participant SetupScript as "scripts/setup-llvm.py"
participant BuildScript as "scripts/build-llvm.py"
participant CMake as "CMake / cmake/toolchain.cmake"
participant Artifacts as "Artifact Storage / gh"
Developer->>GH_Actions: push / PR triggers build-llvm
GH_Actions->>Pixi: run 'pixi setup' (install tools)
GH_Actions->>SetupScript: invoke setup-llvm (select/install artifact)
SetupScript-->>GH_Actions: returns install_path + cmake_dir
GH_Actions->>CMake: configure project with toolchain + llvm cmake dir
GH_Actions->>BuildScript: optionally invoke build-llvm to build llvm-project
BuildScript-->>GH_Actions: produce install artifacts (tar.xz)
GH_Actions->>Artifacts: upload artifacts & pruning manifests
GH_Actions->>Developer: report success/fail, upload logs/artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 (1)
pixi.toml (1)
20-27: Consider adding build tasks.The environment configuration is appropriate, with
uvcorrectly placed in the test feature. However, the empty[tasks]section could be populated with common development tasks to improve developer experience.Consider adding tasks like:
[tasks] configure = "cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCLICE_ENABLE_TEST=ON" build = { cmd = "cmake --build build", depends-on = ["configure"] } test = { cmd = "./build/bin/unit_tests --test-dir=./tests/data", depends-on = ["build"] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.gitattributes(1 hunks).github/workflows/cmake.yml(2 hunks).gitignore(1 hunks)CMakeLists.txt(2 hunks)cmake/package.cmake(1 hunks)pixi.toml(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). (3)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang, clang++)
🔇 Additional comments (11)
.gitignore (1)
64-65: LGTM!The Pixi environment ignore rules follow the standard pattern of ignoring generated artifacts while preserving configuration.
CMakeLists.txt (2)
6-6: LGTM!Disabling C++ module scanning is consistent with the
CPPTRACE_DISABLE_CXX_20_MODULESsetting incmake/package.cmakeand ensures compatibility with the Pixi environment.
53-53: LGTM!Static linking of the standard C++ library and GCC runtime improves binary portability by eliminating dependencies on system library versions. This aligns well with the pinned GCC 14.2.0 version in
pixi.toml.cmake/package.cmake (1)
138-138: LGTM!The
CPPTRACE_DISABLE_CXX_20_MODULEStoggle is correctly positioned beforeFetchContent_MakeAvailableand aligns with the global module scanning disablement inCMakeLists.txt..gitattributes (1)
1-2: LGTM!The
.gitattributesconfiguration forpixi.lockfollows best practices for lockfiles: binary merge strategy prevents merge conflicts, linguist attributes improve GitHub UI display, and-diffreduces noise in logs.pixi.toml (3)
1-6: LGTM!Workspace metadata is well-configured with appropriate platform coverage and the
conda-forgechannel.
15-18: LGTM! Consider the commented sysroot.The exact GCC version pinning (
==14.2.0) ensures reproducible builds and aligns with the static library linking inCMakeLists.txt. The commentedsysroot_linux-64line suggests glibc compatibility may have been considered—document if this will be needed in the future.
8-13: The Clang >=20.1.8 minimum version is appropriate and justified.The project explicitly requires C++23 (
CMAKE_CXX_STANDARD_REQUIRED ONin CMakeLists.txt) and uses C++23 features extensively throughout the codebase, includingstd::expectedandstd::println. Clang 20 is the first version with robust C++23 support; earlier versions had only partial support. The 20.1.8 specification is reasonable for a C++23 project..github/workflows/cmake.yml (3)
34-35: LGTM! Clang with GCC standard library.Using Clang as the compiler while having GCC as a dependency is correct. Clang will use GCC's
libstdc++, which is then statically linked as configured inCMakeLists.txt(line 53).
77-77: LGTM!The test execution correctly uses
uvfrom the Pixi environment to run pytest with appropriate configuration.
47-54: LGTM! Pixi configuration is solid.The
locked: trueoption ensures reproducible builds using the committedpixi.lockfile. The setup correctly activates thedevelopenvironment and caches dependencies for faster CI runs.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmake/package.cmake (2)
88-95: Non-reproducible dependency refs:libuvuses movingv1.x, andtomlplusplushas noGIT_TAG.
- Line 91 (
GIT_TAG v1.x) is a moving target.- Line 113-118 omits
GIT_TAGentirely (typically defaults to the repo’s default branch).Strongly recommend pinning both (and ideally all deps) to immutable tags or commit SHAs for deterministic CI/builds.
Also applies to: 113-118
144-152: Remove ineffective cache variable for cpptrace v1.0.4.The cache variable
CPPTRACE_DISABLE_CXX_20_MODULESdoes not exist in cpptrace v1.0.4. This line has no effect and should be removed entirely. If C++20 modules are a concern, the proper approach per cpptrace documentation is to use regular headers/includes instead of the module-based import.FetchContent_Declare( cpptrace GIT_REPOSITORY https://github.com/jeremy-rifkin/cpptrace.git GIT_TAG v1.0.4 GIT_SHALLOW TRUE GIT_PROGRESS TRUE ) -set(CPPTRACE_DISABLE_CXX_20_MODULES ON CACHE BOOL "" FORCE) FetchContent_MakeAvailable(libuv spdlog tomlplusplus croaring flatbuffers cpptrace).github/workflows/cmake.yml (1)
50-70: Usepixi runto invoke cmake and uv commands for reliable cross-platform execution.The current
activate-environment: trueapproach may fail silently on Windows+bash due to documented$GITHUB_ENV/$GITHUB_PATHbehavior inconsistencies and CRLF line-ending issues. Per Pixi best practices, wrap the build and test steps withpixi run bashto ensure cmake, Ninja, and uv always resolve from the Pixi environment:- name: Build clice run: pixi run bash -c "cmake -B build -G Ninja ... && cmake --build build" - name: Run tests run: pixi run bash -c "EXE_EXT=... && ./build/bin/unit_tests${EXE_EXT} ... && uv run ..."Alternatively, remove
activate-environment: trueand usepixi runfor each command that depends on Pixi-managed tools.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/cmake.yml(1 hunks)CMakeLists.txt(3 hunks)cmake/package.cmake(5 hunks)pixi.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CMakeLists.txt
- pixi.toml
⏰ 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). (2)
- GitHub Check: build (ubuntu-24.04, Debug)
- GitHub Check: build (windows-2025, RelWithDebInfo)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmake/package.cmake (1)
87-95:libuvuses moving tagv1.x: shallow clone won’t make this reproducible.
If you want repeatable CI/dev envs, prefer a fixed tag/commit (even withGIT_SHALLOW TRUE).
♻️ Duplicate comments (2)
.github/workflows/cmake.yml (1)
41-48: Pinprefix-dev/setup-pixiby commit SHA + verifyuvis available in the Pixi env.
You’re using a mutable action tag (@v0.9.3) and relying on Pixi activation to supply tools later (notablyuv). Consider pinning by SHA and ensureuvis part of thedevelopenvironment (or add an explicit step that installs/provides it).- uses: prefix-dev/setup-pixi@v0.9.3 + uses: prefix-dev/setup-pixi@82d477f15f3a381dbcc8adc1206ce643fe110fb7 # v0.9.3prefix-dev/setup-pixi v0.9.3 commit SHA (verify current recommended SHA and whether v0.9.3 tag is mutable)cmake/package.cmake (1)
79-80: Don’t forceFETCHCONTENT_UPDATES_DISCONNECTEDglobally without an opt-out.
This disables updates for all FetchContent deps in every context; please gate behindCLICE_CI_ENVIRONMENT/CLICE_DEV_ENVor provide an option.-set(FETCHCONTENT_UPDATES_DISCONNECTED ON) +option(CLICE_FETCHCONTENT_UPDATES_DISCONNECTED "Disable FetchContent updates (repro/CI)" OFF) +if(CLICE_CI_ENVIRONMENT OR CLICE_DEV_ENV OR CLICE_FETCHCONTENT_UPDATES_DISCONNECTED) + set(FETCHCONTENT_UPDATES_DISCONNECTED ON) +endif()
🧹 Nitpick comments (2)
cmake/package.cmake (1)
140-148: AvoidCACHE ... FORCEunless you really want to prevent overrides (cpptracemodules).
ForcingCPPTRACE_DISABLE_CXX_20_MODULES=ONblocks downstream/user overrides; consider making it conditional (e.g., only in CI/dev env) or defaulting withoutFORCE.CMakeLists.txt (1)
39-42: Prefer target-scoped compile definitions over mutatingCMAKE_CXX_FLAGS.
This is likely fine, but it’s cleaner/safer to attach_LIBCPP_DISABLE_AVAILABILITYtoclice_options(or viaadd_compile_definitions) instead of a global flags string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/cmake.yml(1 hunks)CMakeLists.txt(3 hunks)cmake/package.cmake(5 hunks)docker/.dockerignore(0 hunks)docker/Dockerfile(0 hunks)docker/linux/Dockerfile(0 hunks)docker/linux/build.sh(0 hunks)docker/linux/run.sh(0 hunks)pixi.toml(1 hunks)
💤 Files with no reviewable changes (5)
- docker/Dockerfile
- docker/linux/run.sh
- docker/linux/Dockerfile
- docker/.dockerignore
- docker/linux/build.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- pixi.toml
⏰ 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). (3)
- GitHub Check: build (macos-15, Debug)
- GitHub Check: build (windows-2025, RelWithDebInfo)
- GitHub Check: build (ubuntu-24.04, Debug)
🔇 Additional comments (3)
.github/workflows/cmake.yml (1)
53-57:-DCLICE_DEV_ENV=ONin CI: ensure this doesn’t override toolchains unexpectedly.
SinceCLICE_DEV_ENVforces compilers/linker flags inCMakeLists.txt, double-check CI matches the intended toolchain on all runners (esp. Windows/macOS) and doesn’t fight user-provided toolchain files.cmake/package.cmake (1)
103-110: Good: pinned dependency tags + shallow clones reduce CI time (mostly).
GIT_TAG+GIT_SHALLOW TRUEis a nice improvement for fetch performance; just keep an eye on any deps that still use moving refs.Also applies to: 111-118, 119-125, 129-135
CMakeLists.txt (1)
16-19: Disabling module scanning is consistent with the rest of the PR; verify no targets rely on C++20 modules.
set(CMAKE_CXX_SCAN_FOR_MODULES OFF)is fine if nothing needs automatic module discovery; just ensure future module adoption doesn’t get silently blocked.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
.github/workflows/llvm.yml (2)
77-81: Consider pinningllvm-projectto a specific commit SHA for reproducible CI.Even with
llvmorg-21.1.4, a SHA pin removes “tag moved” ambiguity. A lightweight approach is: clone thengit checkout <sha>(or useactions/checkoutagainst llvm-project at a SHA).
82-87: Add explicit-S .and job timeouts to reduce CI ambiguity/hangs.jobs: build: + timeout-minutes: 180 strategy: fail-fast: false @@ - name: Build clice using installed LLVM shell: bash run: | @@ - cmake -B build -G Ninja \ + cmake -S . -B build -G Ninja \ -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} \ -DCLICE_DEV_ENV=ON \ -DCLICE_ENABLE_TEST=ON \ -DCLICE_CI_ENVIRONMENT=ON \ -DLLVM_INSTALL_PATH="${LLVM_PREFIX}"Also applies to: 88-103
scripts/build-llvm.py (2)
9-36: Good CLI shape; consider normalizingargs.ltoto a boolean early.This simplifies later conditionals and avoids repeating
.lower()checks.args = parser.parse_args() + lto_enabled = args.lto.lower() in {"on", "true"} @@ - print(f"LTO: {args.lto}") + print(f"LTO: {'on' if lto_enabled else 'off'}")
218-235: Tightensubprocesscalls (address Ruff S603/S607 + avoid list concatenation).- subprocess.check_call( - ["cmake", "-S", str(source_dir), "-B", str(build_dir)] + cmake_args - ) + subprocess.run( + ["cmake", "-S", str(source_dir), "-B", str(build_dir), *cmake_args], + check=True, + ) @@ - subprocess.check_call( - ["cmake", "--build", str(build_dir), "--target", "install-distribution"] - ) + subprocess.run( + ["cmake", "--build", str(build_dir), "--target", "install-distribution"], + check=True, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/llvm.yml(1 hunks)scripts/build-llvm-libs.py(0 hunks)scripts/build-llvm.py(1 hunks)
💤 Files with no reviewable changes (1)
- scripts/build-llvm-libs.py
🧰 Additional context used
🪛 Ruff (0.14.8)
scripts/build-llvm.py
221-221: subprocess call: check for execution of untrusted input
(S603)
222-222: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
230-230: subprocess call: check for execution of untrusted input
(S603)
231-231: Starting a process with a partial executable path
(S607)
⏰ 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). (11)
- GitHub Check: build (macos-15, releasedbg, on)
- GitHub Check: build (windows-2025, releasedbg, on)
- GitHub Check: build (ubuntu-24.04, releasedbg, on)
- GitHub Check: build (macos-15, releasedbg, off)
- GitHub Check: build (ubuntu-24.04, releasedbg, off)
- GitHub Check: build (windows-2025, releasedbg, off)
- GitHub Check: build (ubuntu-24.04, debug, off)
- GitHub Check: build (macos-15, debug, off)
- GitHub Check: build (macos-15, Debug)
- GitHub Check: build (windows-2025, RelWithDebInfo)
- GitHub Check: build (ubuntu-24.04, Debug)
🔇 Additional comments (3)
.github/workflows/llvm.yml (2)
30-60: Double-checkwindows-2025runner label availability.If
windows-2025isn’t a valid GitHub-hosted runner label for this repo/org, the workflow will be permanently red. Consider falling back towindows-2022(or whatever you standardize on).- - os: windows-2025 + - os: windows-2022 llvm_mode: releasedbg lto: off - - os: windows-2025 + - os: windows-2022 llvm_mode: releasedbg lto: onAlso applies to: 62-62
68-76: No issues found.setup-pixi@v0.9.3andpixi-version: v0.59.0are current releases (Nov and Oct 2025 respectively) with no breaking changes affecting this workflow. The pinned versions are appropriate and well-supported.scripts/build-llvm.py (1)
195-215: Confirm Debug+ASan+shared-libs is intentional (and only used on supported platforms).Given the workflow only runs
debugon ubuntu/macos, it’s probably fine; just confirm this won’t surprise local devs runningmode=debugon other platforms.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
scripts/build-llvm.py (3)
49-147: Deduplicatellvm_distribution_componentsentries
clangStaticAnalyzerCore/clangStaticAnalyzerFrontendappear twice (Line 102-104 and Line 137-138).@@ "clangStaticAnalyzerCore", "clangStaticAnalyzerFrontend", @@ "clangTransformer", "clangCrossTU", - "clangStaticAnalyzerCore", - "clangStaticAnalyzerFrontend", "clangAnalysisFlowSensitive", "clangAnalysisFlowSensitiveModels",
150-198: Hardcoded clang/clang++ + lld flags will likely break Windows builds; add platform defaults + overrides
Right now-DCMAKE_C_COMPILER=clang,-DCMAKE_CXX_COMPILER=clang++,-fuse-ld=lld, and-DLLVM_USE_LINKER=lldare unconditional. On Windows MSVC-ABI builds you typically needclang-cl(and linker handling differs).@@ import sys import subprocess import shutil import argparse from pathlib import Path +import os @@ def main(): parser = argparse.ArgumentParser( description="Build LLVM with specific configurations." ) + parser.add_argument("--cc", default=os.environ.get("CC"), help="C compiler (overrides platform default)") + parser.add_argument("--cxx", default=os.environ.get("CXX"), help="C++ compiler (overrides platform default)") + parser.add_argument("--llvm-use-linker", default=os.environ.get("LLVM_USE_LINKER"), help="Value for -DLLVM_USE_LINKER=") @@ components_joined = ";".join(llvm_distribution_components) + + is_windows = (sys.platform == "win32") + default_cc = "clang-cl" if is_windows else "clang" + default_cxx = "clang-cl" if is_windows else "clang++" + cc = args.cc or default_cc + cxx = args.cxx or default_cxx + llvm_use_linker = args.llvm_use_linker or ("lld-link" if is_windows else "lld") + cmake_args = [ @@ "Ninja", f"-DCMAKE_INSTALL_PREFIX={install_prefix}", - "-DCMAKE_C_COMPILER=clang", - "-DCMAKE_CXX_COMPILER=clang++", + f"-DCMAKE_C_COMPILER={cc}", + f"-DCMAKE_CXX_COMPILER={cxx}", "-DCMAKE_C_FLAGS=-w", "-DCMAKE_CXX_FLAGS=-w", - "-DCMAKE_EXE_LINKER_FLAGS=-fuse-ld=lld", - "-DCMAKE_SHARED_LINKER_FLAGS=-fuse-ld=lld", - "-DCMAKE_MODULE_LINKER_FLAGS=-fuse-ld=lld", + # NOTE: -fuse-ld=lld is clang/clang++-style; avoid applying it on Windows clang-cl by default. + *([] if is_windows else [ + "-DCMAKE_EXE_LINKER_FLAGS=-fuse-ld=lld", + "-DCMAKE_SHARED_LINKER_FLAGS=-fuse-ld=lld", + "-DCMAKE_MODULE_LINKER_FLAGS=-fuse-ld=lld", + ]), @@ - "-DLLVM_USE_LINKER=lld", + f"-DLLVM_USE_LINKER={llvm_use_linker}", # Distribution f"-DLLVM_DISTRIBUTION_COMPONENTS={components_joined}", ]Suggested verification (so we don’t guess wrong for your CI target): confirm what your Windows CI intends to build (MSVC-ABI vs MinGW), and what LLVM/CMake expects for
LLVM_USE_LINKERon Windows in your pinned llvm-project revision.
242-257: Fail fast if required internal headers are missing (don’t print “Success!”)
Ifclicedepends on these headers, warning-only will push failure downstream and still report success.@@ headers_to_copy = ["CoroutineStmtBuilder.h", "TypeLocBuilder.h", "TreeTransform.h"] + missing = [] for header in headers_to_copy: @@ if src.exists(): shutil.copy(src, dst) print(f" Copied {header}") else: - print(f" Warning: {header} not found in source.") + missing.append(header) + + if missing: + print("Error: required header(s) not found in source:") + for h in missing: + print(f" - {h}") + sys.exit(1)
🧹 Nitpick comments (1)
scripts/build-llvm.py (1)
200-220: Consider making ASan-in-Debug opt-in (or clearly documenting it)
Debugalways enables-DLLVM_USE_SANITIZER=Addressand flipsBUILD_SHARED_LIBS=ON. If that’s intentional for CI determinism, fine; otherwise it’s a surprising default for local dev.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/llvm.yml(1 hunks)scripts/build-llvm.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/llvm.yml
🧰 Additional context used
🪛 Ruff (0.14.8)
scripts/build-llvm.py
226-226: subprocess call: check for execution of untrusted input
(S603)
227-227: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
235-235: subprocess call: check for execution of untrusted input
(S603)
236-236: Starting a process with a partial executable path
(S607)
⏰ 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). (11)
- GitHub Check: build (ubuntu-24.04, Debug)
- GitHub Check: build (windows-2025, RelWithDebInfo)
- GitHub Check: build (macos-15, Debug)
- GitHub Check: build (macos-15, releasedbg, on)
- GitHub Check: build (ubuntu-24.04, releasedbg, off)
- GitHub Check: build (ubuntu-24.04, releasedbg, on)
- GitHub Check: build (macos-15, debug, off)
- GitHub Check: build (windows-2025, releasedbg, on)
- GitHub Check: build (windows-2025, releasedbg, off)
- GitHub Check: build (macos-15, releasedbg, off)
- GitHub Check: build (ubuntu-24.04, debug, off)
🔇 Additional comments (1)
scripts/build-llvm.py (1)
9-36: Nice: fail-fast “must run from llvm-project root” check
This guardrail is solid and prevents confusing mis-invocations.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build-llvm.yml(1 hunks).github/workflows/upload-llvm.yml(1 hunks)pixi.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/upload-llvm.yml
- .github/workflows/build-llvm.yml
🔇 Additional comments (1)
pixi.toml (1)
14-14: Verify macOS toolchain setup.The platforms list includes
osx-arm64, but no macOS-specific activation scripts or toolchain configurations are visible in the manifest. Please confirm that the macOS build environment is properly configured or clarify if this platform support is incomplete.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pixi.toml (1)
28-31: Verify that the exact sysroot version pin is intentional.The
sysroot_linux-64 = "==2.17"constraint was previously flagged as overly restrictive. While pinning to glibc 2.17 may be intentional for backward compatibility with older Linux systems, the exact version requirement could cause dependency resolution failures if this specific version becomes unavailable in conda-forge.Confirm this exact pin is required, or consider relaxing it to
>=2.17to allow newer compatible versions.
🧹 Nitpick comments (1)
scripts/upload-llvm.py (1)
19-27: Consider extracting the exception message.The exception message is defined inline, which triggers a minor linting warning (TRY003). For consistency with Python best practices, consider defining a custom exception class or extracting the message to a constant.
Example refactor:
+class PlatformDetectionError(ValueError): + """Raised when platform cannot be determined from filename.""" + pass + def parse_platform(name: str) -> str: lowered = name.lower() if "windows" in lowered: return "windows" if "linux" in lowered: return "linux" if "macos" in lowered: return "macosx" - raise ValueError(f"Unable to determine platform from filename: {name}") + raise PlatformDetectionError(f"Unable to determine platform from filename: {name}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/upload-llvm.yml(1 hunks)pixi.toml(1 hunks)scripts/upload-llvm.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/upload-llvm.yml
🧰 Additional context used
🪛 Ruff (0.14.8)
scripts/upload-llvm.py
27-27: Avoid specifying long messages outside the exception class
(TRY003)
90-90: subprocess call: check for execution of untrusted input
(S603)
91-91: Starting a process with a partial executable path
(S607)
99-99: subprocess call: check for execution of untrusted input
(S603)
100-100: Starting a process with a partial executable path
(S607)
115-115: subprocess call: check for execution of untrusted input
(S603)
🔇 Additional comments (9)
pixi.toml (5)
1-17: LGTM!The workspace metadata, licensing, and documentation references are well-configured. All standard fields are present and properly formatted.
19-26: LGTM!The dependency versions are appropriately constrained. Pinning LLVM tools to a specific version ensures build consistency, while allowing flexibility for cmake and ninja.
33-37: LGTM!The platform-specific activation scripts are properly configured for Windows and Linux targets.
39-48: LGTM!The feature definitions and environment compositions are well-structured. The version constraint for xz (
>=5.8.1,<6) appropriately balances flexibility and stability.
50-69: LGTM!The task definitions are properly structured. The
upload-llvmtask correctly uses a parameterized branch argument with a sensible default, addressing the previous concern about hardcoded branch names.scripts/upload-llvm.py (4)
11-16: LGTM!The SHA256 computation is correctly implemented with efficient chunked reading to handle large files.
30-34: LGTM!The build type detection logic is simple and appropriate for the use case.
37-47: LGTM!The metadata construction correctly extracts feature flags from filenames and aggregates all relevant artifact information.
87-115: LGTM with input validation!The release management flow is well-structured: checking for existing releases before deletion, and using appropriate flags (
-yfor non-interactive deletion,--latestfor marking the release). The subprocess error handling withcheck=Trueensures failures are caught.Ensure the input validation suggested in the previous comment is implemented to secure these subprocess calls.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CMakeLists.txt (1)
182-190: Copy/install of${LLVM_INSTALL_PATH}/lib/clangis currently failing in CIThese lines:
file( COPY "${LLVM_INSTALL_PATH}/lib/clang" DESTINATION "${PROJECT_BINARY_DIR}/lib" ) install( DIRECTORY "${LLVM_INSTALL_PATH}/lib/clang" DESTINATION "${CMAKE_INSTALL_LIBDIR}" )are failing in the cmake workflow with:
file COPY cannot find '.../build/.llvm/lib/clang'This indicates that the extracted LLVM tree under
${LLVM_INSTALL_PATH}doesn’t havelib/clangat that exact location (e.g., the archive may include a top‑level directory or use a different layout).Two parts to address:
- Align
LLVM_INSTALL_PATHwith the archive layout (inscripts/setup-llvm.py), so it points at the actual LLVM install prefix that containslib/clang.- Make this block robust in case
lib/clangis absent, so the build doesn’t hard‑fail:if(EXISTS "${LLVM_INSTALL_PATH}/lib/clang") message(STATUS "Copying clang resource directory from ${LLVM_INSTALL_PATH}") file(COPY "${LLVM_INSTALL_PATH}/lib/clang" DESTINATION "${PROJECT_BINARY_DIR}/lib") install(DIRECTORY "${LLVM_INSTALL_PATH}/lib/clang" DESTINATION "${CMAKE_INSTALL_LIBDIR}") else() message(WARNING "LLVM clang resource directory not found at ${LLVM_INSTALL_PATH}/lib/clang; skipping copy/install") endif()Right now this is a hard build breaker and should be fixed before merging.
♻️ Duplicate comments (5)
scripts/upload-llvm.py (1)
50-57: Add basic validation fortagandtarget_repobefore callinggh
tagandtarget_repocome straight fromsys.argvand are passed togh releasecommands without validation. Whilesubprocess.runis used safely (noshell=True), validating format helps catch misconfigurations early and addresses the earlier security lint.You can enforce simple patterns (semver-ish tag,
owner/repoformat) before proceeding:- tag, target_repo, workflow_id = sys.argv[1:] + tag, target_repo, workflow_id = (arg.strip() for arg in sys.argv[1:]) + + import re, sys as _sys + if not re.match(r"^[vV]?\d+\.\d+\.\d+(?:[-+][\w.\+]+)?$", tag): + print(f"Invalid tag format: {tag}", file=_sys.stderr) + _sys.exit(1) + if not re.match(r"^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$", target_repo): + print(f"Invalid repository format: {target_repo}", file=_sys.stderr) + _sys.exit(1).github/workflows/cmake.yml (1)
42-49: Pinprefix-dev/setup-pixito a commit SHA for supply-chain hardeningThe Pixi step still uses the mutable tag:
uses: prefix-dev/setup-pixi@v0.9.3Per earlier review, pinning to the corresponding commit SHA makes the workflow reproducible and protects against tag rewrites, e.g.:
uses: prefix-dev/setup-pixi@82d477f15f3a381dbcc8adc1206ce643fe110fb7 # v0.9.3cmake/package.cmake (1)
3-9: MakeFETCHCONTENT_UPDATES_DISCONNECTEDopt‑in instead of forcing it globally
FETCHCONTENT_UPDATES_DISCONNECTEDis set unconditionally:include(${CMAKE_CURRENT_LIST_DIR}/llvm.cmake) setup_llvm("21.1.4+r1") include(FetchContent) set(FETCHCONTENT_UPDATES_DISCONNECTED ON)This globally disables FetchContent updates for all builds, including local dev, which can be surprising and make dependency updates harder.
Consider gating it behind a CI/dev flag or option, e.g.:
option(CLICE_FETCHCONTENT_NO_UPDATES "Disable FetchContent updates" OFF) if(CLICE_FETCHCONTENT_NO_UPDATES OR CLICE_CI_ENVIRONMENT) set(FETCHCONTENT_UPDATES_DISCONNECTED ON) endif()CMakeLists.txt (2)
1-8: CMake 3.30 requirement still looks stricter than necessary – either justify or relaxYou’ve raised:
cmake_minimum_required(VERSION 3.30) set(CMAKE_CXX_SCAN_FOR_MODULES OFF)but there’s no obvious 3.30‑specific feature in this file (CMAKE_CXX_SCAN_FOR_MODULES landed earlier). This was previously flagged; the concern remains for users on older LTS CMake.
Either:
- Lower the minimum to the actual required version (e.g., 3.26 or whatever your features need), or
- Add an inline comment explaining why 3.30 is needed, and document this requirement in your README/INSTALL docs.
81-100: Linux-static-libstdc++ -static-libgccshould be opt-inFor Linux you now link with:
target_link_options(clice_options INTERFACE -fuse-ld=lld -static-libstdc++ -static-libgcc -Wl,--gc-sections )Unconditionally statically linking libstdc++/libgcc can cause portability/runtime issues across distros and increases binary size. This was previously raised and is still unconditional.
Consider gating this behind an option, e.g.:
option(CLICE_LINUX_STATIC_STDLIB "Link libstdc++/libgcc statically on Linux" OFF) if(CMAKE_SYSTEM_NAME STREQUAL "Linux") target_link_options(clice_options INTERFACE -fuse-ld=lld $<$<BOOL:${CLICE_LINUX_STATIC_STDLIB}>:-static-libstdc++ -static-libgcc> -Wl,--gc-sections ) endif()
🧹 Nitpick comments (3)
scripts/upload-llvm.py (1)
30-35: Clarifyparse_build_typemapping for non-Debug artifacts
parse_build_typecurrently returnsDebugif the filename contains"debug"and alwaysRelWithDebInfootherwise. That silently collapsesRelease,RelWithDebInfo,MinSizeRel, etc. into a single bucket, which can make future artifact selection ambiguous or incorrect.Consider aligning this mapping with the consumer in
scripts/setup-llvm.py::normalize_build_typeand distinguishing at leastDebug,RelWithDebInfo, andRelease, e.g.:def parse_build_type(name: str) -> str: lowered = name.lower() - if "debug" in lowered: - return "Debug" - return "RelWithDebInfo" + if "debug" in lowered: + return "Debug" + if "relwithdebinfo" in lowered or "reldbg" in lowered or "releasedbg" in lowered: + return "RelWithDebInfo" + return "Release"This keeps manifest entries consistent with typical CMake build types and with
setup-llvm.py.cmake/llvm.cmake (1)
46-52: Makellvm-libscreation idempotent for potential reuse
llvm-libsis created unconditionally insidesetup_llvm:add_library(llvm-libs INTERFACE IMPORTED) target_include_directories(llvm-libs INTERFACE "${LLVM_INSTALL_PATH}/include")If
setup_llvmis ever called more than once (e.g., from another top‑level CMakeLists or a future subproject), this will error with “Targetllvm-libsalready exists”.A small guard makes this function safe to re‑invoke:
if(NOT TARGET llvm-libs) add_library(llvm-libs INTERFACE IMPORTED) target_include_directories(llvm-libs INTERFACE "${LLVM_INSTALL_PATH}/include") endif()scripts/setup-llvm.py (1)
241-259: Clarify--offlinebehavior: avoid network access when no local LLVM is availableCurrently,
--offlineonly affects private-header fetching:if install_path is None: detected = system_llvm_ok(...) ... artifact = None if install_path is None: artifact = pick_artifact(...) ... ensure_download(url, download_path, artifact["sha256"], token)If
--offlineis passed, no system LLVM is suitable, and no--install-pathis given, the script will still attempt to download the main artifact from GitHub, which is surprising for an “offline” mode.Consider either:
- Treating
--offlineas “must not hit the network”: if noinstall_pathand no suitable system LLVM are found, raise a clear error instead of trying to download, or- Renaming the flag to something narrower (e.g.
--offline-headers) and documenting that it only affects private-header fetching.Example for stricter offline semantics:
- if install_path is None: - artifact = pick_artifact(...) - ... - ensure_download(url, download_path, artifact["sha256"], token) - extract_archive(download_path, install_root) - install_path = install_root + if install_path is None: + if args.offline: + raise RuntimeError( + "Offline mode enabled but no local LLVM install or suitable system LLVM found" + ) + artifact = pick_artifact(...) + ... + ensure_download(url, download_path, artifact["sha256"], token) + extract_archive(download_path, install_root) + install_path = install_root
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/cmake.yml(2 hunks).github/workflows/xmake.yml(1 hunks)CMakeLists.txt(3 hunks)cmake/github.cmake(0 hunks)cmake/llvm.cmake(1 hunks)cmake/llvm_setup.cmake(0 hunks)cmake/package.cmake(5 hunks)cmake/utils.cmake(0 hunks)config/default-toolchain-version(0 hunks)config/llvm-mainfist.json(1 hunks)config/prebuilt-llvm.json(0 hunks)scripts/setup-llvm.py(1 hunks)scripts/upload-llvm.py(1 hunks)
💤 Files with no reviewable changes (5)
- cmake/github.cmake
- config/default-toolchain-version
- config/prebuilt-llvm.json
- cmake/llvm_setup.cmake
- cmake/utils.cmake
✅ Files skipped from review due to trivial changes (1)
- config/llvm-mainfist.json
🧰 Additional context used
🪛 GitHub Actions: cmake
cmake/llvm.cmake
[error] 28-28: CMake execute_process failed: Child return code: 1. See CMakeLists.txt and cmake/package.cmake for details.
CMakeLists.txt
[error] 183-183: CMake error: file COPY cannot find '/home/runner/work/clice/clice/build/.llvm/lib/clang': No such file or directory.
scripts/setup-llvm.py
[error] 283-283: RuntimeError: No matching LLVM artifact in manifest for version=21.1.4, platform=Windows, build_type=Release, is_lto=False
🪛 Ruff (0.14.8)
scripts/setup-llvm.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
60-63: Avoid specifying long messages outside the exception class
(TRY003)
79-79: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
81-81: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
86-86: Avoid specifying long messages outside the exception class
(TRY003)
101-103: Avoid specifying long messages outside the exception class
(TRY003)
111-111: Uses of tarfile.extractall()
(S202)
116-116: Avoid specifying long messages outside the exception class
(TRY003)
117-117: subprocess call: check for execution of untrusted input
(S603)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
146-146: subprocess call: check for execution of untrusted input
(S603)
147-147: subprocess call: check for execution of untrusted input
(S603)
164-164: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
165-165: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
175-175: Do not catch blind exception: Exception
(BLE001)
186-186: Do not catch blind exception: Exception
(BLE001)
237-239: Avoid specifying long messages outside the exception class
(TRY003)
scripts/upload-llvm.py
27-27: Avoid specifying long messages outside the exception class
(TRY003)
90-90: subprocess call: check for execution of untrusted input
(S603)
91-91: Starting a process with a partial executable path
(S607)
99-99: subprocess call: check for execution of untrusted input
(S603)
100-100: Starting a process with a partial executable path
(S607)
115-115: subprocess call: check for execution of untrusted input
(S603)
🔇 Additional comments (1)
.github/workflows/xmake.yml (1)
13-21: PR trigger now only targetsmain-testPRs – confirm this is intentionalThe
pull_requesttrigger is restricted tobranches: [main-test], so xmake CI will not run for PRs targetingmainanymore. If you still want coverage onmain, consider including both branches:branches: [main, main-test]The added
config/**path filter looks good and will ensure config changes exercise this workflow.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmake/llvm.cmake (1)
28-36: Verify that the Windows RelWithDebInfo build-type issue has been resolved.A previous review flagged that this
execute_processcall was failing for Windows withCMAKE_BUILD_TYPE=RelWithDebInfodue to build-type normalization issues in the Python script. The Python script no longer contains anormalize_build_typefunction, suggesting the issue may have been addressed. However, please verify that Windows RelWithDebInfo builds now succeed.Run the following script to check if the Python script properly handles all build types:
#!/bin/bash # Check how build types are used in the Python script rg -n -A3 -B3 'build.type|build_type' scripts/setup-llvm.pyscripts/setup-llvm.py (1)
43-60: Verify that manifest consistently usesis_ltofield.The artifact picker checks
entry.get("is_lto")at line 54. If the manifest usesltoinstead ofis_lto, LTO artifacts won't be matched. Ensure all manifest producers use theis_ltokey consistently, or add a fallback:- if bool(entry.get("is_lto")) != is_lto: + entry_is_lto = entry.get("is_lto") + if entry_is_lto is None: + entry_is_lto = entry.get("lto") + if bool(entry_is_lto) != is_lto: continue
🧹 Nitpick comments (2)
scripts/setup-llvm.py (2)
135-145: Archive extraction is acceptable for controlled sources.Line 141 uses
tarfile.extractall(), which static analysis flags due to potential path traversal vulnerabilities. Since archives are downloaded from controlled GitHub releases with SHA256 verification, this risk is mitigated. However, if archives might come from untrusted sources in the future, consider usingtarfile.data_filter()(Python 3.12+) or manual extraction with path validation.
258-355: Main function correctly handles all installation scenarios.The logic flow manages three scenarios: using an existing provided path, downloading to a provided path, or auto-detecting system LLVM. The implementation is correct. One minor observation: the
needs_installflag and repeatedpick_artifactcalls (lines 306-308 and 319-321) could be consolidated, but the current structure is clear and functional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmake/llvm.cmake(1 hunks)scripts/setup-llvm.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
scripts/setup-llvm.py
40-40: Avoid specifying long messages outside the exception class
(TRY003)
57-60: Avoid specifying long messages outside the exception class
(TRY003)
77-77: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
79-79: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
113-113: Avoid specifying long messages outside the exception class
(TRY003)
115-115: Avoid specifying long messages outside the exception class
(TRY003)
130-132: Avoid specifying long messages outside the exception class
(TRY003)
141-141: Uses of tarfile.extractall()
(S202)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
157-159: Avoid specifying long messages outside the exception class
(TRY003)
189-189: subprocess call: check for execution of untrusted input
(S603)
190-190: subprocess call: check for execution of untrusted input
(S603)
207-207: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
208-208: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
218-218: Do not catch blind exception: Exception
(BLE001)
229-229: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (3)
- GitHub Check: build (macos-15, Debug)
- GitHub Check: build (ubuntu-24.04, Debug)
- GitHub Check: build (windows-2025, RelWithDebInfo)
🔇 Additional comments (12)
cmake/llvm.cmake (2)
38-48: LGTM!The JSON parsing, path resolution, and validation logic are correct and provide clear error messages.
50-107: LGTM!The library setup correctly handles different build configurations. The use of
CONFIGURE_DEPENDSwithfile(GLOB)ensures CMake re-runs when library files change, and the conditional logic appropriately distinguishes between Debug (specific library names) and Release builds (glob all static libraries).scripts/setup-llvm.py (10)
16-30: LGTM!The constants and basic helper functions are well-defined and straightforward.
32-41: LGTM!Platform detection is straightforward and covers the expected platforms with appropriate error handling.
63-68: LGTM!SHA256 checksum implementation is efficient with appropriate chunk size.
71-116: LGTM!The download function includes comprehensive progress reporting and proper error handling. The static analysis warning about
urlopenis a false positive in this context, as downloading from URLs is the intended functionality.
118-133: LGTM!The download verification logic correctly handles caching, re-downloads on mismatch, and validates checksums for security.
147-163: LGTM!The flatten logic correctly handles nested install directories with proper conflict detection.
165-180: LGTM!Version parsing correctly extracts numeric components for comparison.
182-198: LGTM!System LLVM detection appropriately skips Debug builds and validates version requirements. The subprocess security warnings are false positives for invoking the standard
llvm-configtool.
200-233: LGTM!GitHub API integration correctly handles both lightweight and annotated tags. The broad exception handling (lines 218, 229) is appropriate here to gracefully fall back when API calls fail.
235-256: LGTM!Private header fetching correctly handles offline mode and gracefully falls back when commit lookup fails.
16bit-ykiko
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/cmake.yml (1)
42-49: Pin action to commit SHA for supply-chain hardening.Line 43 uses a mutable semantic version tag (
@v0.9.3). For immutability and security, pin to the full commit SHA.Apply this diff:
- - name: Setup Pixi - uses: prefix-dev/setup-pixi@v0.9.3 + - name: Setup Pixi + uses: prefix-dev/setup-pixi@82d477f15f3a381dbcc8adc1206ce643fe110fb7 # v0.9.3
🧹 Nitpick comments (2)
CMakeLists.txt (2)
34-62: Consider using interface target instead of globaladd_compile_options.Line 35 applies
-fsanitize=addressglobally to all targets viaadd_compile_options. For better modularity, consider applying it to theclice_optionsinterface target instead:if(CMAKE_BUILD_TYPE STREQUAL "Debug") - add_compile_options(-fsanitize=address) + target_compile_options(clice_options INTERFACE -fsanitize=address)This provides the same effect (since
clice_optionsis linked by all targets) but is more explicit and follows CMake best practices for managing compile options through interface libraries.
97-103: Consider making static stdlib linking optional.Line 100 statically links libstdc++ and libgcc on Linux, improving portability but increasing binary size and potentially causing issues with dlopen or plugins. For a language server binary, static linking is generally reasonable, but consider making it configurable via an option like
CLICE_LINUX_STATIC_STDLIBto give users flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/cmake.yml(2 hunks)CMakeLists.txt(3 hunks)cmake/llvm.cmake(1 hunks)cmake/toolchain.cmake(1 hunks)config/llvm-manifest.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmake/llvm.cmake
⏰ 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). (3)
- GitHub Check: build (ubuntu-24.04, Debug)
- GitHub Check: build (macos-15, Debug)
- GitHub Check: build (windows-2025, RelWithDebInfo)
🔇 Additional comments (5)
config/llvm-manifest.json (1)
1-83: LGTM!The manifest structure is well-organized with consistent field coverage across all platforms and build configurations. Each entry provides complete metadata including version, filename, SHA-256 checksum, LTO/ASAN flags, platform, and build type. The absence of Debug builds without ASAN is intentional and aligns with the project's build configuration.
CMakeLists.txt (4)
15-30: LGTM!The ThinLTO implementation is well-structured: optional (default OFF), consistently applied to all compiler and linker flags, and appropriate for the Clang-based toolchain.
64-67: LGTM!The
_LIBCPP_DISABLE_AVAILABILITY=1macro is a standard workaround for conda/pixi environments, allowing newer C++ features with older SDKs. This aligns with the PR's transition to pixi-managed toolchains.
81-83: LGTM!The addition of
ntdlllinkage suggests new Windows-specific functionality. Thentdlllibrary provides low-level NT kernel interfaces and is a standard system library on all Windows versions.
105-121: LGTM!The compiler detection correctly handles both MSVC and clang-cl (Clang with MSVC frontend) using
CMAKE_CXX_COMPILER_FRONTEND_VARIANT, ensuring appropriate flags are applied for MSVC-compatible compilers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/setup-llvm.py (1)
43-60: Makelto/is_ltohandling resilient to manifest schema driftRight now
pick_artifactonly looks atentry.get("lto"). If any existing or future manifest producers still emitis_lto, LTO artifacts won’t match even though the rest of the entry is correct.You can make this robust to both key names with a small fallback:
- if bool(entry.get("lto")) != is_lto: - continue + entry_is_lto = entry.get("lto") + if entry_is_lto is None: + entry_is_lto = entry.get("is_lto") + if bool(entry_is_lto) != is_lto: + continueThis keeps behavior identical for current manifests while tolerating older/newer ones that use a different field name.
🧹 Nitpick comments (2)
scripts/setup-llvm.py (2)
265-267: Clarify the scope of--offline(currently only skips private-header fetches)
--offlineis only used to gateensure_private_headers, while the main artifact download path still hits GitHub if the tarball isn’t already present. That’s fine if the flag is meant as “don’t fetch private headers,” but the name and log line suggest full offline behavior.Either update the help/description to make this explicit, or extend
--offlineto also short‑circuit remote downloads (e.g., fail fast when no suitable local/system LLVM is available).Also applies to: 270-275, 337-337
71-116: Optional hardening for network, archive extraction, and subprocess usageA few spots are good candidates for small hardening tweaks:
download/github_apiuseurlopenwithout an explicit timeout or scheme check. In CI, a stuck TCP connection could hang the job. Consider passing a finite timeout and assertingurl.startswith("https://")at call sites.extract_archiveusestarfile.extractall, which is fine for trusted, pipeline‑produced archives, but if these ever become user‑supplied it’d be safer to validate members (e.g., reject paths with..or absolute paths) before extraction.system_llvm_okrunsllvm-configviasubprocess.check_output, which is acceptable since the binary comes fromshutil.which, but if PATH is user‑controlled it might be worth logging the resolved path when a mismatch occurs.lookup_llvm_commitcatches bareExceptiontwice, which hides unexpected failures; you could narrow this toURLError/HTTPError(or log the exception at debug level) while still treating header fetches as non‑fatal.None of these are correctness blockers, but they’d make the script more robust under bad network or environment conditions.
Also applies to: 135-145, 189-190, 200-210, 215-232, 253-255
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/setup-llvm.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
scripts/setup-llvm.py
40-40: Avoid specifying long messages outside the exception class
(TRY003)
57-60: Avoid specifying long messages outside the exception class
(TRY003)
77-77: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
79-79: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
113-113: Avoid specifying long messages outside the exception class
(TRY003)
115-115: Avoid specifying long messages outside the exception class
(TRY003)
130-132: Avoid specifying long messages outside the exception class
(TRY003)
141-141: Uses of tarfile.extractall()
(S202)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
157-159: Avoid specifying long messages outside the exception class
(TRY003)
189-189: subprocess call: check for execution of untrusted input
(S603)
190-190: subprocess call: check for execution of untrusted input
(S603)
207-207: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
208-208: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
218-218: Do not catch blind exception: Exception
(BLE001)
229-229: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (3)
- GitHub Check: build (ubuntu-24.04, Debug)
- GitHub Check: build (macos-15, Debug)
- GitHub Check: build (windows-2025, RelWithDebInfo)
🔇 Additional comments (1)
scripts/setup-llvm.py (1)
258-352: Overall flow and install resolution look solidThe CLI wiring, system‑vs‑bundled LLVM resolution, SHA256‑verified downloads, and install flattening are all well‑structured and easy to follow. Logging is detailed enough to debug CI issues without being noisy, and the JSON output surface (
install_path,cmake_dir,artifact) is nicely minimal.
1 participant
TODO:
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.