Update project and installation methods (CocoaPods and Swift Package Manager) for Xcode 15 #327
Conversation
Makefile
Outdated
| # TODO: replace it with "swift test --enable-test-discovery --sanitize=thread" when swiftPM resource-related bug would be fixed. | ||
| # https://bugs.swift.org/browse/SR-13560 | ||
| swift build | ||
| swift test --enable-test-discovery --sanitize=thread |
There was a problem hiding this comment.
The linked SPM bug has been resolved for some time.
| s.osx.deployment_target = '10.8' | ||
| s.tvos.deployment_target = '9.0' | ||
| s.watchos.deployment_target = '2.0' | ||
| s.cocoapods_version = '>= 1.13.0' |
There was a problem hiding this comment.
This is a common pattern used across other popular open source libraries, ensuring the VisionOS platform is recognized and Resource Bundles are properly generated.
| end | ||
| s.subspec 'Arc-exception-safe' do |sp| | ||
| sp.dependency 'PINCache/Core' | ||
| sp.source_files = 'Source/PINDiskCache.m' | ||
| sp.compiler_flags = '-fobjc-arc-exceptions' | ||
| end | ||
| s.resource_bundles = { 'PINCache' => ['Source/PrivacyInfo.xcprivacy'] } |
There was a problem hiding this comment.
This is an important line. Adding a Privacy Manifest to resources would end up clobbering an app's Privacy Manifest. Popular open source libraries (Firebase, Alamofire, etc.) have been using this approach for several months with no issues.
| @@ -30,9 +30,10 @@ let package = Package( | |||
| dependencies: ["PINOperation"], | |||
| path: "Source", | |||
| exclude: ["Info.plist"], | |||
| resources: [.copy("../PrivacyInfo.xcprivacy")], | |||
| publicHeadersPath: ".", | |||
| resources: [.process("PrivacyInfo.xcprivacy")], | |||
There was a problem hiding this comment.
The Privacy Manifest file needs to be added and referenced within the Xcode project, which has been done as part of this PR. Now, we can simply process the resource instead of copying.
Source/PINCache.h
Outdated
There was a problem hiding this comment.
Using angled brackets is the preferred way of importing framework headers. Now that the Swift Package Manager Package file references a public header folder of symlinks, we can use angled brackets as expected.
Source/PINDiskCache.m
Outdated
There was a problem hiding this comment.
The only thing I've done in this file is removed the availability checks, which were only required when targeting much lower deployment targets.
ghost
mentioned this pull request
|
Ready for review. NOTE: I have a similar PR to open for PINOperation, which I've linked in the PR description. cc: @garrettmoon @garricn |
| # https://bugs.swift.org/browse/SR-13560 | ||
| swift build | ||
| # FIXME: After resolving data races, change the below line to: swift test --sanitize thread | ||
| swift test |
There was a problem hiding this comment.
I was testing the project locally with Thread Sanitizer enabled and it reported one issue (which fails the SPM CI test):
WARNING: ThreadSanitizer: data race (pid=76335)
Write of size 1 at 0x0001057e13ea by thread T7 (mutexes:
write M0):
#0 -[PINDiskCache initializeDiskProperties] PINDiskCache.m:594 (PINCachePackageTests:arm64+0xe920)
#1 __134-[PINDiskCache initWithName:prefix:rootPath:serializer:deserializer:keyEncoder:keyDecoder:operationQueue:ttlCache:byteLimit:ageLimit:]_block_invoke PINDiskCache.m:284 (PINCachePackageTests:arm64+0xa998)
#2 __tsan::invoke_and_release_block(void*) <null>:4312708 (libclang_rt.tsan_osx_dynamic.dylib:arm64e+0x802e4)
#3 _dispatch_client_callout <null>:4312708 (libdispatch.dylib:arm64e+0x43fc)
Previous read of size 1 at 0x0001057e13ea by main thread:
#0 -[PINDiskCache diskStateKnown] PINDiskCache.m:84 (PINCachePackageTests:arm64+0x1b7e4)
#1 -[PINCacheTests testDiskReadingAfterCacheInit] PINCacheTests.m:863 (PINCachePackageTests:arm64+0x42230)
#2 __invoking___ <null>:4312708 (CoreFoundation:arm64e+0x617f0)
Location is heap block of size 320 at 0x0001057e13c0 allocated by main thread:
#0 calloc <null>:4312708 (libclang_rt.tsan_osx_dynamic.dylib:arm64e+0x562cc)
#1 _objc_rootAllocWithZone <null>:4312708 (libobjc.A.dylib:arm64e+0xe740)
#2 __invoking___ <null>:4312708 (CoreFoundation:arm64e+0x617f0)
Mutex M0 (0x0001057e14c0) created at:
#0 pthread_mutex_init <null>:4312708 (libclang_rt.tsan_osx_dynamic.dylib:arm64e+0x31470)
#1 -[PINDiskCache initWithName:prefix:rootPath:serializer:deserializer:keyEncoder:keyDecoder:operationQueue:ttlCache:byteLimit:ageLimit:] PINDiskCache.m:221 (PINCachePackageTests:arm64+0x9eec)
#2 -[PINDiskCache initWithName:prefix:rootPath:serializer:deserializer:keyEncoder:keyDecoder:operationQueue:ttlCache:] PINDiskCache.m:186 (PINCachePackageTests:arm64+0x9950)
#3 -[PINDiskCache initWithName:prefix:rootPath:serializer:deserializer:keyEncoder:keyDecoder:operationQueue:] PINDiskCache.m:166 (PINCachePackageTests:arm64+0x963c)
#4 -[PINDiskCache initWithName:rootPath:serializer:deserializer:operationQueue:] PINDiskCache.m:147 (PINCachePackageTests:arm64+0x9384)
#5 -[PINDiskCache initWithName:rootPath:serializer:deserializer:] PINDiskCache.m:137 (PINCachePackageTests:arm64+0x9160)
#6 -[PINDiskCache initWithName:rootPath:] PINDiskCache.m:130 (PINCachePackageTests:arm64+0x8f6c)
#7 -[PINDiskCache initWithName:] PINDiskCache.m:125 (PINCachePackageTests:arm64+0x8e18)
#8 -[PINCacheTests testDiskReadingAfterCacheInit] PINCacheTests.m:859 (PINCachePackageTests:arm64+0x42048)
#9 __invoking___ <null>:4312708 (CoreFoundation:arm64e+0x617f0)
Thread T7 (tid=23818133, running) is a GCD worker thread
SUMMARY: ThreadSanitizer: data race PINDiskCache.m:594 in -[PINDiskCache initializeDiskProperties]
I suggest keeping this out of scope. It can be addressed as a separate issue that eventually leads to enabling Thread Sanitizer within the Xcode project.
There was a problem hiding this comment.
I pushed a few more changes around unit tests.
I won't push anything new until the reviewers have had a chance to take a look at the current state of this PR and provide feedback.
There was a problem hiding this comment.
Agree that keeping this out of scope is what we want to do. 👍
|
It looks a few of the checks are failing. In particular |
|
@rcancro I've made all the necessary updates in this PR to work with the latest in PINOperation's master branch. We won't be able to get all CI tests passing in this PR until a new PINOperation version is released and pushed to CocoaPods trunk. In the meantime, I've attached a patch you can apply to verify this PR is passing all CI tests (except for CocoaPods). The patch points SPM and Carthage to PINOperation's master branch, which has the changes we merged earlier today. Unfortunately, there's no similar trick we can do for CocoaPods. I'll wait for your reply before moving over to create a similar PR for PINRemoteImage. Carthage + SPM CI tests passing patch: spm-carthage-passing-ci-tests.diff.zip |
|
@tstump-phunware Thanks for all your work! Let me try to get a new version of PINOperation pushed to CocoaPods. |
I'm trying to get the pod spec to lint without much luck. Code signing keeps failing. For sanity's sake, I tried to lint AlamoFire's pod spec and got the same errors. Part of me is tempted to give up on cocoapods in favor of SPM. I'm going to be OOO starting tomorrow, but someone else should be looking into this. |
|
@rcancro Thanks for the update. I just locally ran a lint and simulated pushing the PINOperation podspec to CocoaPods trunk. Both commands ran as expected using Xcode 15.2 and CocoaPods 1.15.2: I hope we can resolve this issue, as I currently depend on the CocoaPods installation method. Enjoy your time off! |
|
@tstump-phunware Thanks for your patience on this! PINOperation is updated to 1.2.3 with your PR. For this PR, it looks like we just need to update Carthage, CocoaPods, and SPM to use that version, then revert the CHANGELOG.md, and we should be good to go. |
… Manager) to support building and running with Xcode 15.2
|
@andyfinnell Thanks for helping to move this PR along! I've updated the PR with everything we talked about (using PINOperation 1.2.3 for CocoaPods, SPM, and Carthage, as well as reverting changes to CHANGELOG.md). All CI tests are now passing on my fork, so we should be good to go! ref: https://github.com/tstump-phunware/PINCache/actions/runs/9037258343/job/24835893412. |
|
@tstump-phunware ...and we're merged! Thank you again for all your work on this! I have a couple of other PRs to merge, then I'll roll a new release with this in it. |
|
@andyfinnell Wohoo! Sounds good about the upcoming release -- I can't wait! 🚀 I owe you guys a PR on PINRemoteImage with similar changes. I should have that ready by early next week. I'll keep an eye out for the new PINCache release. |
ghost
mentioned this pull request
Background
As of April 29th, 2024, all apps submitted to the App Store must be built using Xcode 15 (ref: Apple News).
Additionally, all third-party SDKs are required/encouraged to include a Privacy Manifest file (ref: Apple Support).
Purpose
The purpose of these changes is to upgrade the project and bump the minimum deployment targets for Xcode 15:
These changes span across the following:
Additionally, the Swift Package Manager Package file points to a new "include" public header folder of symlinks. This allows the project to use angled bracket imports for public headers without having to suppress any warnings from clang.
This PR fulfills the following open issues and pull requests:
NOTE: A separate PR has been created on PINOperation with similar changes.
Attachments
PINCache after being installed via CocoaPods (also shows PINOperation PR for completeness):
PINCache targets after being installed via CocoaPods (the blue icon represents a resource bundle target that includes the Privacy Manifest file. also shows PINOperation PR for completeness):
PINCache unit tests passing:
