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

[MBL-624] XCode 14.3 Update #1820

Merged
merged 30 commits into from May 15, 2023
Merged

[MBL-624] XCode 14.3 Update #1820

merged 30 commits into from May 15, 2023

Conversation

msadoon
Copy link
Contributor

@msadoon msadoon commented May 11, 2023

📲 What

We need to upgrade our project from XCode 13.4.1 to some version of XCode 14 in order to keep shipping our app. Source.

🤔 Why

Apple requirement, also we should always be up to date on latest Swift versions. In this we support Swift 5.8

🛠 How

We saw lots errors of this ilk when first building with Xcode 14.2:

image

Fix attempt 1

Initially we thought it was ReactiveSwift/ReactiveExtensions/Prelude.
So we updated each of those for building with XCode 14, imported with SPM, but still there was an error.

Oddly though not all files were breaking for the same errors (ie. something like .map(first)) broke in one file but not another.

Fix attempt 2

What changed between Xcode 13.4.1 (last 13.0 release) and the next 14 release (14.0) changelog

Seems like there's a new build system, so might as well have a look at cleaning things up during the build process.

This led to cleaning the the build phases "link binary with libraries"
relevant learnings info

Basically a Dependency means "we need this framework to be built before we built the target"
And "Link Binary With Libraries" means "we optionally can include these other prebuilt frameworks if the target references any of them".

So you can make a framework a "Dependency" if there's linked frameworks in it that you don't want to include repetitively in "Linked Binary With Libraries".

But still the same reactive swift errors during build, which led to...

** FIx attempt 3 **

Looking at the error itself - ie. It's confused about the type. So maybe explicitly setting the type in the right place makes the compiler happy.

For the screenshot above, turns out the change to get the error to go away is

image

explicitly setting that let userAttributedChanged signal to have a type of Signal<(UserAttribute.Notification?, Bool), Never> fixes the issue!

Turns out that is the fix for all the other files too.

Maybe new build systems' compiler/syntax reader loses some context when you have too many functions passed into one another.

It's relatively easy to fix... something like let someEvent = someOtherEvent needs an explicit type - which will cascade down to all dependent signals.

Screenshots
Lots had to be updated for minor visual differences. Technically it's the same simulator, and iOS version as the recording, but after updated my MacOS to Ventura and XCode itself, seems that Apple changed some fonts/spacing on the simulator that made some of our snapshot fail. reference

Build Settings Context: Clicking the "Update to Recommended Settings warnings"

XCode recommends not signing any of our targets so removed those.

Applied the Apple Clang Verifier which is a new build settings called ENABLE_MODULE_VERIFIER defined as

Xcode 14.3 includes a new module verifier that generates diagnostics for issues in a framework’s modules. (97345247)

but it prevents Stripe framework from linking. So set it to OFF. Not important if all it does is report diagnostics.

I think there were duplicate STRIP_STYLE settings that were removed. Here's some documentation on it.

XCode 14.2 to XCode 14.3

The only issue here was updating Kingfisher dependency to support the new version of Swift (5.7 -> 5.8). Noted in this release.

✅ Acceptance criteria

  • XCode 14.3 building
  • Unit/Snapshot tests passing

⏰ TODO

  • Fix Unit/snapshot testing
  • Run through regression checklist

@msadoon msadoon self-assigned this May 11, 2023
@msadoon msadoon added this to the release-5.8.0 milestone May 11, 2023
@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Merging #1820 (47044a4) into main (e49be00) will decrease coverage by 0.58%.
The diff coverage is 75.63%.

@@            Coverage Diff             @@
##             main    #1820      +/-   ##
==========================================
- Coverage   85.03%   84.46%   -0.58%     
==========================================
  Files        1294     1283      -11     
  Lines      118603   116641    -1962     
  Branches    31442    30947     -495     
==========================================
- Hits       100855    98516    -2339     
- Misses      16659    17047     +388     
+ Partials     1089     1078      -11     
Impacted Files Coverage Δ
.../Controller/DiscoveryPageViewControllerTests.swift 100.00% <ø> (ø)
...BetaTools/Controller/BetaToolsViewController.swift 39.83% <25.00%> (ø)
...Discovery/Controller/DiscoveryViewController.swift 8.33% <25.00%> (ø)
Kickstarter-iOS/AppDelegateViewModelTests.swift 97.74% <76.31%> (-1.18%) ⬇️
Kickstarter-iOS/AppDelegateViewModel.swift 92.13% <93.10%> (-0.31%) ⬇️
...overy/Controller/DiscoveryPageViewController.swift 53.59% <100.00%> (-2.29%) ⬇️

... and 86 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@msadoon msadoon force-pushed the mbl-624/xcode14_3_update branch 2 times, most recently from 767f036 to 47044a4 Compare May 11, 2023 21:43
@msadoon msadoon marked this pull request as ready for review May 11, 2023 21:43
@msadoon msadoon requested a review from scottkicks May 11, 2023 21:44
@msadoon
Copy link
Contributor Author

msadoon commented May 11, 2023

@scottkicks tests should pass, I just reverted some commits back to when they were passing.

Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

🥳 Builds, Runs, and tests pass

I can spend time going through the regression checklist as well, but it looks good to me

@msadoon
Copy link
Contributor Author

msadoon commented May 15, 2023

🥳 Builds, Runs, and tests pass

I can spend time going through the regression checklist as well, but it looks good to me

Awesome! We can start using XCode 14.3 🥳 .
No worries with the regression checklist, we can run through that before releasing. I ran through most steps there anyway, should be fine.

@msadoon msadoon merged commit 6a48e5a into main May 15, 2023
7 checks passed
@msadoon msadoon deleted the mbl-624/xcode14_3_update branch May 15, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants