build/github: Add iOS TestFlight automation for CI/CD #1990
Conversation
- Update GitHub Actions to build signed iOS IPAs and upload to TestFlight - Build iOS on macos-15 runners with automatic current Xcode selection (now 16.4, iOS 18.5 SDK) - Add iOS signing workflow with certificate and provisioning profile setup - Upload to TestFlight automatically on master/ios-testing branches and release tags - Configure iOS app metadata via repository variables (bundle ID, version, build number) - Fix AppIcon.appiconset with required icon sizes for iOS 18+ validation - Update build/ios.mk to use xcrun with explicit SDK for storyboard compilation - Make darwin/sign.sh use environment variables for CI integration - Build with TESTING=y for branch builds, TESTING=n for release tags
WalkthroughThis PR updates CI macOS runners to macos-15, adds production iOS signing and TestFlight upload steps to the GitHub Actions workflow, parameterizes iOS build/signing settings and scripts, refactors iOS app icon asset entries, and adds a small test include adjustment. Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant KC as Temporary Keychain
participant Build as XCSoar Build
participant Sign as Signer Script (darwin/sign.sh)
participant ASC as App Store Connect (Transporter/TestFlight)
GHA->>KC: Create temp keychain & import certs + provisioning profile
activate KC
KC-->>GHA: Keychain ready
deactivate KC
GHA->>Build: Compile XCSoar (TESTING=n) -> produce unsigned IPA
activate Build
Build-->>GHA: unsigned .ipa
deactivate Build
GHA->>Sign: Invoke signing script with IPA & profile env vars
activate Sign
Sign->>KC: Use imported certs to sign IPA
KC-->>Sign: Certificate access granted
Sign-->>GHA: signed .ipa artifact
deactivate Sign
GHA->>ASC: Upload signed .ipa to TestFlight (App Store Connect)
activate ASC
ASC-->>GHA: Upload result
deactivate ASC
GHA->>KC: Remove temp keychain and cleanup secrets
activate KC
KC-->>GHA: Cleanup complete
deactivate KC
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
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-native.yml (1)
385-420: Fix shellcheck warnings for word splitting.Static analysis flagged multiple instances where
$(nproc)should be quoted to prevent word splitting, though this is unlikely to cause issues in practice sincenprocreturns a single number.For shell script best practices, quote the command substitutions:
- make -j$(nproc) ANDROID_BUNDLE_BUILD="$BUNDLE" TARGET=ANDROIDFAT DEBUG=${{ env.DEBUG }} USE_CCACHE=y V=2 \ + make -j"$(nproc)" ANDROID_BUNDLE_BUILD="$BUNDLE" TARGET=ANDROIDFAT DEBUG=${{ env.DEBUG }} USE_CCACHE=y V=2 \Apply similar quoting to all
$(nproc)instances in this step (lines 395, 404, 406, 408, 410, 412, 414, 419).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build-native.yml(9 hunks)Data/iOS/Assets.xcassets/AppIcon.appiconset/Contents.json(1 hunks)build/ios.mk(1 hunks)darwin/sign.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-20T17:09:46.107Z
Learnt from: yorickreum
Repo: XCSoar/XCSoar PR: 1713
File: darwin/clean.sh:0-0
Timestamp: 2025-07-20T17:09:46.107Z
Learning: In Xcode sandbox environments, the `brew` command is not available until `/opt/homebrew/bin` is added to PATH first, creating a chicken-and-egg problem. For scripts executed by Xcode, hardcoding the Homebrew path (`/opt/homebrew/bin`) is the correct approach rather than using `$(brew --prefix)`.
Applied to files:
.github/workflows/build-native.yml
📚 Learning: 2025-10-19T22:01:13.888Z
Learnt from: yorickreum
Repo: XCSoar/XCSoar PR: 1924
File: darwin/build.sh:65-65
Timestamp: 2025-10-19T22:01:13.888Z
Learning: In the XCSoar darwin/build.sh script, the TESTING="y" flag is intentionally hardcoded for all builds (both Debug and Release). This is a safety feature to prevent pilots from flying with development versions—it gives the app a red icon as a visual warning. TESTING has a different purpose than the DEBUG flag and should remain separate.
Applied to files:
.github/workflows/build-native.yml
🪛 actionlint (1.7.9)
.github/workflows/build-native.yml
385-385: shellcheck reported issue in this script: SC2046:warning:10:10: Quote this to prevent word splitting
(shellcheck)
385-385: shellcheck reported issue in this script: SC2046:warning:19:10: Quote this to prevent word splitting
(shellcheck)
385-385: shellcheck reported issue in this script: SC2046:warning:21:10: Quote this to prevent word splitting
(shellcheck)
385-385: shellcheck reported issue in this script: SC2046:warning:23:10: Quote this to prevent word splitting
(shellcheck)
385-385: shellcheck reported issue in this script: SC2046:warning:25:11: Quote this to prevent word splitting
(shellcheck)
385-385: shellcheck reported issue in this script: SC2046:warning:27:11: Quote this to prevent word splitting
(shellcheck)
385-385: shellcheck reported issue in this script: SC2046:warning:29:11: Quote this to prevent word splitting
(shellcheck)
385-385: shellcheck reported issue in this script: SC2046:warning:34:65: Quote this to prevent word splitting
(shellcheck)
⏰ 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). (7)
- GitHub Check: build (ubuntu-22.04, ANDROID, XCSoar, bin, XCSoar, .apk, true, r26d, debian:bookworm-slim)
- GitHub Check: build (macos-15, IOS64, xcsoar-signed, XCSoar, .ipa, true)
- GitHub Check: build (ubuntu-22.04, ANDROID_BUNDLE, XCSoar, bin, XCSoar, .apk, false, r26d, debian:bookworm-slim)
- GitHub Check: build (macos-15, MACOS, XCSoar, bin, XCSoar, false)
- GitHub Check: build (ubuntu-22.04, WIN64, XCSoar, bin, XCSoar, .exe, true, debian:bookworm-slim)
- GitHub Check: build (ubuntu-22.04, UNIX, xcsoar_7.44_amd64, xcsoar_, .deb, true, debian:bookworm-slim)
- GitHub Check: build (ubuntu-22.04, PC, XCSoar, bin, XCSoar, .exe, true, debian:bookworm-slim)
🔇 Additional comments (11)
darwin/sign.sh (1)
4-10: Environment variable defaults approved with placeholder values noted.The environment variable approach is well-suited for CI integration. The placeholder defaults for
PROFILE_PATHandCERTIFICATE_NAMEare acceptable since these will be overridden in the CI workflow (as seen in.github/workflows/build-native.ymllines 474 and 473).build/ios.mk (2)
55-55: LGTM! Explicit SDK targeting improves build reliability.The use of
xcrun -sdk iphoneoswith explicit target device flags ensures proper storyboard compilation for iOS 18+ validation. This aligns well with the device-specific icon entries added to the asset catalog.
51-51: No issues found with IOS_MIN_SUPPORTED_VERSION usage.The variable is properly defined in
build/targets.mk(lines 264, 278, 293) with appropriate minimum deployment target values for different iOS configurations. The parameterized approach used in line 51 is correct and maintainable..github/workflows/build-native.yml (8)
20-20: LGTM! ios-testing branch added for CI triggers.Adding the
ios-testingbranch to the workflow triggers is appropriate for testing the new iOS automation.
157-172: LGTM! macOS 15 runner upgrade provides iOS 18.5 SDK.The upgrade to
macos-15runners provides Xcode 16.4 with the iOS 18.5 SDK, which is necessary for the iOS 18+ app icon validation requirements. The updated IOS64 configuration correctly points to the signed IPA artifact.
196-200: Good practice: Xcode version verification added.Logging the Xcode version helps with debugging and ensures the expected SDK version is available.
352-381: LGTM! iOS signing setup follows security best practices.The certificate and provisioning profile setup demonstrates good security practices:
- Uses temporary keychain isolated from system keychain
- Properly imports P12 certificate with password
- Sets appropriate partition list for codesign access
- Cleans up sensitive certificate file immediately after import
- Pairs well with the keychain cleanup step at lines 523-526
383-420: LGTM! TESTING flag usage aligns with safety requirements.The use of
TESTING=yfor PR/branch builds (line 414) is correct per the project's safety policy—it ensures development builds show a red icon warning to prevent pilots from flying with test versions. The iOS app configuration environment variables (lines 386-391) properly parameterize the bundle ID, version, and build number.Based on learnings, the TESTING flag is a deliberate safety feature separate from the DEBUG flag.
451-460: LGTM! Production release configuration is correct.The release build correctly uses
TESTING=nto produce a production IPA without the red icon warning. The iOS app configuration is properly parameterized with fallbacks to repository variables.Note: The same shellcheck word splitting issue from the PR build step applies here—consider quoting
$(nproc)on line 460.
469-482: LGTM! IPA signing step includes proper fallback handling.The signing step correctly sets environment variables for
darwin/sign.shand includes a sensible fallback to create an unsigned copy when provisioning profiles are missing. This allows the workflow to complete even in non-production scenarios.
523-526: LGTM! Keychain cleanup ensures security.Using
always()condition ensures the temporary keychain is deleted even if previous steps fail, preventing credential leakage in the CI environment.
| { | ||
| "appearances" : [ | ||
| { | ||
| "appearance" : "luminosity", | ||
| "value" : "dark" | ||
| } | ||
| ], | ||
| "idiom" : "universal", | ||
| "platform" : "ios", | ||
| "size" : "1024x1024" | ||
| "idiom" : "iphone", | ||
| "scale" : "2x", | ||
| "size" : "60x60" | ||
| }, | ||
| { | ||
| "appearances" : [ | ||
| { | ||
| "appearance" : "luminosity", | ||
| "value" : "tinted" | ||
| } | ||
| ], | ||
| "idiom" : "universal", | ||
| "platform" : "ios", | ||
| "size" : "1024x1024" | ||
| "idiom" : "iphone", | ||
| "scale" : "3x", | ||
| "size" : "60x60" | ||
| }, | ||
| { | ||
| "idiom" : "ipad", | ||
| "scale" : "2x", | ||
| "size" : "76x76" | ||
| }, | ||
| { | ||
| "idiom" : "ipad", | ||
| "scale" : "2x", | ||
| "size" : "83.5x83.5" | ||
| } |
There was a problem hiding this comment.
Critical: Missing filenames for device-specific icon entries.
The iPhone and iPad icon entries (lines 10-27) lack "filename" attributes. Without actual image files, iOS will fail validation when uploading to TestFlight or the App Store. iOS 18+ requires these specific sizes:
- iPhone: 60x60 @2x (120x120px) and @3x (180x180px)
- iPad: 76x76 @2x (152x152px) and 83.5x83.5 @2x (167x167px)
Apply this diff to add the required icon filenames:
{
"idiom" : "iphone",
"scale" : "2x",
- "size" : "60x60"
+ "size" : "60x60",
+ "filename" : "Icon-60@2x.png"
},
{
"idiom" : "iphone",
"scale" : "3x",
- "size" : "60x60"
+ "size" : "60x60",
+ "filename" : "Icon-60@3x.png"
},
{
"idiom" : "ipad",
"scale" : "2x",
- "size" : "76x76"
+ "size" : "76x76",
+ "filename" : "Icon-76@2x.png"
},
{
"idiom" : "ipad",
"scale" : "2x",
- "size" : "83.5x83.5"
+ "size" : "83.5x83.5",
+ "filename" : "Icon-83.5@2x.png"
}You'll also need to update build/ios.mk line 48 to generate these additional icon sizes from the SVG:
$(IOS_GRAPHICS_DIR)/Assets.xcassets: $(topdir)/Data/iOS/Assets.xcassets $(IOS_ICON_SVG) | $(IOS_GRAPHICS_DIR)/dirstamp
$(Q)cp -r $< $@
$(Q)rsvg-convert $(IOS_ICON_SVG) -w 1024 -h 1024 -a -b white -o $@/AppIcon.appiconset/Icon-1024.png
+ $(Q)rsvg-convert $(IOS_ICON_SVG) -w 120 -h 120 -a -b white -o $@/AppIcon.appiconset/Icon-60@2x.png
+ $(Q)rsvg-convert $(IOS_ICON_SVG) -w 180 -h 180 -a -b white -o $@/AppIcon.appiconset/Icon-60@3x.png
+ $(Q)rsvg-convert $(IOS_ICON_SVG) -w 152 -h 152 -a -b white -o $@/AppIcon.appiconset/Icon-76@2x.png
+ $(Q)rsvg-convert $(IOS_ICON_SVG) -w 167 -h 167 -a -b white -o $@/AppIcon.appiconset/Icon-83.5@2x.png📝 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.
| { | |
| "appearances" : [ | |
| { | |
| "appearance" : "luminosity", | |
| "value" : "dark" | |
| } | |
| ], | |
| "idiom" : "universal", | |
| "platform" : "ios", | |
| "size" : "1024x1024" | |
| "idiom" : "iphone", | |
| "scale" : "2x", | |
| "size" : "60x60" | |
| }, | |
| { | |
| "appearances" : [ | |
| { | |
| "appearance" : "luminosity", | |
| "value" : "tinted" | |
| } | |
| ], | |
| "idiom" : "universal", | |
| "platform" : "ios", | |
| "size" : "1024x1024" | |
| "idiom" : "iphone", | |
| "scale" : "3x", | |
| "size" : "60x60" | |
| }, | |
| { | |
| "idiom" : "ipad", | |
| "scale" : "2x", | |
| "size" : "76x76" | |
| }, | |
| { | |
| "idiom" : "ipad", | |
| "scale" : "2x", | |
| "size" : "83.5x83.5" | |
| } | |
| { | |
| "idiom" : "iphone", | |
| "scale" : "2x", | |
| "size" : "60x60", | |
| "filename" : "Icon-60@2x.png" | |
| }, | |
| { | |
| "idiom" : "iphone", | |
| "scale" : "3x", | |
| "size" : "60x60", | |
| "filename" : "Icon-60@3x.png" | |
| }, | |
| { | |
| "idiom" : "ipad", | |
| "scale" : "2x", | |
| "size" : "76x76", | |
| "filename" : "Icon-76@2x.png" | |
| }, | |
| { | |
| "idiom" : "ipad", | |
| "scale" : "2x", | |
| "size" : "83.5x83.5", | |
| "filename" : "Icon-83.5@2x.png" | |
| } |
🤖 Prompt for AI Agents
In Data/iOS/Assets.xcassets/AppIcon.appiconset/Contents.json around lines 9 to
28, the iPhone and iPad icon entries are missing "filename" attributes causing
App Store/TestFlight validation failures; add filename entries pointing to the
correctly sized PNG files (e.g., AppIcon-60@2x.png, AppIcon-60@3x.png,
AppIcon-76@2x.png, AppIcon-83.5@2x.png) for the corresponding idiom/size/scale
entries, and then update build/ios.mk at line 48 to generate those PNGs from the
SVG (ensure output filenames match the filenames added to Contents.json and the
generated pixel sizes: 120x120, 180x180, 152x152, 167x167).
There was a problem hiding this comment.
TestFlight does not complain about this.
Apple recommends to only provide a 1024x1024 image here https://developer.apple.com/documentation/xcode/configuring-your-app-icon
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai I think the current approach is fine. I think there is no need to generate the other sizes in our build script.
There was a problem hiding this comment.
@yorickreum, understood! I appreciate the clarification. If TestFlight validation accepts the 1024x1024 universal icon and Apple's official documentation confirms this is the recommended approach, then the current implementation is indeed fine. My initial concern was overly cautious.
✏️ Learnings added
Learnt from: yorickreum
Repo: XCSoar/XCSoar PR: 1990
File: Data/iOS/Assets.xcassets/AppIcon.appiconset/Contents.json:9-28
Timestamp: 2025-12-15T01:43:40.517Z
Learning: For XCSoar iOS builds in Data/iOS/Assets.xcassets/AppIcon.appiconset/Contents.json: A single universal 1024x1024 icon is sufficient for TestFlight validation. Device-specific icon entries (iPhone 60x60, iPad 76x76, etc.) can be specified without filenames, as per Apple's official documentation at https://developer.apple.com/documentation/xcode/configuring-your-app-icon.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: yorickreum
Repo: XCSoar/XCSoar PR: 1714
File: src/ProgressWindow.cpp:0-0
Timestamp: 2025-08-02T01:24:24.662Z
Learning: In src/ProgressWindow.cpp, the iOS-specific code that multiplies text_height by 3 for TARGET_OS_IPHONE was determined to be outdated and not required by the PR author yorickreum.
Learnt from: yorickreum
Repo: XCSoar/XCSoar PR: 1990
File: .github/workflows/build-native.yml:484-521
Timestamp: 2025-12-15T01:39:20.273Z
Learning: For XCSoar project: `xcrun altool --upload-app` is the correct and currently supported method for uploading iOS IPAs to TestFlight. altool was only deprecated for notarization purposes in 2023, but remains fully supported for App Store and TestFlight uploads per Apple's official documentation.
|
So i like this very much. Its my understanding that this pushes every build to testflight. Correct? |
|
It's set to push every commit on the |
|
I'd like the things to be prevented from going out, if it the check ever fails on the master branch. I'm trying to write tests, that are meaningful to the platfrom that they run on. (for example key input handling.) |
Fixes compilation error on macOS when building tests with fmt 8.0+. The std::back_inserter function requires <iterator> to be included.
Add test gate before signing and uploading iOS builds to TestFlight. Runs MACOS target tests on the build runner to catch logic errors before distribution. Uses continue-on-error to ensure cleanup runs even if tests fail. Only signs and uploads if tests pass.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/build-native.yml (1)
469-476: Consider running tests for the IOS64 target instead of MACOS.The test step runs
gmake TARGET=MACOS checkrather thanTARGET=IOS64 check. This tests the macOS build, not the iOS build that will be signed and uploaded. While this provides some test coverage, it doesn't validate the actual iOS binary being distributed.If
make checkdoesn't support IOS64 (due to simulator requirements or other constraints), consider adding a comment explaining why MACOS tests are used as a proxy for iOS validation.- - name: Run tests before signing + - name: Run tests before signing (MACOS proxy for iOS validation) id: tests if: ${{ matrix.target == 'IOS64' && (github.ref == 'refs/heads/master' || github.ref == 'refs/heads/ios-testing' || startsWith(github.ref, 'refs/tags/v')) }} continue-on-error: true run: | echo "🧪 Running unit tests..." + # Note: Using TARGET=MACOS as iOS simulator tests are not yet set up in CI gmake TARGET=MACOS check
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build-native.yml(9 hunks)test/src/FakeLogFile.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp}
📄 CodeRabbit inference engine (.cursor/rules/xcsoar-project-rules.mdc)
**/*.{cpp,hpp}: Maximum 79 columns per line (reasonable exceptions allowed)
Use 2 spaces for indentation, never tabs
No indentation fornamespaceblocks
Use.clang-formatfor formatting new code; do not reformat old code unless touched
All source files must start with SPDX license header:// SPDX-License-Identifier: GPL-2.0-or-laterand// Copyright The XCSoar Project
Include file ordering: 1) License header, 2)#pragma once(for headers), 3) Local includes (quoted), 4) System includes (angled), 5) Conditional includes (#ifdef). Use.clang-formatfor automatic ordering
Classes and functions must useCamelCasenaming (notcamelCase)
Variables and class attributes must uselowercase_with_underscoresnaming (e.g.,foo_bar)
Constants and enums must useUPPER_CASEnaming (e.g.,FOO_BAR)
Use C++20 standard
Ensure code isconst-correct; useconstexprinstead ofconstwhenever possible
Usestaticwhenever possible to limit scope and visibility
Make methodsvirtualonly after careful consideration; all overrides must useoverridekeyword; usefinaloften
Compile withWERROR=yflag and fix all compiler warnings
Usenoexceptfor functions that do not throw exceptions
Use GCC attributes when appropriate:[[gnu::pure]],[[gnu::const]],[[noreturn]],[[nodiscard]]
In class declarations, organize: attributes first, then constructor/destructor, then methods
Avoid large functions; split them when they become too large
Avoid dynamic allocation; preferStaticArrayandStaticStringwhen possible
Asterisks belong to variable name: useFoo *a, *bnotFoo* a, b
Avoid preprocessor macros; useinlinefunctions andconstexprvariables instead
Avoid expensive STL containers; preferStaticArray/StaticStringif size is predictable
Avoid template hell; keep templates readable
Enums used in multiple places should include aCOUNTsentinel as the last member; corresponding arrays/lookup tables must usestatic_assertwith `ARR...
Files:
test/src/FakeLogFile.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: yorickreum
Repo: XCSoar/XCSoar PR: 1924
File: darwin/build.sh:65-65
Timestamp: 2025-10-19T22:01:13.888Z
Learning: In the XCSoar darwin/build.sh script, the TESTING="y" flag is intentionally hardcoded for all builds (both Debug and Release). This is a safety feature to prevent pilots from flying with development versions—it gives the app a red icon as a visual warning. TESTING has a different purpose than the DEBUG flag and should remain separate.
📚 Learning: 2025-12-15T01:39:20.273Z
Learnt from: yorickreum
Repo: XCSoar/XCSoar PR: 1990
File: .github/workflows/build-native.yml:484-521
Timestamp: 2025-12-15T01:39:20.273Z
Learning: In CI workflow files that upload iOS IPAs (e.g., .github/workflows/build-native.yml), use xcrun altool --upload-app for TestFlight/App Store uploads. altool remains supported for these uploads (notarization-related deprecation elsewhere). Keep this as the supported method and verify against Apple's current documentation when updating workflows.
Applied to files:
.github/workflows/build-native.yml
📚 Learning: 2025-07-20T17:09:46.107Z
Learnt from: yorickreum
Repo: XCSoar/XCSoar PR: 1713
File: darwin/clean.sh:0-0
Timestamp: 2025-07-20T17:09:46.107Z
Learning: In Xcode sandbox environments, the `brew` command is not available until `/opt/homebrew/bin` is added to PATH first, creating a chicken-and-egg problem. For scripts executed by Xcode, hardcoding the Homebrew path (`/opt/homebrew/bin`) is the correct approach rather than using `$(brew --prefix)`.
Applied to files:
.github/workflows/build-native.yml
📚 Learning: 2025-10-19T22:01:13.888Z
Learnt from: yorickreum
Repo: XCSoar/XCSoar PR: 1924
File: darwin/build.sh:65-65
Timestamp: 2025-10-19T22:01:13.888Z
Learning: In the XCSoar darwin/build.sh script, the TESTING="y" flag is intentionally hardcoded for all builds (both Debug and Release). This is a safety feature to prevent pilots from flying with development versions—it gives the app a red icon as a visual warning. TESTING has a different purpose than the DEBUG flag and should remain separate.
Applied to files:
.github/workflows/build-native.yml
📚 Learning: 2025-12-08T08:34:03.089Z
Learnt from: CR
Repo: XCSoar/XCSoar PR: 0
File: .cursor/rules/xcsoar-project-rules.mdc:0-0
Timestamp: 2025-12-08T08:34:03.089Z
Learning: Applies to **/*.{cpp,hpp} : Logging functions (e.g., `LogFormat()`, `LogDebug()`, `LogInfo()`, `LogError()`) must NOT use translation macros (`_()` or `N_()`) to keep logs in English for consistency and debugging
Applied to files:
test/src/FakeLogFile.cpp
⏰ 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). (8)
- GitHub Check: build (ubuntu-22.04, KOBO, KoboRoot, KoboRoot, .tgz, true, debian:bookworm-slim)
- GitHub Check: build (macos-15, MACOS, XCSoar, bin, XCSoar, false)
- GitHub Check: build (ubuntu-22.04, PC, XCSoar, bin, XCSoar, .exe, true, debian:bookworm-slim)
- GitHub Check: build (ubuntu-22.04, UNIX, xcsoar_7.44_amd64, xcsoar_, .deb, true, debian:bookworm-slim)
- GitHub Check: build (ubuntu-22.04, WIN64, XCSoar, bin, XCSoar, .exe, true, debian:bookworm-slim)
- GitHub Check: build (macos-15, IOS64, xcsoar-signed, XCSoar, .ipa, true)
- GitHub Check: build (ubuntu-22.04, ANDROID_BUNDLE, XCSoar, bin, XCSoar, .apk, false, r26d, debian:bookworm-slim)
- GitHub Check: build (ubuntu-22.04, ANDROID, XCSoar, bin, XCSoar, .apk, true, r26d, debian:bookworm-slim)
🔇 Additional comments (12)
test/src/FakeLogFile.cpp (1)
12-12: LGTM!The
<iterator>include is necessary forstd::back_inserterused on line 26 when compiling with fmt library version 8.0 or later..github/workflows/build-native.yml (11)
20-20: LGTM!Adding the
ios-testingbranch trigger aligns with the PR objectives to enable iOS TestFlight automation for this development branch.
157-172: LGTM!The macOS runner upgrade to
macos-15and the IOS64 configuration changes correctly set up the infrastructure for signed IPA builds and TestFlight uploads.
196-200: LGTM!Logging the Xcode version improves build reproducibility and debugging for iOS-specific issues.
352-381: Well-structured iOS signing setup with proper security practices.The certificate and provisioning profile management follows iOS CI best practices:
- Base64-encoded secrets for secure credential storage
- Temporary keychain creation for build isolation
- Immediate cleanup of sensitive certificate files
- Proper keychain permissions for codesign operations
386-414: LGTM!The iOS app configuration correctly uses repository variables with sensible fallbacks, and
TESTING=yfor PR builds aligns with the project's safety policy to mark development builds with a red icon warning.Based on learnings, the TESTING flag serves as a safety feature to prevent pilots from flying with development versions.
451-460: LGTM!Production release builds correctly use
TESTING=nto remove the red icon warning, which is appropriate for release artifacts intended for distribution to pilots.
477-490: Well-implemented test-gated signing process.The signing step correctly depends on
steps.tests.outcome == 'success', ensuring that builds with failing tests are not signed or distributed. This addresses the concern raised in the PR discussion about preventing uploads when checks fail.The fallback to create an unsigned copy when the provisioning profile is missing is a reasonable approach for debugging.
492-529: LGTM!The TestFlight upload implementation correctly:
- Gates uploads on successful test execution (
steps.tests.outcome == 'success')- Validates the signed IPA exists before attempting upload
- Handles optional credentials gracefully for development workflows
- Uses
xcrun altool --upload-app, which remains the supported method for TestFlight uploads per Apple's documentationBased on learnings, altool is still the appropriate tool for App Store and TestFlight uploads (only deprecated for notarization).
531-534: LGTM!The keychain cleanup correctly uses
always()to ensure the temporary keychain is removed regardless of build outcome, preventing keychain accumulation across builds and potential security issues.
247-247: LGTM!Extending the Homebrew cache to include
macos-15ensures efficient caching for the updated runner.
332-339: LGTM!The Darwin package installation logic correctly handles the new
macos-15runner alongsidemacos-14.
Implementing iOS-specific tests would mean spawning up an iOS simulator inside the CI ... possible, but tedious. I am now running the macOS tests at least. |
Regarding #1957.
Summary by CodeRabbit
New Features
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.