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.

Sign up for GitHub

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

Support Xcode 16 #4066

Merged
merged 9 commits into from
Sep 30, 2024
Merged

Support Xcode 16 #4066

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
636a8fb
Fix StripeConnect to compile in Xcode 16
mludowise-stripe Sep 26, 2024
d13fc70
Test using Xcode 16
mludowise-stripe Sep 26, 2024
ebdef6f
fix lint
mludowise-stripe Sep 26, 2024
d2b0008
Add StripeConnect to all frameworks
mludowise-stripe Sep 26, 2024
fe7fe54
Fix more 16 compile failures
mludowise-stripe Sep 26, 2024
153e55f
fix test
mludowise-stripe Sep 26, 2024
6fe8fa3
attempt to update OCMock
mludowise-stripe Sep 27, 2024
35c8353
fix Package.resolved
mludowise-stripe Sep 27, 2024
22cbcac
add Package.resolved hash back
mludowise-stripe Sep 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions Stripe.xcworkspace/xcshareddata/swiftpm/Package.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"originHash" : "2ad13697f5d60ad400147e34535361f52e72d2f7a280df77bb3c3521cc5623bc",
"originHash" : "c2f4a8c2436760fc0134632c1cdcb8e7f9b44493c60679a155d27d6838da7d8e",
"pins" : [
{
"identity" : "capture-core-sp",
Expand All @@ -24,8 +24,7 @@
"kind" : "remoteSourceControl",
"location" : "https://github.com/erikdoe/ocmock",
"state" : {
"branch" : "master",
"revision" : "05cbc9d934e84ad32530fa53fd7d29c7966d5828"
"revision" : "2c0bfd373289f4a7716db5d6db471640f91a6507"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hash corresponds to the 3.9.4 release tag:
https://github.com/erikdoe/ocmock/releases/tag/v3.9.4

Swift Package manager doesn't allow OCMock unless you add it using a commit hash:
erikdoe/ocmock#500 (comment)

}
},
{
Expand All @@ -38,6 +37,5 @@
}
}
],
"version" : 2
"version" : 3
}

28 changes: 28 additions & 0 deletions Stripe.xcworkspace/xcshareddata/xcschemes/AllStripeFrameworks.xcscheme
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,34 @@
ReferencedContainer = "container:Stripe3DS2/Stripe3DS2.xcodeproj">
</BuildableReference>
</BuildActionEntry>
<BuildActionEntry
buildForTesting = "YES"
buildForRunning = "YES"
buildForProfiling = "YES"
buildForArchiving = "YES"
buildForAnalyzing = "YES">
<BuildableReference
BuildableIdentifier = "primary"
BlueprintIdentifier = "41D17A3F2C5A73A6007C6EE6"
BuildableName = "StripeConnect.framework"
BlueprintName = "StripeConnect"
ReferencedContainer = "container:StripeConnect/StripeConnect.xcodeproj">
</BuildableReference>
</BuildActionEntry>
<BuildActionEntry
buildForTesting = "YES"
buildForRunning = "NO"
buildForProfiling = "NO"
buildForArchiving = "NO"
buildForAnalyzing = "NO">
<BuildableReference
BuildableIdentifier = "primary"
BlueprintIdentifier = "41D17A492C5A73A7007C6EE6"
BuildableName = "StripeConnectTests.xctest"
BlueprintName = "StripeConnectTests"
ReferencedContainer = "container:StripeConnect/StripeConnect.xcodeproj">
</BuildableReference>
</BuildActionEntry>
Comment on lines +177 to +204
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds StripeConnect to AllStripeFrameworks so we'll get CI failures when this module doesn't compile

</BuildActionEntries>
</BuildAction>
<TestAction
Expand Down
4 changes: 2 additions & 2 deletions Stripe/Stripe.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2264,8 +2264,8 @@
isa = XCRemoteSwiftPackageReference;
repositoryURL = "https://github.com/erikdoe/ocmock";
requirement = {
branch = master;
kind = branch;
kind = revision;
revision = 2c0bfd373289f4a7716db5d6db471640f91a6507;
};
};
/* End XCRemoteSwiftPackageReference section */
Expand Down
28 changes: 13 additions & 15 deletions StripeCardScan/StripeCardScan/Source/CardScan/UI/ScanBaseViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,9 +356,9 @@ class ScanBaseViewController: UIViewController, AVCaptureVideoDataOutputSampleBu
override func viewWillAppear(_ animated: Bool) {
super.viewWillAppear(animated)
ScanBaseViewController.isAppearing = true
/// Set beginning of scan session
// Set beginning of scan session
ScanAnalyticsManager.shared.setScanSessionStartTime(time: Date())
/// Check and log torch availability
// Check and log torch availability
Comment on lines +359 to +361
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Swiftlint fix

ScanAnalyticsManager.shared.logTorchSupportTask(
supported: videoFeed.hasTorchAndIsAvailable()
)
Expand Down Expand Up @@ -413,22 +413,20 @@ class ScanBaseViewController: UIViewController, AVCaptureVideoDataOutputSampleBu
from connection: AVCaptureConnection
) {
if self.machineLearningSemaphore.wait(timeout: .now()) == .success {
ScanBaseViewController.machineLearningQueue.async {
self.captureOutputWork(sampleBuffer: sampleBuffer)
guard let pixelBuffer = CMSampleBufferGetImageBuffer(sampleBuffer),
let fullCameraImage = pixelBuffer.cgImage() else {
self.machineLearningSemaphore.signal()
return
}
}
}

func captureOutputWork(sampleBuffer: CMSampleBuffer) {
guard let pixelBuffer = CMSampleBufferGetImageBuffer(sampleBuffer) else {
return
}

guard let fullCameraImage = pixelBuffer.cgImage() else {
return
ScanBaseViewController.machineLearningQueue.async { [weak self] in
self?.captureOutputWork(fullCameraImage: fullCameraImage)
self?.machineLearningSemaphore.signal()
}
}
}
Comment on lines 415 to +427
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both CMSampleBuffer and CVImageBuffer are not Sendable, which I think means that they can only be modified on the main thread? Solution was to convert them to CIImage before passing them to the dispatch queue.

I'm unsure if this is the intended behavior – if it's expensive to convert CMSampleBuffer -> CVImageBuffer -> CIImage, then we're now doing that work on the main thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on my memory from implementing the equivalent for Identity, captureOutput on the AVCaptureVideoDataOutputSampleBufferDelegate is always called off the main thread on its own dedicated thread, so I think this should be okay.

Copy link
Contributor

@davidme-stripe davidme-stripe Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  • I think it might be a little expensive, but I agree that I don't think this will be called on the main thread
  • I don't have the full logic downloaded into my brain atm, but is it okay to call the machineLearningSemaphore outside the machineLearningQueue? That would free anyone else wait-ing on it, right? And no one else should be waiting on it anyway, because captureOutput() won't be called again until this function returns?

Copy link
Contributor Author

@mludowise-stripe mludowise-stripe Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

is it okay to call the machineLearningSemaphore outside the machineLearningQueue?

Yes. In fact, this is what it's designed for. Generally the camera feed is faster than the ML model can process images, so the idea is that you want to block the thread that is reading from the camera in order to finish processing the image in CoreML before asking for more camera frames, otherwise you end queuing up so much of the video buffer that the displayed camera feed begins to drop frames and the app lags.

Here's an excerpt from the copy of the CoreML survival guide that gives a basic example:
https://drive.google.com/open?id=1gwW8pgJvg8t_EOkM2cgnDsQDI5Hc_f4o&disco=AAAAVxpjn24

Public link (page 400):
https://www.scribd.com/document/689236733/Core-ML-Survival-Guide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So yeah, given that we're explicitly calling wait in order to pause this DispatchQueue, I think that it's inconsequential that converting CMSampleBuffer -> CIImage could be non-performant


func captureOutputWork(fullCameraImage: CGImage) {
// confirm videoGravity settings in previewView. Calculations based on .resizeAspectFill
DispatchQueue.main.async {
assert(self.previewView?.videoPreviewLayer.videoGravity == .resizeAspectFill)
Expand Down Expand Up @@ -472,9 +470,9 @@ class ScanBaseViewController: UIViewController, AVCaptureVideoDataOutputSampleBu
// MARK: - OcrMainLoopComplete logic
func complete(creditCardOcrResult: CreditCardOcrResult) {
ocrMainLoop()?.mainLoopDelegate = nil
/// Stop the previewing when we are done
// Stop the previewing when we are done
self.previewView?.videoPreviewLayer.session?.stopRunning()
/// Log total frames processed
// Log total frames processed
ScanAnalyticsManager.shared.logMainLoopImageProcessedRepeatingTask(
.init(executions: self.getScanStats().scans)
)
Expand Down
14 changes: 13 additions & 1 deletion StripeConnect/StripeConnect/Source/Internal/Webview/ApplicationURLOpener.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,22 @@ import UIKit
/// Protocol used to dependency inject `UIApplication.open` for use in tests
protocol ApplicationURLOpener {
func canOpenURL(_ url: URL) -> Bool
func open(_ url: URL, options: [UIApplication.OpenExternalURLOptionsKey: Any], completionHandler completion: ((Bool) -> Void)?)
func open(
_ url: URL,
options: [UIApplication.OpenExternalURLOptionsKey: Any],
completionHandler completion: OpenCompletionHandler?
)
}

extension ApplicationURLOpener {
// The signature of the `completionHandler` in UIApplication.open changed
// in Xcode 16
#if compiler(>=6.0)
typealias OpenCompletionHandler = @MainActor @Sendable (Bool) -> Void
#else
typealias OpenCompletionHandler = (Bool) -> Void
#endif

func openIfPossible(_ url: URL) {
guard canOpenURL(url) else {
// TODO: MXMOBILE-2491 Log as analytics when url can't be opened.
Expand Down
4 changes: 2 additions & 2 deletions StripeConnect/StripeConnectTests/Internal/Webview/ConnectWebViewTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,13 +157,13 @@ class MockNavigationAction: WKNavigationAction {

class MockURLOpener: ApplicationURLOpener {
var canOpenURLOverride: ((_ url: URL) -> Bool)?
var openURLOverride: ((_ url: URL, _ options: [UIApplication.OpenExternalURLOptionsKey: Any], _ completion: ((Bool) -> Void)?) -> Void)?
var openURLOverride: ((_ url: URL, _ options: [UIApplication.OpenExternalURLOptionsKey: Any], _ completion: OpenCompletionHandler?) -> Void)?

func canOpenURL(_ url: URL) -> Bool {
canOpenURLOverride?(url) ?? false
}

func open(_ url: URL, options: [UIApplication.OpenExternalURLOptionsKey: Any], completionHandler completion: ((Bool) -> Void)?) {
func open(_ url: URL, options: [UIApplication.OpenExternalURLOptionsKey: Any], completionHandler completion: OpenCompletionHandler?) {
openURLOverride?(url, options, completion)
}
}
31 changes: 31 additions & 0 deletions bitrise.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ stages:
workflows:
- framework-tests: {}
- test-builds-xcode-15: {}
- test-builds-xcode-16: {}
- test-builds-vision: {}
- install-tests-non-carthage: {}
- lint-tests: {}
Expand All @@ -53,7 +54,9 @@ stages:
workflows:
- framework-tests-no-mocks: {}
- test-builds-xcode-15: {}
- test-builds-xcode-16: {}
- test-builds-xcode-15-release: {}
- test-builds-xcode-16-release: {}
Comment on lines +57 to +59
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds tests to ensure the frameworks build on both Xcode 15 and 16

- test-builds-vision: {}
- deploy-docs: {}
- install-tests-non-carthage: {}
Expand All @@ -71,6 +74,7 @@ stages:
stage-nightly-all:
workflows:
- test-builds-xcode-15-release: {}
- test-builds-xcode-16-release: {}
- framework-tests-no-mocks: {}
- check-docs: {}
- legacy-tests-15: {}
Expand Down Expand Up @@ -348,6 +352,21 @@ workflows:
bitrise.io:
stack: osx-xcode-15.0.x
machine_type_id: g2-m1.8core
test-builds-xcode-16:
steps:
- xcode-build-for-test@2:
inputs:
- scheme: AllStripeFrameworks
- destination: generic/platform=iOS Simulator
- deploy-to-bitrise-io@2: {}
envs:
- DEFAULT_TEST_DEVICE: platform=iOS Simulator,name=iPhone 15,OS=18.0
before_run:
- prep_all
meta:
bitrise.io:
stack: osx-xcode-16.0.x
machine_type_id: g2-m1.8core
test-builds-xcode-15-release:
steps:
- script@1:
Expand All @@ -360,6 +379,18 @@ workflows:
bitrise.io:
stack: osx-xcode-15.0.x
machine_type_id: g2-m1.8core
test-builds-xcode-16-release:
steps:
- script@1:
inputs:
- content: xcodebuild build -workspace "Stripe.xcworkspace" -scheme "AllStripeFrameworks" -configuration "Release" -sdk "iphonesimulator" | xcpretty
title: Build release builds
before_run:
- prep_all
meta:
bitrise.io:
stack: osx-xcode-16.0.x
machine_type_id: g2-m1.8core
test-builds-vision:
steps:
- xcode-build-for-test@2:
Expand Down
Loading