Repository navigation
build: modernize LLVM build and release workflow - #476
Merged
Merged
Conversation
- Replace build-llvm.yml with simplified version (no prune/upload steps, those move to release-llvm.yml) - Add release-llvm.yml: discover unused libs, create clice-llvm release, repackage with pruning, generate manifest - Replace cmake/llvm.cmake: switch to find_package(LLVM/Clang) with automatic artifact download based on manifest - Add scripts/release-llvm.py: unified discover/apply/repackage - Remove old scripts: setup-llvm.py, upload-llvm.py, download-llvm.sh, validate-llvm-components.py, prune-llvm-bin.py, llvm-components.json - Convert llvm-manifest.json from list to key-based dict format - Add upgrade-llvm skill and changelog infrastructure Temporary push trigger on build-llvm.yml to validate with LLVM 21.1.8 build (will be removed before merge).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReworks LLVM packaging and installation around manifest-driven artifacts, adds a new release workflow, updates manifest/version handling, removes old LLVM helper scripts, and adds upgrade/changelog documentation. ChangesLLVM release pipeline modernization
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4719a733b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-llvm.yml:
- Around line 85-86: The Checkout repository step in the build-llvm workflow is
persisting repository credentials into later steps, which is unnecessary for a
read-only job. Update the actions/checkout usage in the workflow to disable
persisted credentials so the token is not available to subsequent build/package
steps.
- Around line 10-14: Remove the temporary push trigger from the workflow so it
no longer runs on the build/modernize-llvm-workflow branch; update the triggers
in the build-llvm workflow to keep only the intended event configuration. Locate
the change in the workflow’s top-level push section and delete the
branch-specific push filter before merging.
- Around line 175-198: The archive-name and packaging step in the build script
is running before strict shell mode is enabled, so failures from
scripts/release-llvm.py or unknown target triples can surface later and be
harder to diagnose. Move the strict shell setup to before computing ARCHIVE, and
make the matrix.target_triple handling in the PLATFORM/ARCH selection fail fast
on unsupported values instead of silently continuing.
In @.github/workflows/release-llvm.yml:
- Line 33: The checkout step is persisting Git credentials unnecessarily, which
leaves the token in .git/config for later steps. Update both actions/checkout
usages in the release-llvm workflow to disable persisted credentials, since this
job does not perform git pushes. Use the checkout configuration itself as the
fix point so the token is not retained for subsequent script or build steps.
- Around line 57-58: The release-llvm workflow is interpolating
workflow_dispatch inputs directly in shell commands, which can allow
quote-breaking injection. Update the affected run blocks to use the
already-defined environment variables instead of referencing the raw inputs,
especially around SOURCE_RUN_ID and LLVM_VERSION usage in the
download/build/publish steps. Keep the command logic the same, but route all
user-provided values through env vars consistently across the listed run
sections.
- Around line 15-18: The release-llvm workflow is relying on the default
GITHUB_TOKEN permissions instead of explicitly declaring least-privilege access.
Update the workflow’s top-level permissions to read-only for checkout and
artifact download, and keep release upload actions using secrets.UPLOAD_LLVM;
reference the workflow definition and its release job so the permissions are
scoped as narrowly as possible.
In `@cmake/llvm.cmake`:
- Around line 84-87: The configure-time download in the file(DOWNLOAD) call
needs explicit timeout limits to avoid hanging CMake indefinitely. Update the
download logic in llvm.cmake so the existing file(DOWNLOAD) invocation includes
both a total timeout and an inactivity timeout, while preserving the current
_URL, _DOWNLOAD_PATH, EXPECTED_HASH, SHOW_PROGRESS, and STATUS _DL_STATUS
behavior. Refer to the file(DOWNLOAD) block in this CMake script when making the
change.
- Around line 108-115: Reject manifest/version drift in the LLVM lookup path by
validating the manifest’s top-level version before consuming the artifact hash.
In the setup_llvm flow, after reading llvm-manifest.json and before using
string(JSON ... GET ...) for the artifact hash, inspect the manifest version and
compare it against the expected version driven by setup_llvm("21.1.8"); if they
differ, fail early with a clear fatal error. Keep the existing artifact lookup
in place, but ensure the validation is done in the same manifest-reading logic
so mismatched manifests are caught before any hash/download handling.
- Around line 133-149: When _NEED_INSTALL is true in the llvm download/install
flow, clear out any existing install root before extracting so reruns don’t
collide with stale partial contents; update the logic around
_download_and_extract and the build-install rename/cleanup block to remove
"${_INSTALL_ROOT}" first, then recreate it and proceed with extraction and
version stamping.
- Around line 65-67: The ASAN suffix logic in the llvm CMake flow is using the
host check in the suffix block instead of the target platform, so update the
`CMAKE_BUILD_TYPE`/`_SUFFIX` handling in `cmake/llvm.cmake` to key off
`_PLATFORM` rather than `WIN32`. Adjust the condition near the `_SUFFIX` append
so cross-target Windows builds don’t add `-asan` and non-Windows debug targets
still do, keeping the artifact name selection aligned with the target platform.
In `@scripts/release-llvm.py`:
- Around line 55-73: Rename the ambiguous tuple variable in the ARTIFACTS
comprehension that feeds build_artifact_name, since Ruff flags the single-letter
l name. Update the list comprehension in release-llvm.py to use a clearer
identifier for that boolean flag (and keep the call to build_artifact_name
aligned with the renamed variable) so linting passes without changing behavior.
- Around line 217-221: The manifest entry name handling in the removal loop is
unsafe because `entry["name"]` is joined directly into `install_dir / name`, so
validate the filename before using it in `removed` processing. In the
`release-llvm.py` logic around `_replace_with_empty_archive`, reject any `name`
containing path separators, `..`, or absolute paths, and only allow a normalized
basename that stays within `install_dir`. Keep the check close to the
`entry["name"]` extraction so malformed manifest entries cannot escape the LLVM
lib directory.
In `@scripts/update-llvm-version.py`:
- Around line 19-30: The manifest validation in the update-llvm-version script
is too loose, since it only checks for a dict with artifacts and can still pass
invalid shapes that later break on .items() or get copied unchanged. Tighten the
validation block around data/src/dest to require a dict with version and
artifacts, then verify each artifact entry contains the required metadata fields
before writing the manifest. Update the same schema checks in the related
copy/check path referenced by the comment so both flows enforce the new manifest
contract consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54f96bca-99d3-46a2-9707-c987ba28ea84
📒 Files selected for processing (17)
.claude/commands/upgrade-llvm.md.github/workflows/build-llvm.yml.github/workflows/release-llvm.yml.github/workflows/upload-llvm.ymlcmake/llvm.cmakeconfig/llvm-manifest.jsondocs/en/changelog/feature-changelog.mddocs/en/changelog/llvm-changelog.mdscripts/build-llvm.pyscripts/download-llvm.shscripts/llvm-components.jsonscripts/prune-llvm-bin.pyscripts/release-llvm.pyscripts/setup-llvm.pyscripts/update-llvm-version.pyscripts/upload-llvm.pyscripts/validate-llvm-components.py
💤 Files with no reviewable changes (7)
- scripts/download-llvm.sh
- .github/workflows/upload-llvm.yml
- scripts/validate-llvm-components.py
- scripts/llvm-components.json
- scripts/prune-llvm-bin.py
- scripts/setup-llvm.py
- scripts/upload-llvm.py
clangOptions and clangTidyCustomModule don't exist in LLVM 21. These will be added back in the LLVM 22 upgrade PR.
16bit-ykiko
force-pushed
the
build/modernize-llvm-workflow
branch
from
June 30, 2026 10:49
eeb18a1 to
0a44c74
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a44c7418f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Update llvm-manifest.json with hashes from new release-llvm build - Update cmake/package.cmake version to 21.1.8+r1 - Remove temporary push trigger from build-llvm.yml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39b8dd1367
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- URL-encode '+' in LLVM version for GitHub release download - Remove stale upload-llvm task from pixi.toml - Fix upgrade-llvm skill: remove non-existent skip_clice_build input - Fix changelog heading level guidance (H1 → H2) - Clear LLVM 21→22 changelog content (belongs in LLVM 22 PR) - Add persist-credentials: false to build-llvm checkout - Normalize platform field from "macosx" to "macos" in manifest
- cmake/llvm.cmake: use FetchContent instead of hand-rolled download/ cache logic; search flat JSON array manifest by structured fields - config/llvm-manifest.json: convert to flat array; normalize arch to arm64/x64, platform to linux/macos/windows - release-llvm.yml: finalize step outputs flat array manifest - release-llvm.py: inline small helpers, remove decorative comments, remove artifact-name subcommand, remove version from metadata - build-llvm.py: inline normalize_mode and human_readable, simplify config output - Delete unused: update-llvm-version.py, monitor-resources.sh/ps1, delete-artifacts.bash - pixi.toml: remove stale helper tasks - check-format.yml: remove update-llvm-version validation step - upgrade-llvm.md: update Step 6, remove stale version-cache note
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5595d49f79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Restore build-llvm task in pixi.toml (used by CI workflow) - Add permissions block to release-llvm.yml (contents+actions read) - Add --ref to release-llvm trigger in upgrade-llvm skill
- Remove pixi build-llvm task, call script directly in CI - Revert permissions addition to release-llvm.yml (was working without) - Add --ref to upgrade-llvm skill Step 5
Pack from inside build-install/ so archives extract directly to include/ and lib/ without an extra directory layer. Remove --strip-components=1 from release-llvm.py and build-install detection from cmake/llvm.cmake. Temporary push trigger to rebuild LLVM 21.1.8 with new layout.
- Rename aarch64-linux-gnu-* and aarch64-windows-msvc-* artifacts to arm64-linux-gnu-* and arm64-windows-msvc-* for consistency - Add concurrency groups to build-llvm and release-llvm workflows to cancel previous runs on new pushes
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c92dca1560
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92a38597be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
FetchContent downloads from HTTPS (TLS integrity), so SHA256 verification is redundant. The artifact filename is deterministic from (arch, platform, toolchain, mode, lto, asan), so compute it in cmake instead of looking up a manifest file. - Delete config/llvm-manifest.json - cmake/llvm.cmake: build filename directly, drop URL_HASH - release-llvm.yml: remove finalize job and metadata upload - release-llvm.py: remove build_metadata_entry, --version, hashlib - upgrade-llvm.md: simplify Step 6 to just updating package.cmake
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db55ea79e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Delete _wait_and_download_manifest and apply CLI subcommand from release-llvm.py (never invoked by any workflow) - cmake/llvm.cmake: FATAL_ERROR on unrecognized CMAKE_SYSTEM_PROCESSOR instead of silently defaulting to x64
3 of 4 tasks
2 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Overhaul the LLVM build and release infrastructure. Net result: -1100 lines, simpler workflows, no manifest file.
Before → After
setup-llvm.py(405 lines): hand-rolled download, extract, version stamp, hash verifycmake/llvm.cmake: 5-lineFetchContent_Declare+FetchContent_MakeAvailableconfig/llvm-manifest.jsonwith SHA256 hashes, looked up by filename keyprune-llvm-bin.py(261 lines) +upload-llvm.py(129 lines) +upload-llvm.ymlrelease-llvm.py(~270 lines) +release-llvm.yml— unified discover/apply/repackagetar -C .llvm -cf - build-install(extra directory layer, workarounds everywhere)tar -C .llvm/build-install -cf - .(flat: extract directly to include/ and lib/)aarch64-linux-gnu-*andarm64-macos-clang-*arm64-*for all ARM platformsvalidate-llvm-components.py(163 lines) +llvm-components.json(99 lines)update-llvm-version.py(165 lines) — regex-replace cmake + copy manifestcpmanifest + edit one line inpackage.cmake(now just edit one line, no manifest)Deleted files (9)
setup-llvm.py,upload-llvm.py,download-llvm.sh,prune-llvm-bin.py,validate-llvm-components.py,llvm-components.json,update-llvm-version.py,monitor-resources.sh,monitor-resources.ps1Also
/upgrade-llvmskill for automated LLVM upgradesdocs/en/changelog/directory for tracking changesVerified with
build-llvm14/14 success for LLVM 21.1.8release-llvm19/19 success → clice-llvm21.1.8+r1published