Switch to Swift Package Manager #527

taquitos · 2021-06-10T22:04:40Z

Supporting M1/X86_64 plus Xcode 12.5 for development has been a challenge with the current setup.

Removes carthage for testing dependencies and relies on SPM now
Added some helper functions since Nimble doesn't support expectAssertion() when built with SPM
Throw errors for bit manipulation instead of fatal()
Update Fastfile and CircleCI config

taquitos · 2021-06-10T22:05:33Z

I tried to separate out some of these commits into individual PRs, but you can see the error of my ways. It was much easier to do 1 large-ish PR to fix the build issues.

Sorry!

aboedo · 2021-06-11T15:52:24Z

@taquitos is this ready for review? if so feel free to request a review from the SDK team

Nimble doesn't support raiseException() when built with SPM - paves way for SPM support and M1 support

Specify specific BitShiftError cases in test

taquitos · 2021-06-11T17:38:04Z

Just force-pushed for a rebase on the swiftlint updates.

aboedo

🎉🎉🎉🎉
looks great! left a few comments

carthage.sh

aboedo · 2021-06-11T16:34:50Z

.circleci/config.yml

-
-  integration-tests-carthage:
-    macos:
-      xcode: "12.5.0"
-    working_directory: ~/purchases-ios/
-    shell: /bin/bash --login -o pipefail
-    steps:
-      - checkout
-      - trust-github-key
-      - update-carthage-integration-commit
-      # Carthage
-      - restore_cache:
-          keys: 
-            - carthage-cache-{{ checksum "Cartfile.resolved" }}
-      - run:
-          name: Carthage Update
-          working_directory: IntegrationTests/CarthageIntegration/
-          # install without building, then remove the tests and build, so that carthage
-          # doesn't try to build the other integration tests
-          command: |
-              ./carthage.sh update --no-build
-              rm -rf Carthage/Checkouts/purchases-root/IntegrationTests/
-              ./carthage.sh build
-      - save_cache:
-          key: carthage-cache-{{ checksum "Cartfile.resolved" }}
-          paths:
-            - Carthage
-
-      - install-gems-scan-and-archive:
-          directory: IntegrationTests/CarthageIntegration/


these tests are to test that you can integrate our sdk using carthage, we should keep them

I was just making sure you were paying attention 👀

😄

aboedo · 2021-06-11T16:38:13Z

Purchases.xcodeproj/project.pbxproj

@@ -2372,6 +2357,63 @@
 			defaultConfigurationName = Release;
 		};
 /* End XCConfigurationList section */
+
+/* Begin XCRemoteSwiftPackageReference section */
+		B3D5CFBB267282630056FA67 /* XCRemoteSwiftPackageReference "Nimble" */ = {


just double-checking:
since we have these dependencies at the target level, but not in Package.swift, clients of the SDK using SPM won't actually download them, correct?

That's my understanding, it's a dependency to build the test targets. Xcode might download them automatically because it could assume you want to make sure the tests pass, but I'm not sure.

Just tested with a new project, I pointed SPM to this branch and it only checked out the SDK 🎉

:chefskiss:

aboedo · 2021-06-11T16:39:08Z

PurchasesCoreSwift/LocalReceiptParsing/DataConverters/UInt8+Extensions.swift

+enum BitShiftError: Error {
+
+    case invalidIndex(_ index: UInt8)
+    case rangeFlipped(from: UInt8, to: UInt8)
+    case rangeLargerThanByte
+    case unhandledRange
+
+}


aboedo · 2021-06-11T16:41:17Z

PurchasesTests/PurchasesTests-Bridging-Header.h

@@ -1,50 +1,52 @@
 //
 //  Use this file to import your target's public headers that you would like to expose to Swift.
 //
-#include <Purchases/RCPurchases.h>


glad to see these alphabetized

It made my eye twitch when I read it. Sorry, I had to do it 😄

aboedo · 2021-06-11T18:20:28Z

PurchasesTests/TestHelpers/RCObjC.h

+
+@interface RCObjC : NSObject
+
+ (BOOL)catchException:(void(^)(void))tryBlock error:(__autoreleasing NSError **)error;


should this method name reference its first param? we can always use NS_SWIFT_NAME to skip its name in swift

I'm not strongly opposed to either way, I'll update and see what you think about it!

aboedo · 2021-06-11T18:21:46Z

fastlane/Fastfile

@@ -99,7 +99,6 @@ platform :ios do
    version_number = current_version_number
    check_no_git_tag_exists(version_number)
    check_pods
-    carthage_archive


this one is here to make sure that we can create a carthage archive. The contents get deployed with releases, so that carthage can use it by default instead of re-compiling whenever someone does carthage update Purchases

aboedo · 2021-06-11T18:22:20Z

fastlane/Fastfile

@@ -123,7 +122,6 @@ platform :ios do
  lane :deploy do |options|
    version_number = current_version_number
    push_pods
-    carthage_archive


we should keep the carthage_archive, since we're not removing compatibility with carthage, we're just not using it for development anymore

aboedo · 2021-06-11T18:23:24Z

fastlane/Fastfile

-  # can't use fastlane carthage integration directly because of Carthage/Xcode 12 compatibility issues:
-  # https://github.com/Carthage/Carthage/issues/3019
-  Dir.chdir("..") do
-    sh("./carthage.sh", "build", "--no-skip-current")
-    sh("./carthage.sh", "archive", "Purchases")


with carthage 0.38 this is no longer true, so we should be able to do
carthage build --no-skip-current && carthage archive Purchases. So I'd update this but not remove it

aboedo · 2021-06-11T18:24:52Z

.circleci/config.yml

@@ -353,7 +297,6 @@ workflows:
      - deployment-checks: *release-branches
      - integration-tests-cocoapods: *release-tags-and-branches
      - integration-tests-swift-package-manager: *release-tags-and-branches
-      - integration-tests-carthage: *release-tags-and-branches


we actually should probably run all of these tests on the branch since it makes structural changes, just to make sure that everything still works afterwards on all package managers

aboedo · 2021-06-11T20:59:41Z

@taquitos lmk when you'd like me to do another pass

taquitos · 2021-06-11T21:03:43Z

@aboedo all ready for another pass 😃

aboedo

thanks for doing this!! I left a couple of minor comments and it's ready to go

aboedo · 2021-06-11T21:07:36Z

Purchases.xcodeproj/project.pbxproj

-/* Begin PBXShellScriptBuildPhase section */
-		2D49B31D253F7D1B003CBC8B /* Copy Carthage Frameworks */ = {
-			isa = PBXShellScriptBuildPhase;


tears of joy

aboedo · 2021-06-11T21:10:59Z

PurchasesTests/TestHelpers/ObjCThrowExceptionMatcher.swift

@@ -0,0 +1,84 @@
+//
+//  ObjCThrowExceptionMatcher.swift


Maybe we should add a note here, pointing to the line in Nimble that specifies that their matcher doesn't work with SPM, so that future devs understand why we have our own matcher specifically for this

Ayyy makes sense.

aboedo · 2021-06-11T21:14:05Z

PurchasesTests/TestHelpers/ObjCThrowExceptionMatcher.swift

+    }
+}
+
+func messageForError(error: NSError?, named: String?) -> ExpectationMessage {


let's make these private, since they're only meant to be inside this file but they're global

…tive

taquitos mentioned this pull request Jun 10, 2021

CicrcleCI seems to have a stale cache, updating cache keys #526

Closed

taquitos requested a review from a team June 11, 2021 16:11

taquitos added 6 commits June 11, 2021 10:35

Initial removal of some fatalError calls

a841844

Update for Nimble SPM limitations

aedb389

Nimble doesn't support raiseException() when built with SPM - paves way for SPM support and M1 support

Update UInt8+ExtensionsTests.swift

12696f1

Specify specific BitShiftError cases in test

Let's just remove carthage while we're at it.

8b49d02

Remove integration-tests-carthage from config.yml

143071d

Update fastlane Fastfile for removing carthage

dc7465a

taquitos force-pushed the fatalToError branch from 4518f4e to dc7465a Compare June 11, 2021 17:37

Linter errors

a996cdc

aboedo requested changes Jun 11, 2021

View reviewed changes

taquitos added 2 commits June 11, 2021 13:47

Updates for comments

1255991

Fix config.yml

23e7ed0

taquitos requested a review from aboedo June 11, 2021 21:03

aboedo approved these changes Jun 11, 2021

View reviewed changes

taquitos added 2 commits June 11, 2021 14:27

Update ObjCThrowExceptionMatcher.swift

2528b58

private instead of fileprivate, I always forget which is more restric…

da7bafe

…tive

taquitos merged commit 79008f5 into main Jun 11, 2021

taquitos deleted the fatalToError branch June 11, 2021 22:21

aboedo mentioned this pull request Jul 13, 2021

Release: 3.12.0 #648

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to Swift Package Manager #527

Switch to Swift Package Manager #527

taquitos commented Jun 10, 2021

taquitos commented Jun 10, 2021

aboedo commented Jun 11, 2021

taquitos commented Jun 11, 2021

aboedo left a comment

aboedo Jun 11, 2021

taquitos Jun 11, 2021

aboedo Jun 11, 2021

taquitos Jun 11, 2021

taquitos Jun 11, 2021

aboedo Jun 11, 2021

aboedo Jun 11, 2021

taquitos Jun 11, 2021

aboedo Jun 11, 2021

taquitos Jun 11, 2021

aboedo Jun 11, 2021

taquitos Jun 11, 2021

aboedo Jun 11, 2021

taquitos Jun 11, 2021

aboedo Jun 11, 2021

taquitos Jun 11, 2021

aboedo Jun 11, 2021

taquitos Jun 11, 2021

aboedo Jun 11, 2021

taquitos Jun 11, 2021

aboedo commented Jun 11, 2021

taquitos commented Jun 11, 2021

aboedo left a comment

aboedo Jun 11, 2021

aboedo Jun 11, 2021

taquitos Jun 11, 2021

aboedo Jun 11, 2021

taquitos Jun 11, 2021


		@interface RCObjC : NSObject

		+ (BOOL)catchException:(void(^)(void))tryBlock error:(__autoreleasing NSError **)error;

Switch to Swift Package Manager #527

Switch to Swift Package Manager #527

Conversation

taquitos commented Jun 10, 2021

taquitos commented Jun 10, 2021

aboedo commented Jun 11, 2021

taquitos commented Jun 11, 2021

aboedo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo commented Jun 11, 2021

taquitos commented Jun 11, 2021

aboedo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment