fix(ci): merge macOS auto-update manifests for multi-arch releases#2300
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a composite GitHub Action to download and merge arm64/x64 macOS update manifests, updates stable and canary release workflows to invoke it, improves arch extraction for dmg/mac.zip/AppImage artifacts, and centralizes mac manifest stabilization under the new action. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow as Release Workflow
participant Action as merge-mac-manifests Action
participant Artifacts as Artifact Storage
Workflow->>Action: invoke (artifact-prefix, artifacts-path, extra-manifest-names)
Action->>Artifacts: download <prefix>-mac-arm64-update-manifest
Action->>Artifacts: download <prefix>-mac-x64-update-manifest
Action->>Action: locate arm64 and x64 *-mac.yml
alt both manifests exist
Action->>Action: merge (arm64 base + x64 files)
else only one exists
Action->>Action: copy sole manifest as latest-mac.yml
end
Action->>Artifacts: write artifacts-path/latest-mac.yml
alt extra-manifest-names provided
Action->>Artifacts: copy latest-mac.yml -> artifacts-path/<extra-names>
end
Action->>Workflow: output final latest-mac.yml content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
The x64 macOS build omits the arch suffix from filenames (e.g., Superset-1.1.4.dmg instead of Superset-1.1.4-x64.dmg), which caused the stable-copy script to cp a file onto itself, crashing the release. Additionally, both arch builds produce their own latest-mac.yml but merge-multiple: true caused one to overwrite the other, breaking auto-update for whichever arch lost. Changes: - Download each arch's manifest separately and merge them with yq, using arm64 as the canonical base - Detect version-number-as-arch in stable copy naming, default to x64 - Remove redundant mac manifest copy step (merge handles it) - Revert desktop version bump (release will be re-triggered after fix)
Kitenite
changed the title
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release-desktop.yml:
- Around line 73-96: The script assumes one of ARM64 or X64 manifests exists but
doesn’t handle the case when neither does; add a guard after the existing
if/elif chain (or before the final cat "$OUTPUT") that checks whether "$OUTPUT"
exists (or that at least one of "$ARM64" or "$X64" exists), and if not echo a
clear error message (include the variable names ARM64 and X64 in the message)
and exit with a non‑zero status so the workflow fails fast instead of hitting
cat "$OUTPUT" with a confusing error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f68e0a68-6dbf-4cb0-8d16-b86937314bb7
📒 Files selected for processing (1)
.github/workflows/release-desktop.yml
Comment on lines
+73
to
+96
| - name: Merge macOS auto-update manifests | ||
| run: | | ||
| ARM64="manifests/arm64/latest-mac.yml" | ||
| X64="manifests/x64/latest-mac.yml" | ||
| OUTPUT="release-artifacts/latest-mac.yml" | ||
|
|
||
| if [[ -f "$ARM64" && -f "$X64" ]]; then | ||
| # Merge both arch manifests — arm64 is canonical (base path/sha512) | ||
| yq eval-all ' | ||
| select(fileIndex == 0) * | ||
| {"files": [select(fileIndex == 0).files[], select(fileIndex == 1).files[]]} | ||
| ' "$ARM64" "$X64" > "$OUTPUT" | ||
| echo "Merged arm64 + x64 manifests into latest-mac.yml" | ||
| elif [[ -f "$ARM64" ]]; then | ||
| cp "$ARM64" "$OUTPUT" | ||
| echo "Using arm64-only manifest" | ||
| elif [[ -f "$X64" ]]; then | ||
| cp "$X64" "$OUTPUT" | ||
| echo "Using x64-only manifest" | ||
| fi | ||
|
|
||
| echo "Final latest-mac.yml:" | ||
| cat "$OUTPUT" | ||
|
|
Contributor
There was a problem hiding this comment.
Add error handling when neither manifest exists.
If both download steps fail (or with continue-on-error: true), neither manifest file will exist. The script silently proceeds and then fails at cat "$OUTPUT" with a confusing error.
Suggested change
elif [[ -f "$X64" ]]; then
cp "$X64" "$OUTPUT"
echo "Using x64-only manifest"
+ else
+ echo "::error::No macOS update manifest found for either architecture"
+ exit 1
fi
echo "Final latest-mac.yml:"
cat "$OUTPUT"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Merge macOS auto-update manifests | |
| run: | | |
| ARM64="manifests/arm64/latest-mac.yml" | |
| X64="manifests/x64/latest-mac.yml" | |
| OUTPUT="release-artifacts/latest-mac.yml" | |
| if [[ -f "$ARM64" && -f "$X64" ]]; then | |
| # Merge both arch manifests — arm64 is canonical (base path/sha512) | |
| yq eval-all ' | |
| select(fileIndex == 0) * | |
| {"files": [select(fileIndex == 0).files[], select(fileIndex == 1).files[]]} | |
| ' "$ARM64" "$X64" > "$OUTPUT" | |
| echo "Merged arm64 + x64 manifests into latest-mac.yml" | |
| elif [[ -f "$ARM64" ]]; then | |
| cp "$ARM64" "$OUTPUT" | |
| echo "Using arm64-only manifest" | |
| elif [[ -f "$X64" ]]; then | |
| cp "$X64" "$OUTPUT" | |
| echo "Using x64-only manifest" | |
| fi | |
| echo "Final latest-mac.yml:" | |
| cat "$OUTPUT" | |
| - name: Merge macOS auto-update manifests | |
| run: | | |
| ARM64="manifests/arm64/latest-mac.yml" | |
| X64="manifests/x64/latest-mac.yml" | |
| OUTPUT="release-artifacts/latest-mac.yml" | |
| if [[ -f "$ARM64" && -f "$X64" ]]; then | |
| # Merge both arch manifests — arm64 is canonical (base path/sha512) | |
| yq eval-all ' | |
| select(fileIndex == 0) * | |
| {"files": [select(fileIndex == 0).files[], select(fileIndex == 1).files[]]} | |
| ' "$ARM64" "$X64" > "$OUTPUT" | |
| echo "Merged arm64 + x64 manifests into latest-mac.yml" | |
| elif [[ -f "$ARM64" ]]; then | |
| cp "$ARM64" "$OUTPUT" | |
| echo "Using arm64-only manifest" | |
| elif [[ -f "$X64" ]]; then | |
| cp "$X64" "$OUTPUT" | |
| echo "Using x64-only manifest" | |
| else | |
| echo "::error::No macOS update manifest found for either architecture" | |
| exit 1 | |
| fi | |
| echo "Final latest-mac.yml:" | |
| cat "$OUTPUT" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release-desktop.yml around lines 73 - 96, The script
assumes one of ARM64 or X64 manifests exists but doesn’t handle the case when
neither does; add a guard after the existing if/elif chain (or before the final
cat "$OUTPUT") that checks whether "$OUTPUT" exists (or that at least one of
"$ARM64" or "$X64" exists), and if not echo a clear error message (include the
variable names ARM64 and X64 in the message) and exit with a non‑zero status so
the workflow fails fast instead of hitting cat "$OUTPUT" with a confusing error.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Same manifest merge fix as the stable workflow — download each arch's manifest separately and merge with yq so canary auto-update works for both arm64 and x64. Also removes the mac manifest copy step since the merge step now produces both canary-mac.yml and latest-mac.yml.
Both stable and canary release workflows now use the same .github/actions/merge-mac-manifests composite action instead of duplicating the download + yq merge logic inline.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/actions/merge-mac-manifests/merge-mac-manifests.mjs">
<violation number="1" location=".github/actions/merge-mac-manifests/merge-mac-manifests.mjs:205">
P1: Dynamically serialize all `files` properties to avoid dropping critical update metadata.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| for (const [key, value] of Object.entries(manifest)) { | ||
| if (key === "files") { | ||
| lines.push("files:"); | ||
| for (const file of value) { |
Contributor
There was a problem hiding this comment.
P1: Dynamically serialize all files properties to avoid dropping critical update metadata.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/actions/merge-mac-manifests/merge-mac-manifests.mjs, line 205:
<comment>Dynamically serialize all `files` properties to avoid dropping critical update metadata.</comment>
<file context>
@@ -0,0 +1,274 @@
+ for (const [key, value] of Object.entries(manifest)) {
+ if (key === "files") {
+ lines.push("files:");
+ for (const file of value) {
+ lines.push(` - url: ${yamlQuote(file.url)}`);
+ lines.push(` sha512: ${yamlQuote(file.sha512)}`);
</file context>
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
Superset-1.1.4.dmg→cpsame-file error)latest-mac.ymlmanifests so auto-update works for both architectures (previouslymerge-multiple: truecaused one to overwrite the other)Details
electron-builder omits the arch suffix for x64 macOS builds (the historical "default" arch). The stable-copy script extracted the version number as the arch, producing
cp 'Superset-1.1.4.dmg' 'Superset-1.1.4.dmg'.For auto-update, each arch build generates its own
latest-mac.yml. Withmerge-multiple: true, one overwrites the other nondeterministically. Now we download each manifest separately and merge withyq, using arm64 as the canonical base (path/sha512).Test plan
latest-mac.ymlcontains entries for both arm64 and x64Superset-arm64.dmg,Superset-x64.dmg)Summary by CodeRabbit