Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OneSignalLiveActivities framework included for mac catalyst #1446

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented May 29, 2024

Description

One Line Summary

Add mac catalyst to supported destinations in the OneSignalLiveActivities framework, make ActivityKit-depending files blank, with no change to the umbrella OneSignalFramework.

Details

What's Changed

  • "Remove" any files containing ActivityKit-related functionality for Mac Catalyst
  • See screenshots for project changes

Screenshot 2024-06-09 at 4 26 49 PM

Screenshot 2024-06-09 at 4 26 59 PM

Solutions Investigated

The errors developers encountered was that the umbrella framework couldn't find OneSignalLiveActivities. So, initially, we considered just adding OneSignalLiveActivities to the OneSignalFramework for mac catalyst.

However, this does not work when building the frameworks with our build script. Tried some other modifications to no avail.

Back to the drawing board... can we continue to exclude Mac Catalyst and make changes elsewhere? Mac catalyst really should not include OneSignalLiveActivities. The problem next is with our Podspec that names it as a dependency of OneSignalFramework.

I investigated how to modify our Podspec to only include Live Activities for ios and not mac catalyst. However, it is tricky. A key problem is that Mac Catalyst is an ios sdk and not a macOS sdk, and not easily differentiable so we cannot use ios vs osx specifications for a Podspec.

What I landed on finally was including Mac Catalyst for OneSignalLiveActivities and removing functionality related to ActivityKit. It did not appear any changes to OneSignalFramework was needed.

Motivation

Scope

Mac Catalyst support

Testing

Unit testing

None

Manual testing

Reproduction of Original Issue

  • Reproduced error when I added 5.2.0 via SPM to another project and deploying to Mac Catalyst.

/Users/me/Library/Developer/Xcode/DerivedData/SPMTestApp-dgyizzluzviitzglkzsjezrvnaql/SourcePackages/artifacts/onesignal-xcframework/OneSignalLiveActivities/OneSignalLiveActivities.xcframework:1:1 While building for Mac Catalyst, no library for this platform was found in '/Users/me/Library/Developer/Xcode/DerivedData/SPMTestApp-dgyizzluzviitzglkzsjezrvnaql/SourcePackages/artifacts/onesignal-xcframework/OneSignalLiveActivities/OneSignalLiveActivities.xcframework'.

  • The error is not reproducible in the SDK's example app due to how dependencies are set up.

How the changes in this PR were tested

Cocoapods

  1. Build the SDK with the build all frameworks script
  2. Pod install to a project and run on iPhone and Mac Catalyst
  3. Mac Catalyst logs the stub message "oneSignalLiveActivities not found. In order to use OneSignal's LiveActivities features the OneSignalLiveActivities module must be added"

Swift Package Manager

  1. Change the URL in the Package.swift file for each zip to point to the OneSignalXCFramework repo and pick an existing release. Then edit that release and attach the zips.
  2. Have a test app point at this branch when adding SwiftPM
  3. Run on Mac Catalyst and iPhone
  4. Mac Catalyst logs the stub message "oneSignalLiveActivities not found. In order to use OneSignal's LiveActivities features the OneSignalLiveActivities module must be added"

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

This change is Reviewable

@nan-li nan-li requested a review from emawby May 29, 2024 17:00
@nan-li nan-li force-pushed the fix/live_activities_mac_catalyst branch 3 times, most recently from 628df93 to c20ec2c Compare June 5, 2024 19:29
@nan-li nan-li changed the title OneSignalLiveActivities framework add mac catalyst to supported destinations WIP - testing archive - OneSignalLiveActivities framework add mac catalyst to supported destinations Jun 5, 2024
@nan-li nan-li force-pushed the fix/live_activities_mac_catalyst branch from 1b1678a to c20ec2c Compare June 5, 2024 22:45
@nan-li nan-li changed the title WIP - testing archive - OneSignalLiveActivities framework add mac catalyst to supported destinations OneSignalLiveActivities framework included for mac catalyst Jun 5, 2024
Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

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

Looks good! I recommend that we also test Swift PM before releasing though. It is a bit janky because we need to have the correct zips pulled.
You should be able to do the following though:

  1. Change the URL in the Package.swift file for each zip to point to the OneSignalXCFramework repo and pick an existing release. Then edit that release and attach the zips.
  2. Have your test app point at this branch when adding SwiftPM
  3. Revert those changes once we have finished testing

@nan-li
Copy link
Contributor Author

nan-li commented Jun 6, 2024

There's some weird behavior I am still investigating. Going to focus on building just Live Activities, I think related to its scheme.

Making Live Activities use the normal build script from build_all_frameworks.sh causes some strange issues when looking at the files generated.
OneSignal_LiveActivities itself built differently, looks like some file creation and removal didn't happen but also every package before it in the build script is missing the zip (seems correct). Every package after (Location, IAM, XCFramework) contains the zip, I think the old zips so these packages never built successfully?

Build logs are also so verbose it is hard to parse, no obvious errors either.

Screenshot 2024-06-06 at 9 00 39 AM

@emawby
Copy link
Contributor

emawby commented Jun 6, 2024

There's some weird behavior I am still investigating. Going to focus on building just Live Activities, I think related to its scheme.

Making Live Activities use the normal build script from build_all_frameworks.sh causes some strange issues when looking at the files generated. OneSignal_LiveActivities itself built differently, looks like some file creation and removal didn't happen but also every package before it in the build script is missing the zip (seems correct). Every package after (Location, IAM, XCFramework) contains the zip, I think the old zips so these packages never built successfully?

Build logs are also so verbose it is hard to parse, no obvious errors either.

Screenshot 2024-06-06 at 9 00 39 AM

Is it just the zipping that is failing or are the XCFrameworks corrupted/out of date. You can try dragging in the XCFrameworks to a test app directly to see if they build/have the expected changes.

@nan-li
Copy link
Contributor Author

nan-li commented Jun 6, 2024

I haven't run yet the 2nd script that generates the zips.
The build_all_frameworks.sh will delete the zips. I believe probably building Live Activities is failing and preventing the subsequent frameworks to be built (which would remove their zips, which are still present).

Still looking at this.

* To build the SDK as is for Mac Catalyst (without ripping out Live Activities or making more complicated changes), Mac Catalyst is added as a platform for the OneSignalLiveActivities framework
* Effectively empty any file referencing ActivityKit in order to support Mac Catalyst builds
* See #1446 for details
@nan-li nan-li force-pushed the fix/live_activities_mac_catalyst branch from 8cab614 to 83f790c Compare June 9, 2024 23:52
* This change is needed to build the SDK for distribution or else fail to archive
@emawby emawby self-requested a review June 10, 2024 17:44
@nan-li nan-li merged commit 37ede25 into main Jun 10, 2024
4 checks passed
@nan-li nan-li deleted the fix/live_activities_mac_catalyst branch June 10, 2024 23:22
@webbpage
Copy link

This is not working for me. Are there new deployment steps specific to Catalyst that I'm missing?

@nan-li
Copy link
Contributor Author

nan-li commented Jun 25, 2024

I see you followed up at #1445. I will continue the conversation there.

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.

[Bug]: Mac Catalyst Build Breaks with Latest Release
3 participants