fix: separate arm64 and x64 builds for Apple Silicon support#2
Open
Carme99 wants to merge 2 commits intokudosscience:mainfrom
Open
fix: separate arm64 and x64 builds for Apple Silicon support#2Carme99 wants to merge 2 commits intokudosscience:mainfrom
Carme99 wants to merge 2 commits intokudosscience:mainfrom
Conversation
- Split macOS CI into two runners: macos-latest (x64) and macos-14 (arm64) - Add --x64/--arm64 flag to download-pdfium.js for cross-compilation - Pass arch flag to electron-builder to build only target arch - Update author: Carme99
Reviewer's GuideSeparates macOS CI builds into distinct x64 and arm64 jobs for Apple Silicon support, adds architecture flags to the PDFium download script and electron-builder invocation, and updates the package author metadata. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds macOS x64 and arm64 build matrix entries, propagates architecture flags through CI build and artifact naming, updates PDFium download script to accept arch overrides, and sets package author to "Carme99". Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In the workflow matrix,
archis only defined for the macOS entries, but you unconditionally use${{ matrix.arch }}in theDownload PDFium binariesstep and in the artifact name; this will be undefined for Linux/Windows runs and should either be given a default arch for those entries or the step/path should be made conditional onmatrix.platform == 'mac'. - The
Build installerstep uses the condition${{ matrix.arch == 'mac' && ... }}, butarchis set tox64/arm64whileplatformismac; this condition will never be true, so the arch flag is not passed—update the condition to checkmatrix.platform == 'mac'instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the workflow matrix, `arch` is only defined for the macOS entries, but you unconditionally use `${{ matrix.arch }}` in the `Download PDFium binaries` step and in the artifact name; this will be undefined for Linux/Windows runs and should either be given a default arch for those entries or the step/path should be made conditional on `matrix.platform == 'mac'`.
- The `Build installer` step uses the condition `${{ matrix.arch == 'mac' && ... }}`, but `arch` is set to `x64`/`arm64` while `platform` is `mac`; this condition will never be true, so the arch flag is not passed—update the condition to check `matrix.platform == 'mac'` instead.
## Individual Comments
### Comment 1
<location path=".github/workflows/release.yml" line_range="57" />
<code_context>
- name: Build installer
- run: npm run dist:${{ matrix.platform }}
+ run: npm run dist:${{ matrix.platform }} ${{ matrix.arch == 'mac' && format('-- --{0}', matrix.arch) || '' }}
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
</code_context>
<issue_to_address>
**issue (bug_risk):** GitHub Actions condition likely compares the wrong field (`matrix.arch` vs `matrix.platform`) for mac-specific args
`matrix.arch == 'mac'` looks incorrect. To pass `-- --x64/arm64` only for mac builds, this should probably be `matrix.platform == 'mac' && format('-- --{0}', matrix.arch)`. As written, no matrix entry will have `arch == 'mac'`, so the extra args are never applied.
</issue_to_address>
### Comment 2
<location path=".github/workflows/release.yml" line_range="64" />
<code_context>
uses: actions/upload-artifact@v4
with:
- name: installer-${{ matrix.platform }}
+ name: installer-${{ matrix.platform }}-${{ matrix.arch }}
path: |
release/*.exe
</code_context>
<issue_to_address>
**issue:** Artifact name may include an empty or `null` arch segment for non-mac jobs
Non-mac matrix entries don’t define `arch`, so this will likely produce names like `installer-win-` or `installer-linux-` (or `null`). To avoid inconsistent or confusing artifact names, either add explicit `arch` values for all platforms (e.g., `x64`), or append `-${{ matrix.arch }}` only when it’s defined.
</issue_to_address>
### Comment 3
<location path=".github/workflows/release.yml" line_range="47-48" />
<code_context>
- name: Download PDFium binaries
- run: node scripts/download-pdfium.js
+ run: node scripts/download-pdfium.js --${{ matrix.arch }}
- name: Build native addon
</code_context>
<issue_to_address>
**suggestion:** Passing `--${{ matrix.arch }}` when `arch` is undefined leads to a stray `--` argument
For non-mac runs `matrix.arch` is empty, so this becomes `node scripts/download-pdfium.js --`. While the script ignores the flag, this is misleading and may confuse future maintainers. Consider conditionally adding the flag, e.g. `node scripts/download-pdfium.js ${{ matrix.arch && format('--{0}', matrix.arch) || '' }}`, or using separate steps for mac vs non-mac.
```suggestion
- name: Download PDFium binaries
run: node scripts/download-pdfium.js ${{ matrix.arch && format('--{0}', matrix.arch) || '' }}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 706147e763
ℹ️ 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".
.github/workflows/release.yml
Outdated
|
|
||
| - name: Build installer | ||
| run: npm run dist:${{ matrix.platform }} | ||
| run: npm run dist:${{ matrix.platform }} ${{ matrix.arch == 'mac' && format('-- --{0}', matrix.arch) || '' }} |
There was a problem hiding this comment.
The Build installer command never adds --x64/--arm64 because the guard compares matrix.arch to 'mac', but matrix.arch is set to x64 or arm64 in the mac matrix entries (and unset elsewhere). As written, the expression is always false, so this change does not actually forward the architecture flag and the workflow can still produce mac artifacts without the intended per-arch restriction.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
17-22:⚠️ Potential issue | 🟡 MinorAdd explicit
archto Windows and Linux matrix entries.Windows and Linux entries don't define
arch, causing:
- Line 48:
--${{ matrix.arch }}expands to--(an empty flag)- Line 64: Artifact names become
installer-win-andinstaller-linux-with trailing hyphensWhile the download script happens to fall back to
process.arch, this is implicit and fragile.🔧 Proposed fix
include: - os: windows-latest platform: win + arch: x64 artifact: release/*.exe - os: ubuntu-latest platform: linux + arch: x64 artifact: release/*.AppImage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 17 - 22, The matrix entries for "os: windows-latest" and "os: ubuntu-latest" are missing an explicit arch, which makes "${{ matrix.arch }}" expand empty (producing a stray "--" flag and trailing hyphens in artifact names); update those matrix items by adding an appropriate "arch" key (e.g., arch: x64 or arch: arm64 as required) for the Windows and Linux entries so matrix.arch is defined and artifact names (installer-win-..., installer-linux-...) and the CLI flag (--${{ matrix.arch }}) render correctly.
🤖 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.yml:
- Line 57: The conditional in the release workflow uses the wrong variable:
replace the check matrix.arch == 'mac' with matrix.platform == 'mac' so the
architecture flag is passed for mac builds; update the run command that invokes
npm run dist:${{ matrix.platform }} to use ${{ matrix.platform == 'mac' &&
format('-- --{0}', matrix.arch) || '' }} (keeping matrix.arch for the format
argument) so mac jobs receive the correct -- --{arch} flag.
---
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 17-22: The matrix entries for "os: windows-latest" and "os:
ubuntu-latest" are missing an explicit arch, which makes "${{ matrix.arch }}"
expand empty (producing a stray "--" flag and trailing hyphens in artifact
names); update those matrix items by adding an appropriate "arch" key (e.g.,
arch: x64 or arch: arm64 as required) for the Windows and Linux entries so
matrix.arch is defined and artifact names (installer-win-...,
installer-linux-...) and the CLI flag (--${{ matrix.arch }}) render correctly.
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.
Hi Henry,
Cool looking app! Hope you don't mind me contributing to it!
Summary
macos-latest(Intel/x64) andmacos-14(Apple Silicon/arm64)--x64/--arm64flag todownload-pdfium.jsfor cross-compilationProblem
The current CI builds arm64 DMG on an Intel runner (
macos-latest), downloads Intel PDFium binaries, and packages them into an arm64 app. This results in an "App is damaged" error when users try to install on Apple Silicon Macs.Solution
Use separate runners for each architecture so each build downloads and uses the correct PDFium binaries:
macos-latest→ builds x64 DMG with x64 PDFiummacos-14→ builds arm64 DMG with arm64 PDFiumFiles Changed
.github/workflows/release.yml— split macOS matrix, pass arch flagsscripts/download-pdfium.js— add--x64/--arm64CLI argspackage.json— update authorSummary by Sourcery
Separate macOS release builds by architecture and ensure each build uses matching PDFium binaries for correct Apple Silicon support.
New Features:
Bug Fixes:
Build:
CI:
Chores:
Summary by CodeRabbit
New Features
Chores