Make universal2 wheels on macos #3625
Conversation
📝 WalkthroughWalkthroughPer-arch macOS builds now output Changes
Sequence DiagramsequenceDiagram
actor CI as CI Trigger
participant Build as Per-arch Build Jobs (x86_64, arm64)
participant Store as Artifact Store
participant Univ as universal2 Job
participant Delocate as delocate
participant Env as Test Environment
CI->>Build: start per-arch macOS builds
Build->>Store: upload `macos-partial-<arch>` artifacts
CI->>Univ: trigger universal2 job (after builds)
Univ->>Store: download `macos-partial-*` artifacts
Store-->>Univ: deliver per-arch wheel files
Univ->>Delocate: merge arm64 + x86_64 -> universal2 (or copy single-arch)
Delocate-->>Univ: produce universal wheels in wheelhouse
Univ->>Env: install one merged wheel and run pygame tests (sanity check)
Env-->>Univ: test result
Univ->>Store: upload `pygame-wheels-macos` universal artifacts (zero compression)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks✅ Passed checks (5 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 |
ankith26
force-pushed
the
ankith26-mac-universal2
branch
from
ad3fa54 to
78b142e
Compare
ankith26
force-pushed
the
ankith26-mac-universal2
branch
5 times, most recently
from
7258ddc to
6914f31
Compare
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build-macos.yml(2 hunks).github/workflows/release-gh-draft.yml(1 hunks)
🔇 Additional comments (5)
.github/workflows/build-macos.yml (4)
73-74: LGTM!The deployment target configuration is correct: 10.11 for x86_64 (El Capitan) and 11.0 for arm64 (Big Sur, which is when Apple Silicon support was introduced).
115-119: LGTM!Good naming convention using
macos-partial-*for intermediate artifacts. This clearly distinguishes them from the finalpygame-wheels-*artifacts and ensures they won't be accidentally included in the release workflow.
170-174: LGTM!Final artifact naming
pygame-wheels-macoscorrectly matches thepygame-wheels-*pattern expected by the release workflow.
130-155: Well-structured merge logic with proper error handling.The script correctly:
- Uses
set -euo pipefailfor strict error handling- Uses
shopt -s nullglobto avoid literal glob patterns when no matches exist- Handles edge cases where one architecture's wheel is missing
- The wheel filename parsing with
cut -d- -f1-4correctly extracts the package-version-python-abi prefixThe output filenames are handled correctly—delocate-merge automatically generates the merged wheel name with no need to perform further changes, so the script appropriately delegates naming responsibility to the tool rather than attempting manual validation.
.github/workflows/release-gh-draft.yml (1)
31-36: Good addition of pattern filter to exclude intermediate artifacts.The
pygame-wheels-*pattern correctly filters out the intermediatemacos-partial-*artifacts while including all final wheel distributions:pygame-wheels-windows-*,pygame-wheels-manylinux-*,pygame-wheels-sdist, andpygame-wheels-macos.
ankith26
force-pushed
the
ankith26-mac-universal2
branch
from
6914f31 to
ef068bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/build-macos.yml (1)
125-129: Consider adding an explicit pattern filter for artifact download.Using
merge-multiple: truewithout apatternwill download all artifacts from the workflow run. While this works currently since onlymacos-partial-*artifacts exist, it could become fragile if other jobs add artifacts in the future.🔎 Proposed fix
- name: Download artifacts uses: actions/download-artifact@v7 with: + pattern: macos-partial-* merge-multiple: true
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build-macos.yml(2 hunks).github/workflows/release-gh-draft.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/release-gh-draft.yml
⏰ 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). (9)
- GitHub Check: arm64
- GitHub Check: x86_64
- GitHub Check: dev-check
- GitHub Check: x86
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: i686
- GitHub Check: AMD64
- GitHub Check: aarch64
- GitHub Check: x86_64
🔇 Additional comments (4)
.github/workflows/build-macos.yml (4)
73-74: LGTM!The conditional deployment target correctly sets 10.11 for x86_64 (supporting older Intel Macs) and 11.0 for arm64 (Apple Silicon minimum).
117-120: LGTM!The artifact renamed to
macos-partial-*clearly indicates these are intermediate artifacts for the universal2 merge step.
130-155: Well-structured merge script with proper error handling.The script correctly handles:
- Merging matching arm64/x86_64 wheel pairs with
delocate-merge- Copying single-arch wheels as fallback when counterpart is missing
- Using
set -euo pipefailandnullglobfor robustness
168-172: LGTM!The final artifact name
pygame-wheels-macoscorrectly matches thepygame-wheels-*pattern used by the release workflow for artifact merging.
|
Very cool! I like this strategy. We already depend on delocate through CIBW iirc, so we're just depending on delocate working here too. Is it worth pinning the delocate version? I would like to get someone on MacOS to confirm this works before merging, as an additional sanity check. Edit: asked on discord - https://discord.com/channels/772505616680878080/772505616680878083/1455098234631622761 |
ankith26
force-pushed
the
ankith26-mac-universal2
branch
from
ef068bd to
34fab54
Compare
|
I updated the commit by adding a version pin to delocate |
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/build-macos.yml (1)
104-104: Cache key missing OS suffix for consistency.The cache key here is missing the
-${{ matrix.os }}suffix that's present in the deps job (line 40). While both jobs currently usemacos-15, this inconsistency could lead to cache collisions if OS versions diverge in the matrix in the future.🔎 Proposed fix
- key: macdep-${{ hashFiles('buildconfig/manylinux-build/**') }}-${{ hashFiles('buildconfig/macdependencies/*.sh') }}-${{ matrix.macarch }} + key: macdep-${{ hashFiles('buildconfig/manylinux-build/**') }}-${{ hashFiles('buildconfig/macdependencies/*.sh') }}-${{ matrix.macarch }}-${{ matrix.os }}
♻️ Duplicate comments (1)
.github/workflows/build-macos.yml (1)
165-165: Replace--forcewith--force-reinstallin the pip install command.The
--forceflag is not a valid pip option. The correct flag is--force-reinstallto ensure packages are reinstalled during the sanity check.🔎 Proposed fix
- python3 -m pip install --no-index --force --find-links wheelhouse pygame-ce + python3 -m pip install --no-index --force-reinstall --find-links wheelhouse pygame-ce
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build-macos.yml.github/workflows/release-gh-draft.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/release-gh-draft.yml
⏰ 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). (9)
- GitHub Check: arm64
- GitHub Check: x86_64
- GitHub Check: x86
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: AMD64
- GitHub Check: dev-check
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: i686
🔇 Additional comments (1)
.github/workflows/build-macos.yml (1)
135-135: The pinned version0.13.0is the latest available release (as of January 2026) and is appropriate for universal2 wheel building with no known critical issues. The version pinning ensures reproducibility.
.github/workflows/build-macos.yml
Outdated
| for arm in *arm64.whl; do | ||
| x86="$(ls "$(echo "$arm" | cut -d- -f1-4)"-macosx_*_x86_64.whl 2>/dev/null | head -n1 || true)" | ||
| if [[ ! -f "$x86" ]]; then | ||
| echo "Copying arm wheel as is (missing x86 component): $arm" | ||
| mv "$arm" "$OUT_DIR" | ||
| continue | ||
| fi | ||
| echo "Merging:" | ||
| echo " ARM: $arm" | ||
| echo " X86: $x86" | ||
| delocate-merge "$arm" "$x86" -w "$OUT_DIR" | ||
| rm "$x86" |
There was a problem hiding this comment.
Wheel name matching logic is fragile and lacks post-merge validation.
The pattern matching at line 140 uses cut -d- -f1-4 and wildcard expansion to find the corresponding x86_64 wheel. This approach has several reliability concerns:
- If multiple x86_64 wheels match the pattern,
head -n1arbitrarily selects the first, which may not be the correct counterpart. - The wildcard
macosx_*could match unintended wheels if naming conventions vary. - No validation confirms the output is actually a universal2 wheel after
delocate-mergecompletes.
Consider adding validation after the merge (e.g., using delocate-listdeps or checking wheel tags) to confirm universal2 architecture, and tighten the matching logic to ensure deterministic pairing.
🔎 Suggested improvements
Add validation after the merge loop:
# After line 155, before the sanity check
for wheel in "$OUT_DIR"/*.whl; do
if [[ "$wheel" =~ universal2 ]]; then
echo "✓ Validated universal2 wheel: $(basename "$wheel")"
else
echo "⚠ Warning: Non-universal2 wheel found: $(basename "$wheel")"
fi
doneOptionally, make the x86_64 matching more explicit by extracting the full base name pattern instead of relying on field splitting.
🤖 Prompt for AI Agents
.github/workflows/build-macos.yml lines 139-150: the current x86_64 wheel
discovery uses cut + wildcard and head -n1 which can pick the wrong file or none
and there is no post-merge validation; update the pairing logic to
deterministically locate the exact matching x86_64 wheel (e.g., derive the full
expected basename from the ARM wheel using the wheel filename components or a
strict regex and fail if zero or multiple exact matches are found), only call
delocate-merge when a single precise counterpart is identified, verify the merge
succeeded before removing the x86 wheel, and add a post-merge validation step
that inspects the resulting wheel tags (or runs delocate-listdeps/checks) to
confirm it is a universal2 wheel and log or fail on non-universal results.
|
I got a test of this on Mac from Notenlish on discord: https://discord.com/channels/772505616680878080/772505616680878083/1455903851793354823 They report no issues. |
Starbuck5
left a comment
Starbuck5
left a comment
There was a problem hiding this comment.
Ideally we'd get someone to test suite, PyInstaller build with this before releasing it.
Also I would like a comment added on the universal2 step just to lay out in plain text what this is doing and how, for future maintainers.
But overall I'm good to see this merged.
ankith26
force-pushed
the
ankith26-mac-universal2
branch
from
34fab54 to
aa137b9
Compare
|
I added the comment to explain the step at high level. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/build-macos.yml (1)
142-154: Wheel matching logic remains fragile and lacks validation.Line 143's pattern matching (
cut -d- -f1-4+ wildcard +head -n1) has reliability concerns:
- Non-deterministic pairing: If multiple x86_64 wheels match the pattern,
head -n1arbitrarily selects the first, which may not be the correct counterpart for the specific ARM wheel being processed.- Overly broad wildcard:
macosx_*_x86_64.whlcould match unintended wheels if naming conventions vary or if there are wheels from different Python versions or builds in the directory.- No post-merge validation: After
delocate-mergecompletes, there's no verification that the output is actually a universal2 wheel with both architectures present.🔎 Recommended improvements
Tighten the matching logic:
- x86="$(ls "$(echo "$arm" | cut -d- -f1-4)"-macosx_*_x86_64.whl 2>/dev/null | head -n1 || true)" + # Extract the base pattern more precisely (e.g., name-version-pythontag-abitag) + base="$(echo "$arm" | sed 's/-macosx_[^-]*_arm64\.whl$//')" + x86="${base}-macosx_"*"_x86_64.whl" + # Check for exact match + matches=( $x86 ) + if [[ ${#matches[@]} -eq 0 ]]; then + echo "Copying arm wheel as is (missing x86 component): $arm" + mv "$arm" "$OUT_DIR" + continue + elif [[ ${#matches[@]} -gt 1 ]]; then + echo "Error: Multiple x86_64 wheels match $arm: ${matches[*]}" + exit 1 + fi + x86="${matches[0]}"Add post-merge validation:
delocate-merge "$arm" "$x86" -w "$OUT_DIR" rm "$x86" + # Validate the merged wheel is universal2 + merged_wheel="$OUT_DIR/$(basename "$arm" | sed 's/_arm64\.whl$/_universal2.whl/')" + if [[ ! -f "$merged_wheel" ]]; then + echo "Error: Expected universal2 wheel not found: $merged_wheel" + exit 1 + fi + echo "✓ Created universal2 wheel: $(basename "$merged_wheel")" done
🧹 Nitpick comments (2)
.github/workflows/build-macos.yml (2)
160-169: Consider verifying universal2 architecture in sanity check.The sanity check installs and tests pygame-ce but doesn't explicitly verify that a universal2 wheel was installed or that both architectures are present. While the current test confirms basic functionality, you could enhance confidence by checking the wheel architecture.
🔎 Suggested enhancement
run: | + # Verify we're installing a universal2 wheel + wheel_file="$(ls wheelhouse/*universal2.whl | head -n1)" + echo "Installing universal2 wheel: $wheel_file" + python3 -m pip install --no-index --find-links wheelhouse pygame-ce + # Optionally verify the installed package contains universal2 binaries + installed_path="$(python3 -c 'import pygame; print(pygame.__file__)')" + echo "Installed pygame at: $installed_path" python3 -m pip install --no-index --find-links wheelhouse pygame-ce python3 -m pygame.tests -v --exclude opengl,music,timing --time_out 300
138-138: Consider verifying delocate installation.While
pipx installtypically succeeds, adding a quick verification could catch rare installation failures before attempting to merge wheels.🔎 Suggested enhancement
pipx install delocate==0.13.0 + delocate-merge --version || { echo "Error: delocate installation failed"; exit 1; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build-macos.yml.github/workflows/release-gh-draft.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/release-gh-draft.yml
⏰ 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). (9)
- GitHub Check: arm64
- GitHub Check: x86_64
- GitHub Check: dev-check
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: AMD64
- GitHub Check: x86
🔇 Additional comments (4)
.github/workflows/build-macos.yml (4)
73-74: LGTM: Deployment target comment is clear and accurate.The comment correctly explains the conditional MACOSX_DEPLOYMENT_TARGET setting for different architectures.
117-120: LGTM: Artifact naming accurately reflects partial/intermediate status.The name change to
macos-partial-${{ matrix.macarch }}clearly indicates these are intermediate artifacts before universal2 merging.
121-129: LGTM: Universal2 job setup is correct.The job dependency and artifact download configuration properly sets up for wheel merging.
171-175: LGTM: Final artifact upload correctly publishes universal2 wheels.The artifact name
pygame-wheels-macosappropriately represents the final merged universal2 wheels, replacing the previous arch-specific artifacts.
closes #3535