Skip to content

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

Merged
fadi-george merged 5 commits into
mainfrom
fadi/sdk-4998
Aug 18, 2026
Merged

feat: [SDK-4998] enable KMP logger on Mac Catalyst#1714
fadi-george merged 5 commits into
mainfrom
fadi/sdk-4998

Conversation

@fadi-george

@fadi-george fadi-george commented Aug 12, 2026

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 previously used a no-op remote logger because the KMP XCFramework did 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.
  • Pins and links the KMP XCFramework containing the Mac Catalyst slice.
  • Sets Mac Catalyst 14.0 as the minimum across shipped framework targets, SwiftPM, and CocoaPods while retaining iOS 11.0 for device builds.
  • Adds an internal SPI transport seam so the Catalyst host captures and validates an exported request without contacting production.
  • Archives the umbrella OneSignalFramework consumer for Catalyst and runs a host that verifies start, log, flush, shutdown, and post-shutdown transport blocking.
  • Keeps Catalyst slice architecture and Mach-O metadata validation in the KMP build task.

API impact

The existing public OSRemoteLogger initializer is unchanged. The new request-sender initializer is exposed only through @_spi(OneSignalInternal) for integration verification. SwiftPM now requires tools version 5.5 to declare Mac Catalyst 14.0 support.

Testing

Automated testing

  • Built and verified the KMP XCFramework with arm64 and x86_64 Mac Catalyst slices.
  • Confirmed all eight shipped consumer schemes resolve Mac Catalyst deployment target 14.0.
  • Archived OneSignalFramework for generic/platform=macOS,variant=Mac Catalyst using the Release configuration.
  • Compiled and ran the Catalyst logger host; it captured a non-empty POST /sdk/log, opened no network sockets, and rejected export after shutdown.
  • Ran 13 focused OSLoggerAdaptersTests successfully on an iOS simulator.
  • Validated SwiftPM and both CocoaPods specifications.
  • SwiftLint reported no violations in the changed Swift files.

Manual testing

The Catalyst host provides the runtime verification; no physical Catalyst device is required.

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 Outdated
Comment thread .github/workflows/ci.yml
Comment thread iOS_SDK/OneSignalSDK/CatalystLoggerHost/main.swift
Comment thread iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj Outdated
@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.

Base automatically changed from fadi/sdk-4978 to main August 17, 2026 21:10
fadi-george and others added 2 commits August 17, 2026 14:12
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>

@nan-li nan-li left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ran OneSignalExample > My Mac (Mac Catalyst) and see KMP logger logs post

@fadi-george
fadi-george merged commit 8540cb9 into main Aug 18, 2026
2 of 3 checks passed
@fadi-george
fadi-george deleted the fadi/sdk-4998 branch August 18, 2026 16:35
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.

3 participants