Skip to content

feat: [SDK-4998] enable KMP logger on Mac Catalyst - #1714

Open
fadi-george wants to merge 2 commits into
fadi/sdk-4978from
fadi/sdk-4998
Open

feat: [SDK-4998] enable KMP logger on Mac Catalyst#1714
fadi-george wants to merge 2 commits into
fadi/sdk-4978from
fadi/sdk-4998

Conversation

@fadi-george

Copy link
Copy Markdown
Collaborator

Description

One Line Summary

Enables the shared KMP logger when the iOS SDK is built for Mac Catalyst.

Details

Motivation

Mac Catalyst builds currently use a no-op remote logger because the KMP XCFramework does not expose a Catalyst slice. With the Catalyst slice added in OneSignal-KMP-SDK#18, the iOS SDK can compile, link, and run the shared logging pipeline on Catalyst.

Scope

  • Removes the Catalyst exclusions from the Swift KMP logger adapters.
  • Links the Mac Catalyst slice from OneSignalKMP.xcframework.
  • Adds CI verification for both Catalyst architectures and Mach-O platform metadata.
  • Archives OneSignalOSCore for Catalyst and runs a small host that exercises log, flush, and shutdown.
  • Stacked on SDK-4978.

Testing

Unit testing

No new unit tests are needed for the platform guards and Xcode link settings. CI adds a Catalyst runtime host that exercises the KMP logger adapters end to end.

Manual testing

  • Rebuilt and verified the KMP XCFramework with arm64 and x86_64 Mac Catalyst slices.
  • Archived OneSignalOSCore for generic/platform=macOS,variant=Mac Catalyst.
  • Compiled and ran the Catalyst logger host successfully.
  • Ran 23 focused crash-handler and logger-adapter tests successfully on the stacked base.
  • SwiftLint reported no violations in the changed source files; the repository-wide local run encountered existing generated demo build-file violations.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

Made with Cursor

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multimodal adversarial review (interrogate)

Verdict: request changes. The Catalyst enablement is not self-consistent at this head: CI already fails, and the next steps would fail for independent reasons.

Intent

Enable the shared KMP remote logger on Mac Catalyst by removing #if !targetEnvironment(macCatalyst) no-ops, linking the Catalyst XCFramework slice, and adding CI that verifies the slice, archives OneSignalOSCore for Catalyst, and runs a host through log/flush/shutdown.

Reviewers

  • A: claude-fable-5-thinking-xhigh — 9 findings
  • B: gpt-5.6-sol-xhigh — 3 findings
  • C: cursor-grok-4.5-high-fast — 7 findings
  • D: claude-opus-5-thinking-high — 10 findings

Act On

  1. KMP gitlink not bumped (A/B/C/D). Head still pins OneSignal-KMP-SDK @ 72581f3, which only builds iosX64 / iosArm64 / iosSimulatorArm64. No ios-arm64_x86_64-maccatalyst slice exists. CI run 31646971241 already dies at lipo on the missing path. Removing the Catalyst stubs also means Catalyst archives (including build_all_frameworks.sh) will fail to import OneSignalKMP until the pin includes KMP #18.
  2. Host target arch ≠ runner arch (A/B/C/D). Job is macos-15-large (Intel; confirmed via JAVA_HOME_17_X64 in the failed run). Host is hardcoded to arm64-apple-ios14.0-macabi, then executed. After the slice exists, this step fails with bad CPU type.
  3. No shared OneSignalOSCore scheme (C; verified). Shared schemes have Core / UnitTestApp / etc., but not OneSignalOSCore. Archive uses -scheme OneSignalOSCore and will fail on a clean CI checkout once the prior step passes.
  4. Host never calls start() (A/B/C/D). HTTP send is gated by lifecycle.performIfTransportActive (isStarted). Without start(), flush completes as disabled while the semaphore still succeeds — false green for the stated log/flush/shutdown exercise.

Consider

  • Drop or co-land kotlinVersion / swiftVersion / additionalVersionAttributes with the KMP pin that actually declares them (A/C/D).
  • Move Catalyst Mach-O checks into KMP verifyOneSignalKMPXCFramework instead of duplicating lipo/ar/vtool in YAML (A/C/D).
  • Put FRAMEWORK_SEARCH_PATHS[sdk=macosx*] on OneSignalOSCore next to the iOS keys, not only project-level configs (A/C/D).
  • Prefer xcodebuild test of OneSignalOSCoreTests on Catalyst over a bespoke host (A/D).
  • Add a Catalyst platform discriminator so logs are not indistinguishable from iPad (sdkBase/UIDevice vs kern.osversion) (A/D).

Noted

  • "unavailable" preconditions are tautological after stub removal (A/D).
  • Leftover no-op OSRemoteLoggerProtocol.start() default (A).
  • Hardcoded slice path proliferation / [sdk=macosx*] broader than Catalyst (D).
  • Import/format leftovers from #if deletion (D).

Agreement map

All four models independently flagged the unpinned Catalyst slice and the host disabled/start() gap. Three+ flagged the Intel-vs-arm64 host target. Scheme absence and protocol-stub drift were lower fan-out but verified in-repo. No reviewer found the PR ready to merge as-is.

Skill: Cursor interrogate (pstack).

Open in Web View Automation 

Sent by Cursor Automation: Untitled

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml
Comment thread iOS_SDK/OneSignalSDK/CatalystLoggerHost/main.swift
Comment thread iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj Outdated
fadi-george and others added 2 commits August 13, 2026 18:35
Pin the Catalyst-capable KMP artifact and exercise the active logger lifecycle on the CI runner architecture through a shared archive scheme.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cursor

cursor Bot commented Aug 14, 2026

Copy link
Copy Markdown

Multimodal adversarial review (interrogate)

Verdict: request changes. Prior Act On items (KMP pin, shared scheme, uname -m host target, start(), OSCore framework search paths) are fixed and the Catalyst archive/host CI steps are green. Two independent issues remain: the host still talks to production ingest, and the Catalyst 14.0 bump is not propagated to the modules build_all_frameworks.sh actually ships.

Intent

Enable the shared KMP remote logger when the iOS SDK is built for Mac Catalyst. Remove #if !targetEnvironment(macCatalyst) no-op stubs from the Swift KMP logger adapters so Catalyst uses the real composition; pin OneSignal-KMP-SDK to a commit that produces the Mac Catalyst XCFramework slice; link that slice via Xcode search/link settings; and add CI that archives OneSignalOSCore for Catalyst and runs a small host that exercises start, log, flush, and shutdown.

Reviewers

  • A: claude-fable-5-thinking-xhigh — 5 findings
  • B: gpt-5.6-sol-xhigh — 6 findings
  • C: cursor-grok-4.5-high-fast — 5 findings
  • D: claude-opus-5-thinking-high — 11 findings

Act On

  1. CI host POSTs to production api.onesignal.com and still does not prove the round trip (A/B/C/D). CatalystLoggerHost sets remoteLogLevelProvider to "INFO", calls start() (opens performIfTransportActive), then log + forceFlush. That path uses a live URLSession against OS_API_SERVER_URL (https://api.onesignal.com/sdk/log?app_id= with a nil app id). The same host would also have passed against the deleted stub (kmpVersion/crashStoragePath were "unavailable"; flush always invokes completion and discards success). log is fire-and-forget, so flush can drain an empty buffer. Inject an @_spi sender/base-URL seam, point the host at a capturing stub or loopback, and assert a real exported request (and that shutdown refuses a further send).

  2. Catalyst 14.0 min was applied only to OneSignalCore / OneSignalOSCore (A/B/D). The KMP slice requires macabi 14.0, but OneSignalUser, OneSignalFramework, Live Activities, and the other shipped targets still archive Catalyst at the clamped 13.1 / advertised iOS 11. Swift will refuse to import OSCore 14.0 from a 13.1 client, so build_all_frameworks.sh Catalyst archives of the umbrella and User modules are the first place this breaks. CI only archives OneSignalOSCore itself. Set the 14.0 override at project level (or on every shipped framework target), reconcile podspec / SwiftPM Catalyst mins, and archive a consumer (OneSignalFramework or OneSignalUser) in CI.

Consider

  • OneSignalOSCoreTests still has no FRAMEWORK_SEARCH_PATHS[sdk=macosx*], so the existing adapter tests cannot be the Catalyst proof even if you add a Catalyst destination (B).
  • additionalVersionAttributes["apple_platform"] = "mac_catalyst" is the only Catalyst discriminator, is untested, and sits next to sdkBase = "ios" plus mixed UIDevice iOS version / kern.osversion macOS build (D).
  • Workflow paths-ignore: '.github/**' means a CI-only follow-up cannot validate these new steps (A/B/D).
  • Host lives outside the Xcode project (swiftc in YAML only), so local reproduction depends on copying the CI snippet (C/D).

Noted

  • Leftover public extension OSRemoteLoggerProtocol { func start() {} } after stub removal (A/C/D).
  • kotlinVersion / swiftVersion stay nil; Swift is the one host where swiftVersion is meaningful (D).
  • Hardcoded KMP XCFramework slice paths proliferate in project.pbxproj (D).
  • Job 31761224844 is red on IAM/User tests; those suites also fail on stacked base feat: [SDK-4978] add iOS KMP crash capture and upload #1713. Catalyst archive + host steps succeeded. Unrelated to this diff.

Dismissed

  • Formatting leftover in KotlinByteArray+Data.swift (D) — not actionable.
  • Redundant BUILD_LIBRARY_FOR_DISTRIBUTION=YES on the archive command (D) — matches build_all_frameworks.sh.
  • [sdk=macosx*] being broader than Catalyst (prior pass) — that is how Xcode identifies the Catalyst SDK; this repo does not ship native macOS.

Agreement Map

All four models independently flagged production HTTP from the host and that the host assertions would pass against the deleted stub. Three flagged the log/flush race and the incomplete 14.0 rollout. Lone-model items (test-target FSP, metadata discriminator, pbxproj path copies) did not reach Act On. Prior-review blockers (unpinned KMP slice, Intel/arm64 host target, missing scheme, missing start()) verified fixed at dff80257.

Skill: Cursor interrogate (pstack). Re-review of head dff80257 vs base fadi/sdk-4978. GitHub MCP was unavailable in this run, so this is a summary comment rather than inline review comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant