Improve SDL2 bundling and add SDL3 support for macOS#7549
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe macOS Companion packaging now bundles SDL2 and optionally SDL3 with corrected embedded paths. The macOS build guide updates Homebrew, toolchain, SDL, Python environment, CMake configuration, build, runtime, and packaging instructions. ChangesMacOS Build Support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CMake
participant SDL2SDL3
participant Frameworks
participant InstallNameTool
CMake->>SDL2SDL3: Resolve imported targets or dylib paths
SDL2SDL3->>Frameworks: Install SDL2 and optional SDL3 dylibs
CMake->>InstallNameTool: Rewrite packaged dylib identifiers
InstallNameTool->>Frameworks: Update SDL2 and SDL3 -id values
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
companion/src/CMakeLists.txt (1)
597-610: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBrew-prefix candidate path is structurally wrong due to REALPATH symlink resolution.
_sdl2_realwas resolved viaREALPATH(line 582), which follows Homebrew'sopt/<pkg>symlink into the underlyingCellar/<pkg>/<version>directory. Walking up three directories from that resolved path (lines 598-600) lands onCellar/sdl2, not the actual Homebrew prefix (e.g./opt/homebrew). The resulting candidate${_brew_prefix}/sdl3/lib/libSDL3.0.dylibwill therefore never exist, and only the two hardcoded fallback paths (which assume default Homebrew locations) can ever succeed — meaning custom Homebrew--prefixinstalls get no working candidate at all. Combined with the target-name casing bug above, SDL3 discovery here is effectively 100% reliant on the two hardcoded, version-suffix-pinned fallback strings.Consider using
find_library()with Homebrew's actual prefix (e.g. viaexecute_process(COMMAND brew --prefix sdl3 ...)orHINTS ENV HOMEBREW_PREFIX) instead of manually reconstructing paths from a resolved symlink target.🤖 Prompt for 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. In `@companion/src/CMakeLists.txt` around lines 597 - 610, Fix SDL3 discovery in the NOT _sdl3_real branch without deriving the Homebrew prefix from the REALPATH-resolved _sdl2_real path. Use find_library() with a Homebrew prefix obtained from brew --prefix sdl3 or an equivalent HOMEBREW_PREFIX hint, while preserving the existing fallback candidates and assigning the resolved result to _sdl3_real.docs/building/macos-sequoia.md (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language identifiers to fenced code blocks.
markdownlint (MD040) flags these shell code fences as missing a language tag.
📝 Proposed fix
-``` +```bash /bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)"(apply the same `bash` tag to the fences at lines 43, 73, 82, 88, and 94) </details> Also applies to: 43-43, 73-73, 82-82, 88-88, 94-94 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/building/macos-sequoia.mdat line 7, Update every shell command fenced
code block in macos-sequoia.md, including the blocks near the Homebrew command
and the referenced sections, to use the bash language identifier. Ensure no
affected fenced block remains untagged so markdownlint MD040 passes.</details> <!-- cr-comment:v1:23cd07e4e817d3047a7adbc0 --> _Source: Linters/SAST tools_ </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@companion/src/CMakeLists.txt:
- Around line 591-596: Update the target check in the SDL3 library-location
resolution block to use the exported target name SDL3::SDL3-shared instead of
SDL3::SDL3-Shared, while preserving the existing IMPORTED_LOCATION and REALPATH
handling.- Around line 620-626: Update the install(CODE) block in
companion/src/CMakeLists.txt to capture RESULT_VARIABLE and ERROR_VARIABLE for
each install_name_tool execute_process call, then fail the installation with the
captured error when either command fails. Apply the same handling to the
conditional libSDL3.dylib call while preserving the existing install-name
arguments and component.
Nitpick comments:
In@companion/src/CMakeLists.txt:
- Around line 597-610: Fix SDL3 discovery in the NOT _sdl3_real branch without
deriving the Homebrew prefix from the REALPATH-resolved _sdl2_real path. Use
find_library() with a Homebrew prefix obtained from brew --prefix sdl3 or an
equivalent HOMEBREW_PREFIX hint, while preserving the existing fallback
candidates and assigning the resolved result to _sdl3_real.In
@docs/building/macos-sequoia.md:
- Line 7: Update every shell command fenced code block in macos-sequoia.md,
including the blocks near the Homebrew command and the referenced sections, to
use the bash language identifier. Ensure no affected fenced block remains
untagged so markdownlint MD040 passes.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `61bb11b5-a644-4808-bbf1-68c18d314fd3` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f3cabd42f7cd05044fd7af09a36a2aa9c5bb39c8 and b27b32ce7ac4c2d14006db220cb92f8bced2c747. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `companion/src/CMakeLists.txt` * `docs/building/macos-sequoia.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/building/macos-sequoia.md (1)
122-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language identifiers to new fenced code blocks.
markdownlint flags MD040 on all five newly added code blocks (lines 122, 138, 145, 153, 163) since none specify a language.
📝 Proposed fix
- ``` + ```bash source ../.venv/bin/activate(apply the same
bashtag to the fences at lines 138, 145, 153, and 163)Also applies to: 138-138, 145-145, 153-153, 163-163
🤖 Prompt for 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. In `@docs/building/macos-sequoia.md` at line 122, Add the bash language identifier to each of the five newly added fenced code blocks in the macOS Sequoia build instructions, including the blocks around the commands at lines 122, 138, 145, 153, and 163, while leaving their command contents unchanged.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@docs/building/macos-sequoia.md`:
- Line 122: Add the bash language identifier to each of the five newly added
fenced code blocks in the macOS Sequoia build instructions, including the blocks
around the commands at lines 122, 138, 145, 153, and 163, while leaving their
command contents unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 99f8c5de-57b0-4bc7-9b1d-9df772dc45a4
📒 Files selected for processing (2)
companion/src/CMakeLists.txtdocs/building/macos-sequoia.md
🚧 Files skipped from review as they are similar to previous changes (1)
- companion/src/CMakeLists.txt
Tozapid
left a comment
Tozapid
left a comment
There was a problem hiding this comment.
I hope I fixed everything the bots pointed out
2 participants
Summary of changes:
I ran into some issues building EdgeTX Companion for macOS Tahoe, so I updated the documentation a bit. Now the build works if you simply follow the instructions.
I also added a section on building the .dmg package.
Summary by CodeRabbit
Bug Fixes
Documentation
uv-based Python setup, and reworked build and DMG packaging workflow details.