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

Adopt Swift 6 and Xcode 16 #322

Merged
merged 16 commits into from
Oct 19, 2024
Merged

Adopt Swift 6 and Xcode 16 #322

merged 16 commits into from
Oct 19, 2024

Conversation

dfed
Copy link
Collaborator

@dfed dfed commented Oct 11, 2024

Breaking change! Gets us running on Xcode 16, dropping support for prior Xcode versions. Fixes all consumer-facing warnings regardless of build system, belatedly addressing #294. Also (see inline comments) removes API for tvOS and watchOS that likely never worked given warnings received when compiling on Xcode 16.

Getting on Swift 6 allows us to use typed throws. Utilizing typed throws feels like the right move given that we haven't seen a change to what errors we throw in seven years.

@dfed dfed self-assigned this Oct 11, 2024
Comment on lines +9 to +12
.iOS(.v12),
.tvOS(.v12),
.watchOS(.v4),
.macOS(.v10_13),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

minimum versions in Xcode 16 updated

if !userPrompt.isEmpty {
secItemQuery[kSecUseOperationPrompt as String] = userPrompt
let context = LAContext()
context.localizedReason = userPrompt
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sigh. this API doesn't exist on either tvOS or watchOS. the methods that call into this method no longer expose the userPrompt

/// - Parameters:
/// - key: A key used to retrieve the desired object from the keychain.
/// - userPrompt: The prompt displayed to the user in Apple's Face ID, Touch ID, or passcode entry UI.
/// - Returns: The data currently stored in the keychain for the provided key.
/// - Throws: An error of type `KeychainError`.
@objc
public func object(forKey key: String, withPrompt userPrompt: String) throws -> Data {
try execute(in: lock) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Swift 6 can't rethrow with a typed error, so we just inline the helper here and elsewhere

@@ -22,7 +22,7 @@ import XCTest

extension SecureEnclaveIntegrationTests {

@available (*, deprecated)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

funnily enough, this space is now a warning

@@ -1,20 +0,0 @@
#!/bin/bash -l
Copy link
Collaborator Author

@dfed dfed Oct 11, 2024

Choose a reason for hiding this comment

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

not needed on recent Xcodes!

@@ -14,7 +14,7 @@
// limitations under the License.
//

#import "VALLegacyValet.h"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whoops. Xcode is warning about these malformed imports now, which is long overdue!

@dfed dfed marked this pull request as ready for review October 11, 2024 08:11
@dfed dfed requested a review from NickEntin October 11, 2024 21:09
@@ -677,28 +677,31 @@ class ValetIntegrationTests: XCTestCase
removeQueue.async(flags: .barrier) {
removeQueueExpectation.fulfill()
}

waitForExpectations(timeout: 10.0, handler: nil)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Xcode is pushing us towards the fulfillment method below now.

{
let setQueue = DispatchQueue(label: "Set String Queue", attributes: .concurrent)
let removeQueue = DispatchQueue(label: "Remove Object Queue", attributes: .concurrent)

for _ in 1...50 {
setQueue.async {
setQueue.async { [vanillaValet, passcode, key] in
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

self isn't Sendable, so we capture what we need rather than self

static var sharedAccessGroupIdentifier: SharedGroupIdentifier = {
static let sharedAccessGroupIdentifier: SharedGroupIdentifier = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these were never set. not sure why I made them var to begin with, but... easy fix!

@@ -216,14 +260,6 @@ public final class SecureEnclaveValet: NSObject, Sendable {
try migrateObjects(matching: valet.baseKeychainQuery, removeOnCompletion: removeOnCompletion)
}

// MARK: Renamed Methods
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These were renamed in more than four years ago in the V4 release. Open to keeping them around but doing some roomba-ing.


## Migrating from prior Valet versions

The good news: most Valet configurations do _not_ have to migrate keychain data when upgrading from an older version of Valet. All Valet objects are backwards compatible with their counterparts from prior versions. We have exhaustive unit tests to prove it (search for `test_backwardsCompatibility`). Valets that have had their configurations deprecated by Apple will need to migrate stored data.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the exhaustive tests required the v2 (objc) version of Valet to live forever without changes, and these old files were utilizing deprecated API. We maintained these tests for the better part of a decade. We successfully managed to deploy a rewrite in Swift without regression years ago. Time to let go.

README.md Outdated Show resolved Hide resolved
import Foundation


internal final class WeakStorage<T: AnyObject>: @unchecked Sendable {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thread-safe because the underlying storage is only ever accessed or modified within a lock.

return valet
}
}

/// - Parameters:
/// - identifier: A non-empty string that must correspond with the value for keychain-access-groups in your Entitlements file.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We missed updating these in #297.

@dfed dfed merged commit 23bad01 into master Oct 19, 2024
10 checks passed
@dfed dfed deleted the dfed--swift-6 branch October 19, 2024 04:30
@dfed dfed mentioned this pull request Nov 10, 2024
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.

1 participant