Merged
Conversation
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #79 +/- ##
==========================================
+ Coverage 83.24% 84.90% +1.66%
==========================================
Files 2 7 +5
Lines 179 265 +86
Branches 62 71 +9
==========================================
+ Hits 149 225 +76
- Misses 30 40 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
📝 WalkthroughWalkthroughThis PR replaces dynamic GitHub-release lookups with a static version manifest and manifest-driven downloader, removes the token input, adds manifest generation/updater code and workflow, updates action build to include the updater, and standardizes CI runners to Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions (update-known-versions)
participant Script as Node Script (update-known-versions.ts)
participant Manifest as Manifest Builder (manifest.ts)
participant GitHub as GitHub API (Octokit)
participant FS as Filesystem
participant PR as PR Creation (peter-evans)
GHA->>Script: run updater
Script->>Manifest: call updateManifest()
Manifest->>GitHub: list releases (paginated)
GitHub-->>Manifest: releases + assets
Manifest->>Manifest: extract asset metadata, pair zstd entries
Manifest->>FS: write `version-manifest.json`
Manifest->>FS: update `README.md` section
Manifest->>GitHub: fetch latest release
Manifest-->>Script: return latest-tag
Script->>PR: create/update PR with manifest changes
sequenceDiagram
participant User as User / Workflow
participant Action as setup-mlir Action (index.ts)
participant Download as Download Utils (download.ts)
participant FS as Filesystem (version-manifest.json)
participant Net as Network (asset downloads)
User->>Action: request install (version, platform, arch)
Action->>Download: getMLIRUrl(version, platform, arch, debug)
Download->>FS: read `version-manifest.json`
FS-->>Download: manifest entries
Download->>Download: find matching entry (prefix match)
Download-->>Action: return MLIR asset url & name
Action->>Download: getZstdUrl(version, platform, arch)
Download-->>Action: return zstd url & name
Action->>Net: download asset & zstd
Net-->>Action: binaries
Action->>User: setup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/update-version-manifest.yml:
- Line 33: Change the workflow step that runs node
dist/update-version-manifest/index.js so it does not pass the token as a CLI
argument; instead set GITHUB_TOKEN in the step's environment and remove the
positional argument, and update the script dist/update-version-manifest/index.js
to read the token from process.env.GITHUB_TOKEN (replace usages of
process.argv[2] with process.env.GITHUB_TOKEN) so the token is provided via
environment variables rather than command-line arguments.
In `@src/update-version-manifest.ts`:
- Around line 110-120: The releases fetch only requests a single page; modify
the logic around octokit.request (the releases variable) to paginate by looping
with a page counter (e.g., page=1, per_page=100), repeatedly calling "GET
/repos/{owner}/{repo}/releases" with owner=REPO_OWNER and repo=REPO_NAME until
an empty page is returned, concatenating releases.data into a single array, and
then derive downloadUrls from that aggregated releases array (keeping the
existing asset filtering by name.startsWith("llvm-mlir")). Ensure the loop
handles rate limits/errors and stops when a page returns fewer than per_page
items or no items.
- Around line 28-31: The release-fetch call currently only requests per_page:
100 and misses additional pages; modify the logic that calls
octokit.repos.listReleases (the code that builds the releases list in
update-version-manifest.ts) to implement pagination: loop incrementing a page
parameter (e.g., page = 1,2,3...) calling octokit.repos.listReleases({ owner:
REPO_OWNER, repo: REPO_NAME, per_page: 100, page }) until the returned releases
array is empty, concatenating results into the full releases array before
processing; apply the identical pagination fix to the analogous call in
get-download-link.ts (the function that queries releases there) so both places
collect all pages rather than only the first page.
denialhaag
force-pushed
the
known-versions
branch
from
b093eba to
b897158
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @.github/workflows/update-version-manifest.yml:
- Around line 42-44: The workflow references
steps.update-version-manifest.outputs.latest-version but the script sets the
output as latest-tag; update the workflow to use
steps.update-version-manifest.outputs.latest-tag (replace latest-version with
latest-tag in the title and body templates) so the PR title and body get the
correct value from the src/update-version-manifest.ts output.
In `@CHANGELOG.md`:
- Around line 12-14: Update the changelog entry string "✨ Add version maifest
([`#79`]) ([**@denialhaag**])" to correct the typo by replacing "maifest" with
"manifest" so the line reads "✨ Add version manifest ([`#79`])
([**@denialhaag**])" in CHANGELOG.md.
In `@src/get-download-link.ts`:
- Around line 206-223: The pagination loop fetching releases (variables/releases
array, page, and the octokit.request call) should stop when a page is partial
instead of only when empty; after fetching releasesPage.data check if
releasesPage.data.length === 0 or releasesPage.data.length < per_page and break
early if true (use the same per_page value passed to octokit.request), push the
data before breaking, and keep using REPO_OWNER/REPO_NAME as currently used in
the request.
In `@src/update-version-manifest.ts`:
- Around line 78-98: The manifest is written compactly which causes formatter
churn; in updateManifest replace the compact JSON write with a pretty-printed
JSON string and a trailing newline by using JSON.stringify(manifest, null, 2) +
'\n' when calling fs.writeFile for MANIFEST_FILE so the manifest variable is
written indented and stable across commits.
- Around line 100-142: The top-level run() invocation should surface failures to
GitHub Actions by catching errors and calling core.setFailed; modify the end of
the file so the call to run() is wrapped in a promise rejection handler (use
run() and core.setFailed) that converts the thrown error to a string and passes
it to core.setFailed to ensure Action shows a clean failure instead of
unhandled-rejection noise.
In `@version-manifest.json`:
- Around line 1-245: version-manifest.json was modified by CI formatters
(prettier and end-of-file-fixer); re-run the same formatter toolchain locally
(prettier with project config and the repo's EOF fixer), apply the changes to
version-manifest.json, verify the file ends with a single newline and matches
the CI output, then commit the updated version-manifest.json so the workflow
passes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/update-version-manifest.yml:
- Line 38: The workflow step currently appends "|| true" to the uvx prek command
("uvx prek --files version-manifest.json || true"), which silences failures;
remove the "|| true" suffix so the job fails on formatter errors, or replace it
with an explicit check that runs the command and logs a warning/error when it
exits non-zero (capture exit status of "uvx prek --files version-manifest.json"
and use a conditional to fail the step or print diagnostic output) to ensure
formatting failures are surfaced.
b897158 to
1ecd1d8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/update-version-manifest.yml:
- Around line 3-7: The workflow currently includes a broad pull_request trigger
which causes the update-version-manifest job to run on every PR; either remove
the pull_request trigger or narrow it to avoid unintended runs — for example
replace the top-level pull_request with a restricted form (e.g., pull_request:
paths: ["path/to/manifest/**"] or pull_request: types: [opened, reopened,
synchronize]) or move the PR-based testing to a separate workflow; update the
on: block in .github/workflows/update-version-manifest.yml to implement the
chosen restriction so the manifest update script only runs when intended.
In `@src/update-version-manifest.ts`:
- Around line 78-98: The updateManifest function builds manifest entries from
downloadUrls using getPlatform and getVersion then writes to MANIFEST_FILE; make
the output deterministic by sorting the manifest array before writing—e.g., sort
by tag (descending), then platform, then arch—so change updateManifest to
populate the manifest as now, then perform a stable sort on the manifest entries
(compare ManifestEntry.tag, then ManifestEntry.platform, then
ManifestEntry.arch) and only then call fs.writeFile(MANIFEST_FILE,
JSON.stringify(manifest)); keep existing fields and debug logging via
core.debug.
- Around line 54-76: getPlatform and getVersion are synchronous helpers but are
declared async and return Promises; remove the async keyword and change their
signatures to return plain types (getPlatform(): [string,string,boolean] and
getVersion(): string), keep their internal regex logic unchanged, and then
update all callers (e.g., in updateManifest) to stop awaiting their results and
use the returned tuple/string directly (adjust destructuring where used).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/update-version-manifest.ts`:
- Around line 54-61: getPlatform currently returns platform and arch verbatim
from the asset name which yields mixed-case keys; update getPlatform to
normalize the returned strings (e.g., convert to lower-case) and apply a small
canonical mapping for common variants (e.g., map "x86"/"X64"/"X86_64" -> "x64",
"aarch64"/"arm64"/"arm" -> "arm64") while still returning the same boolean for
debug; modify the return of getPlatform to use these normalized/canonical values
for platformMatch[2] and platformMatch[3] so manifest keys stay consistent.
This reverts commit 1ecd1d8.
denialhaag
force-pushed
the
known-versions
branch
from
12e5cb4 to
fd2e4dd
Compare
burgholzer
requested changes
Feb 3, 2026
Member
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Right now the added code is completely untested and duplicates a lot of the logic that we use in
get-download-link.ts.I have completely rewritten the code, and the action now uses the manifest. I have also added a test.
Very nice! This indeed looks a lot better and cleaner now.
One thing to keep in mind: By relying on the manifest, we would currently have to release a new version of
setup-mlirwhenever the manifest is updated. There are workarounds; we could always retrieve the manifest frommain.
Good point. I tried to check how this works in https://github.com/astral-sh/setup-uv/blob/main/src/download/download-version.ts#L143 and at least my conclusion would be that this is how the setup-uv action works as well. I don't particularly think we need a workaround for this. I would hope that the versions are not changing that often anyway.
And it's a nice way to keep a semi-regular release cadence here.
- the PR should probably only be created when the workflow is running on main. This should be straight forward to add.
This should already be the case as the workflow only runs on
main.
Yeah, indeed. I was just confused because the PR was already created; but I figure this was while testing.
- the Installation scripts could be similarly updated to make use of the manifest file. This could easily be deferred to a follow-up PR.
I agree that this would be useful, but I'd be in favor of deferring this to a follow-up PR.
Yeah. +1 from my side for that.
- Should we add some pointer in the documentation that lists the available versions or points to the file? Could we autogenerate a list of versions programmatically from the manifest that gets populated in the readme as part of the update PRs?
I have added an auto-generated list to the README. 👍
That's very nice 👍🏼
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Line 45: The thrown Error message uses a template literal though it contains
no interpolations; replace the template literal with a plain string literal by
updating the throw new Error(`Debug builds are only available on Windows.`)
expression to use regular quotes (e.g., "Debug builds are only available on
Windows.") so the Error construction in src/index.ts uses a simple string
instead of a template literal.
In `@src/utils/constants.ts`:
- Around line 1-33: The MANIFEST_FILE constant is duplicated in
src/utils/download.ts; remove the local MANIFEST_FILE definition there and
import the shared MANIFEST_FILE from src/utils/constants.ts instead to
centralize the path. Update the import list in download.ts to include
MANIFEST_FILE, delete the duplicate __filename/__dirname/join logic in
download.ts, and ensure all uses reference the imported MANIFEST_FILE symbol so
path resolution remains ESM-compatible.
In `@src/utils/download.ts`:
- Line 34: The duplicate constant MANIFEST_FILE defined in download.ts should be
removed and the existing exported MANIFEST_FILE from the central constants
module should be imported and used instead; replace the local declaration of
MANIFEST_FILE with an import of MANIFEST_FILE from the constants module and
update any references in download.ts to use that imported symbol to keep a
single source of truth.
In `@src/utils/manifest.ts`:
- Around line 138-142: The function updateManifest currently falls back to an
empty string for GITHUB_TOKEN which produces an unauthenticated Octokit
(createOctokit) and risks hitting rate limits; update the function to explicitly
detect a missing process.env.GITHUB_TOKEN and log a clear warning (via the same
logger used in this module or console.warn) before creating the octokit client,
and optionally proceed or fail fast depending on policy—reference the
GITHUB_TOKEN env var, the createOctokit call, and the octokit variable so the
change is applied where the client is instantiated.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/utils/download.ts`:
- Around line 37-59: getManifestEntry currently uses case-sensitive startsWith
to match versions causing uppercase commit hashes to miss entries; update the
comparison in getManifestEntry so both sides are normalized (e.g.,
toLowerCase()) before calling startsWith (use
entry.version.toLowerCase().startsWith(version.toLowerCase())) while leaving
other checks (platform, architecture, debug) unchanged; locate usage in the
getManifestEntry function and ensure ManifestEntry/MANIFEST_FILE references
remain intact.
In `@src/utils/manifest.ts`:
- Around line 170-204: The current loop uses a Set named versions to deduplicate
only by version which causes skipping of valid platform/arch/debug assets from
older releases; change deduplication to a per-asset key (e.g., combine version,
platform, architecture, debug) and use that Set (e.g., seenKeys) so each asset
is checked/added individually inside the inner loop; in the block that parses
asset metadata via getMetadata(asset.name) and
getVersionFromAssetName(asset.name) build the compound key and only continue
when that key is already seen, otherwise push the ManifestEntry and add the
compound key to the Set (do not only add version once per release).
burgholzer
requested changes
Feb 4, 2026
Member
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Great progress here! This is starting to feel really good.
Some more work on the zstd handling then this should be good to go.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
37-37:⚠️ Potential issue | 🔴 CriticalRemove or make
tokenoptional.The
tokeninput is retrieved on line 37 withrequired: truebut is never used anywhere in the code. Bothtc.downloadToolcalls pass only the URL without authentication headers. Since the manifest-based approach resolves URLs directly viagetMLIRUrlandgetZstdUrl, the token is not needed. Either remove this input or change it to{ required: false }if needed for backward compatibility.
🤖 Fix all issues with AI agents
In `@src/utils/manifest.ts`:
- Around line 263-266: The versions set may miss versions because
versions.add(version) is executed after the inner asset loop; move the call
inside the per-asset try block immediately after the successful push to manifest
(the block that handles parsing each asset) so that whenever you successfully
parse and push an asset (refer to the variables version and manifest and the
try/catch around asset handling), you also add that specific version to the
versions Set used for README generation.
67d60a6 to
e45c4f9
Compare
Contributor
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 (2)
src/index.ts (1)
100-133:⚠️ Potential issue | 🟠 MajorDon’t fail extraction on acceptable zstd SIGPIPE.
The comment explicitly allows zstd to exit non‑zero when tar closes early, but the current handler rejects on any non‑zero exit. This can fail successful extractions. Track tar success and ignore SIGPIPE (or only reject zstd when tar hasn’t succeeded).
🛠️ Suggested fix
- await new Promise<void>((resolve, reject) => { + await new Promise<void>((resolve, reject) => { + let tarSucceeded = false; const zstd = spawn(zstdPath, ["-d", file, "--long=30", "--stdout"]); const tar = spawn("tar", ["-x", "-f", "-", "-C", extractedDir]); @@ - tar.on("close", (code) => { + tar.on("close", (code) => { if (code !== 0) { reject(new Error(`tar exited with code ${code}`)); } else { + tarSucceeded = true; resolve(); } }); - zstd.on("close", (code) => { - if (code !== 0) { - reject(new Error(`zstd exited with code ${code}`)); - } - }); + zstd.on("close", (code, signal) => { + if (!tarSucceeded && code !== 0 && signal !== "SIGPIPE") { + reject(new Error(`zstd exited with code ${code}`)); + } + }); });__tests__/integration.test.ts (1)
174-188:⚠️ Potential issue | 🟡 MinorMake the architecture assertion case‑insensitive.
Other tests and the manifest use lowercase identifiers (
x86,aarch64), so expecting"X86"in the asset name is likely brittle. Prefer a case‑insensitive check or matchx86_64.🛠️ Suggested fix
- expect(asset.name).toContain("X86"); + expect(asset.name.toLowerCase()).toContain("x86");
🤖 Fix all issues with AI agents
In `@__tests__/update-known-versions.test.ts`:
- Around line 143-145: Update the test assertion that checks zstd asset names to
use the updater-recognized "zstd-" prefix instead of "zstd_": modify the regex
used in the expect for entry.zstd_asset_name (the toMatch call) to start with
"zstd-" (e.g., /^zstd-.*\.(zip|tar\.gz)$/) so the test aligns with the updater's
asset naming convention.
In `@src/utils/create-oktokit.ts`:
- Around line 20-28: The JSDoc says the token is optional but the function
signature requires a string; update consistency by making the parameter optional
and clarifying the JSDoc: change the createOctokit function parameter to accept
token?: string (or token: string | undefined) and update the JSDoc `@param` to
state the token is optional, leaving the existing behavior that an
empty/undefined token results in Octokit being constructed with no auth;
reference createOctokit and OctokitOptions in the change.
- Around line 25-28: Rename the file src/utils/create-oktokit.ts to
src/utils/create-octokit.ts so the filename matches the exported function
createOctokit and the Octokit library naming; then update the import in
src/utils/manifest.ts from "./create-oktokit.js" to "./create-octokit.js" (or
corresponding extension) to reference the renamed module and avoid import
errors.
In `@src/utils/platform.ts`:
- Around line 23-63: Update getPlatform and getArchitecture to perform
case-insensitive validation and return lowercase keys to match manifest lookup:
accept inputs in any case, compare after lowercasing against the canonical
lowercase sets ("host","linux","macos","windows") for getPlatform and
("host","x86","aarch64") for getArchitecture, and when resolving "host" call
determinePlatform()/determineArchitecture() and lowercase their results before
returning; ensure any hardcoded "macOS"/"X86"/"AArch64" checks are replaced with
the lowercase equivalents and all returns are lowercase.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/manifest.ts`:
- Around line 204-222: The loop that populates latestZstdInfo iterates all
releases and allows older releases to overwrite newer zstd entries; change the
assignment logic in the release/assets loop (the block that handles
asset.name.match(...) and sets
latestZstdInfo[`asset_name_${platform}_${architecture}`] and
[`download_url_${platform}_${architecture}`]) so that you only set each key when
it is not already defined (e.g., check for undefined before assigning), ensuring
the first (newest) occurrence wins; keep the same key names and type assertions
used currently.
burgholzer
approved these changes
Feb 4, 2026
Member
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
This is looking really great now. 👍🏼
There's one open CodeRabbit comment (and maybe one hidden one), but that should really be it 🎉
Then this should be ready to go 🚀
Collaborator
Author
I have addressed the final comment, and CodeRabbit didn't raise any further issues. Merging this now! 🎉 |
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
2 participants
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.
This PR adds a
version_manifest.jsonthat lists all LLVM/MLIR versions that can be installed. The version manifest is updated automatically via a workflow.Given 1ecd1d8, the workflow successfully created #80.