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

Add back 32 bit architectures for distribution but use 64 bit config for Xcode 14 development #1134

Merged
merged 6 commits into from
Oct 20, 2022

Conversation

emawby
Copy link
Contributor

@emawby emawby commented Oct 20, 2022

Description

One Line Summary

PR #1132 Removed 32 bit architectures since they are not supported in Xcode 14, but this causes us to drop support for iOS 9 and 10. This PR adds back the 32 bit architectures while still allowing us to develop in Xcode 14.

Details

Xcode 14 removed 32 bit architecture support so you will get a build error if you build for armv7 using Xcode 14. However we still want to support iOS 9 and 10 (32 bit archs) until Xcode 14 is required for uploading apps to the app store. In order to allow us to develop with Xcode 14 but distribute binaries that include armv7 I have kept armv7 in the release and debug configs and then added an additional debug/release 64 bit config that does not include armv7.

We will need to always release using Xcode 13 build tools (the script will fail if you use Xcode 14 tools)

This PR also adds the OneSignalExample scheme as a shared scheme so that folks use the debug 64 bit config by default

Motivation

Allows us to develop with Xcode 14 but distribute using Xcode 13 build tools with armv7.

Scope

Build and release binaries

Testing

Unit testing

Unit test schemes also use debug 64 bit scheme

Manual testing

I have tested building an app using Xcode 14 when including an XCFramework built with Xcode 13 build tools that contains an armv7 slice.

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
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • 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.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

Copy link
Contributor

@fhboswell fhboswell left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jennantilla, @jkasten2, and @nan-li)

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jennantilla and @nan-li)

@emawby emawby merged commit 8d7df7e into main Oct 20, 2022
@emawby emawby deleted the add_64_bit_config branch October 20, 2022 22:14
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