Add support for Swift Package Manager #186

bjhomer · 2021-07-27T15:29:47Z

This PR adds support for Swift Package Manager!

Don't be scared off by the huge number of changes. The majority of the deletions are removing the old .xcodeproj package that is no longer needed, and the majority of the additions are in the new Package.swift file that describes the package structure.

A few highlights to help guide you through this PR:

Source compatibility

This should be fully source-compatible with anyone using Tracks via Cocoapods. (Please check me on that, though!) And switching an existing project over to SPM from Cocoapods should be as simple as adding the SPM dependency and removing the Cocoapod. No code changes should be required.

Organizational changes

When built under SPM, the single "AutomatticTracks" library has now been split into multiple separate modules that implement the various pieces of functionality

AutomatticTracksEvents for reporting events
AutomatticExperiments for running A/B tests using ExPlat
AutomatticRemoteLogging for uploading app logs and crash logs
AutomatticCrashLoggingUI for displaying crash logs in a SwiftUI view
AutomatticTracks imports all of the above

There are also a couple other modules that just implement shared code: AutomatticTracksModel and AutomatticTracksModelObjC. These aren't very useful on their own, but it seemed useful to pull out the common code.

If you're using the library via Cocoapods, we still just expose import AutomatticTracks as before, so there should be no source changes needed there.

Directory structure

The directory structure has also changed. SPM really wants all files of a single target to belong to a single directory. Because SPM can't have Swift and ObjC code in the same target, I had to make some changes anyway, and since I was doing so, I decided to bring them more in line with what SPM would usually produce. Here's a rough overview of the changes:

Before:
After:

Most of those loose files have just been deleted, as they were unnecessary.

Other changes of note.

The README has been updated to explain how to install using SPM.
The "Reachability" dependency has been removed. It was using deprecated technology, which is now implemented using nw_path_monitor.
I've commented inline on other changes, where relevant.

This is OCMock 3.8.1, downloaded from ocmock.org and built into an xcframework using the following command: ```bash xcodebuild -create-xcframework -framework OCMock/iOS/OCMock.framework -framework OCMock/Mac/OCMock.framework -output OCMock.xcframework ``` We were previously pulling in OCMock using Cocoapods, but we are migrating from that to Swift Package Manager. However, OCMock uses unsafe flags in its SPM package manifest, and Xcode 12.5 does not allow dependencies on a package with unsafe flags in its manifest. (Xcode 13 does, so long as you pin to a particular commit, but as of this writing Xcode 13 is still in beta.)

Because tests don't run on iOS devices, our OCMock xcframework doesn't include a slice for physical devices. That means that the tests fail to compile if you're building them for an iOS device destination. But we don't need that anyway, because Xcode already prevents the tests from running in that case.

…e structure accordingly.

Cocoapods have been removed from the demo app.

…pdate it. This preserves backward-compatibility with everyone already using Tracks.

This prevents us from needing an entirely separate module for just one ObjC file.

Calling it "AutomatticTracksiOS" felt wrong, since it also works on Mac.

bjhomer · 2021-08-10T13:51:59Z

Okay, I think this is ready to review again.

…age-manager

jkmassel · 2021-08-26T22:44:43Z

@mokagio – tagging you for review in this, because I had to replace the work done in #147 – even with the test target sandboxed, the tests ask for access to the user's Documents directory.

After this change, the Tracks DB will always live in the Application Support directory – whether inside the container, or inside ~/Library/Application Support – thus never triggering this prompt.

The downside is that we'll lose some tracks events in the transition, but it shouldn't be very many, we should only need to do it once, and it'll only be on macOS.

mokagio

Thank you for doing all this great work @bjhomer. Impressive.

A question for @jkmassel: this PR updates both CircleCI and Buildkite. Is it intentional? Are we waiting to remove CircleCI in a dedicated PR?

The tests now run on a sandboxed app. Ideally, I would like to test this in a non-sandboxed app before approving, but I'm running out of time today. Besides, the only client that is not sandboxed is AutoProxxy, on which we are not planning to do any change soon (which is unfortunate, but we have bigger fishes to fry 🤷 ) It might be cool to add a new build configuration for the macOS demo app, to which we could add a different entitlement file which doesn't use sandboxing. But doesn't have to happen here 👍

Also, this is happening on my local copy:

🙃

Is it just me? I'm approving this under the assumption that it's just me.

There's a few questions in which I could have checked the result myself. I wanted to do so, but between my Xcode being stuck in that resolve packages loop and GitHub refusing to scrolling somehow, I'm choosing the lazy route and simply leaving the questions for you to answer. Sorry about that 😞

mokagio · 2021-09-13T03:15:38Z

Package.swift

+        // target that _only_ builds that one dependency.
+        // We ignore any failures when building this target.
+        // Then we go on to build the actual product, which
+        // builds correctly.


Is there a radar for this? Do you happen to have tested it on Xcode 13?

Yes, it works fine on Xcode 13. When I originally put up this PR, Xcode 13 was still a few months out so I was shooting for compatibility with Xcode 12. But since the final Xcode 13 build is likely to be coming out this week, we may be able to just drop this altogether. (On the other hand, this workaround doesn't cause any problems in Xcode 13 either.)

It is mentioned as fixed in the Xcode 13 b3 release notes:

Fixed an issue where resolution of packages with binary dependencies failed to extract the binary archive. (77011310) (FB9085487)

Excellent! Thank you for the update 🙇

In my experience in Automattic Apps Division so far, we haven't been jumping onto the latest Xcode version as soon as released because of the various projects and CI setups. So, I'd lean towards shipping with this and drop it later on.

README.md

Sources/AutomatticTracks/AllExports.swift

Sources/AutomatticTracks/AutomatticTracks.h

Sources/Event Logging/TracksEventService.h

Sources/Remote Logging/RemoteLoggingExports.swift

mokagio · 2021-09-13T04:19:52Z

Sources/Workaround-SPM/File.swift

+//
+//  File.swift
+//  
+//
+//  Created by BJ Homer on 7/27/21.
+//
+


What do you think of replacing this with a few lines explaining why we need this workaround here?

Depending on how soon we're willing to support Xcode 13 in CI builds, we may be able to just remove this workaround altogether. But if we need to support Xcode 12 in CI builds for a while, then yeah, a comment here would make sense.

We're not likely to ship Xcode 13 CI support until about the time macOS Monterey ships (typically most of the bugs are worked out by then). That may change if there are critical features we need from XC13.

mokagio · 2021-09-13T04:21:11Z

Tests/Tests/CrashLoggingTests.swift

    }

    override func tearDown() {
        mockDataProvider.reset()
+        UserDefaults.standard.removeObject(forKey: "force-crash-logging")


Nitpick: we could extract the "force-crash-logging" string in an instance private let to prevent typos in the future.

Yeah, I'll do that.

mokagio · 2021-09-13T04:25:06Z

Tests/Tests/CrashLoggingTests.swift

+        #if SWIFT_PACKAGE
+        // When building for SPM, there is no active application for the tests.
+        XCTAssertEqual("unavailable", event?.tags?["app.state"])


I don't know the answer either, but I would guess it's an SPM thing, something to do with Linux compatibility maybe? When running the tests for a package with swift test, for example, the Simulator doesn't load. Also, in the template projects, tests are simply files in a folder, there's no mention of an application in the Package.swift or in the project Xcode generates to work on the package. 🤔

mokagio · 2021-09-13T04:28:50Z

TracksDemo/TracksDemo Mac/TracksDemo Mac.entitlements

-	<key>IDEDidComputeMac32BitWarning</key>
+	<key>com.apple.security.app-sandbox</key>


Because of the .plist structure, Git considers this a rename, but it's actually two actions:

Add a .entitlements for the Mac demo app to enable sandboxing

Remove the IDEWorkspaceChecks.plist file

I wonder why the checks file was removed 🤔 Is this something you did intentionally, or something Xcode automated away?

The checks file was removed when I removed the entire .xcworkspace file. It's no longer needed, since Package.swift now serves as the project structure. You can see the commit where it was removed here.

Package.swift now serves as the project structure

I assume to open the project in this configuration you either open Package.swift or the entire project folder with Xcode. Right?

In such a case, how do you run the demo app from the Package version of the project in Xcode? 🤔 I tried adding a new scheme for it, but couldn't find a way.

The only way I found was to open the xcodeproj in the TracksDemo folder. When I do that, Xcode regenerates the IDE checks file.

Ah, yep. Opening the .xcodeproj file directly is the expected way to run the demo project; the Package.swift file doesn't reference it at all. (Which makes sense, because users of the library don't need to know about it.)

If you want to just open the library code, you can double-click on the Package.swift file, and it will open the whole Swift Package structure in Xcode.

I don't object to checking in the IDEChecks file for the demo project, though.

Co-authored-by: Gio Lodi <[email protected]>

bjhomer · 2021-09-13T15:31:26Z

@mokagio

Is it just me? I'm approving this under the assumption that it's just me.

I am not seeing that repeated package resolution thing, either in Xcode 12.5 or in Xcode 13 betas. So either it's just you, or maybe you're using a different version of Xcode than the ones I've tested on.

mokagio · 2021-09-14T02:14:35Z

Thank you for taking the time to address my suggestions @bjhomer and for answering my questions 🙇 .

So either it's just you, or maybe you're using a different version of Xcode than the ones I've tested on.

I get that behavior both on 12.5.1 and 13 beta 4 😞 It's not the first time SPM-related stuff behaves weird on my Mac, I'll file it in that same bucked for now.

mokagio · 2021-09-14T02:36:39Z

Something I noticed while doing some experiments around the IDEWorkspaceChecks.plist file (see this comment) is that, with both Xcode 12.5.1 and 13 beta 4, when I open the project the .swiftpm folder is generated.

I'm pretty sure that's the expected behavior, or, at least, is what I've seen in other projects that depend on Package.swift.

The UX around that folder is a bit unclear, but we should at least ignored .swiftpm/xcode. Some references:

https://forums.swift.org/t/files-created-when-opening-spm-package-in-xcode-11/28589/2
Add .swiftpm folder to default GitIgnore file swiftlang/swift-package-manager#2200
Add .swiftpm/xcode to default gitignore swiftlang/swift-package-manager#2209
The default .gitignore configuration pertaining to .swiftpm when creating a new package at the time of writing

The above as well is all based on how I open the code within Xcode: by opening the entire folder with open -a Xcode .. Maybe I'm just not opening it the right way? 🤔

bjhomer · 2021-09-14T04:09:11Z

I've added .swiftpm/xcode to the gitignore, and I've committed the IDEWorkspaceChecks plist file.

I'm on Xcode 13 b5 instead of b4, so it's possible the SPM looping behavior is related to that… but I wouldn't put my hopes on that.

jkmassel

Given the fact that Gio is good with this, I'm onboard to merge it at any time now 🙂

bjhomer · 2021-09-23T20:42:52Z

Great, I'm gonna merge it!

bjhomer added 27 commits July 9, 2021 14:25

Switch dependencies from Cocoapods to Swift Package Manager

e38635a

Adjust Sentry import to work with modules

79e53d6

Fix a test to actually test something.

604ecf4

Standardize Tracks.framework headers

f761f6c

Split the library into a few separate targets, and reorganize the fil…

05dec3e

…e structure accordingly.

Start consolidating targets.

96381f5

Add tests to SPM manifest

584d54a

Get TracksDemo compiling using SPM.

ef25da6

Cocoapods have been removed from the demo app.

Delete unnecessary dependencies!

4f2046b

Make AutomatticExperiments a dependency of TracksService, so it can u…

654f9f1

…pdate it. This preserves backward-compatibility with everyone already using Tracks.

Update Podspec to _hopefully_ still work

b8d6d85

Make Tracks still work for Cocoapods.

c9a8a56

Update Readme.

8a9138d

Rename ABTesting to Experiments

3cfaf4f

Reimplement a small bit of ObjC in Swift.

f54988e

This prevents us from needing an entirely separate module for just one ObjC file.

Fix a test that was failing under SPM

396f8fc

Rename the library product to "AutomatticTracks".

3dadc0b

Calling it "AutomatticTracksiOS" felt wrong, since it also works on Mac.

Remove unused xcodeproj file.

2dd75d8

Fix warnings when building with cocoapods.

1d6f1ed

Reorganize Package.swift

3f6aba7

Delete unused files

cc2778b

Remove unnecessary annotation

9313372

Remove unused function

5fb91eb

Fix code style things

afdc0e0

Update build.sh

ae9db9f

bjhomer force-pushed the bjhomer/swift-package-manager branch 3 times, most recently from 7fc51a0 to 4415eab Compare July 27, 2021 17:04

bjhomer and others added 3 commits August 13, 2021 11:34

Merge remote-tracking branch 'origin/develop' into bjhomer/swift-pack…

5506864

…age-manager

Bump the tooling plugin version

0e76850

Fix version consistency check

ad40359

jkmassel force-pushed the bjhomer/swift-package-manager branch 2 times, most recently from 5ca6ba1 to 1689f40 Compare August 13, 2021 23:40

Fix iOS Tests

95d8df7

jkmassel force-pushed the bjhomer/swift-package-manager branch from 1689f40 to a4e96d9 Compare August 13, 2021 23:48

jkmassel added 3 commits August 26, 2021 16:20

Fix broken tests

1831eae

Try using local-only signing

18dc418

Commit the TracksDemo resolved package list

7c91b79

jkmassel force-pushed the bjhomer/swift-package-manager branch from e9025cc to 65bad60 Compare August 26, 2021 22:20

Use sandboxing for the Tracks demo app

b8f8af4

jkmassel force-pushed the bjhomer/swift-package-manager branch from 74fa9da to b8f8af4 Compare August 26, 2021 22:43

jkmassel requested a review from mokagio August 26, 2021 22:44

mokagio approved these changes Sep 13, 2021

View reviewed changes

bjhomer and others added 2 commits September 13, 2021 09:26

Apply suggestions from code review

6013661

Co-authored-by: Gio Lodi <[email protected]>

Apply more suggestions from code review

9eba0c5

Update gitignore and add Xcode-generated file

731adc2

jkmassel self-requested a review September 14, 2021 16:50

jkmassel approved these changes Sep 14, 2021

View reviewed changes

bjhomer merged commit d923cb3 into develop Sep 23, 2021

bjhomer deleted the bjhomer/swift-package-manager branch September 23, 2021 20:43

mokagio mentioned this pull request Mar 9, 2022

Ensure network monitor path update handler is called on start #199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Swift Package Manager #186

Add support for Swift Package Manager #186

bjhomer commented Jul 27, 2021 •

edited

Loading

bjhomer commented Aug 10, 2021

jkmassel commented Aug 26, 2021

mokagio left a comment

mokagio Sep 13, 2021

bjhomer Sep 13, 2021 •

edited

Loading

mokagio Sep 14, 2021

mokagio Sep 13, 2021

bjhomer Sep 13, 2021

jkmassel Sep 14, 2021

mokagio Sep 13, 2021

bjhomer Sep 13, 2021

mokagio Sep 13, 2021

mokagio Sep 13, 2021

bjhomer Sep 13, 2021

mokagio Sep 14, 2021

bjhomer Sep 14, 2021

bjhomer Sep 14, 2021

bjhomer commented Sep 13, 2021

mokagio commented Sep 14, 2021

mokagio commented Sep 14, 2021

bjhomer commented Sep 14, 2021

jkmassel left a comment

bjhomer commented Sep 23, 2021

		<key>IDEDidComputeMac32BitWarning</key>
		<key>com.apple.security.app-sandbox</key>

Add support for Swift Package Manager #186

Add support for Swift Package Manager #186

Conversation

bjhomer commented Jul 27, 2021 • edited Loading

Source compatibility

Organizational changes

Directory structure

Other changes of note.

bjhomer commented Aug 10, 2021

jkmassel commented Aug 26, 2021

mokagio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjhomer Sep 13, 2021 • edited Loading

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

bjhomer commented Sep 13, 2021

mokagio commented Sep 14, 2021

mokagio commented Sep 14, 2021

bjhomer commented Sep 14, 2021

jkmassel left a comment

Choose a reason for hiding this comment

bjhomer commented Sep 23, 2021

bjhomer commented Jul 27, 2021 •

edited

Loading

bjhomer Sep 13, 2021 •

edited

Loading