search

Automattic / Automattic-Tracks-iOS

Client library for tracking user events for later analysis
GNU General Public License v2.0
43 stars 12 forks source link

Add support for Swift Package Manager #186

Closed bjhomer closed 3 years ago

bjhomer commented 3 years ago

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

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:

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

Other changes of note.

Ecarrion commented 3 years ago

Great work @bjhomer!

I tried an early integration on the Woo App and these are my findings.

  1. Tracks events work as expected!

    Screen Shot 2021-07-29 at 10 37 29 AM

  2. We end up with multiple dependencies on CocoaLumberJack. I tried removing the Cocoapods ones, but the whole project starts to scream. I spent 30 minutes looking at it, but wasn’t able to fix it right away.

The project outputs warnings about this, EG:
Class DDAbstractDatabaseLogger is implemented in both /private/var/containers/Bundle/Application/43E99593-901F-4F18-9371-5E41A513FF75/WooCommerce.app/Frameworks/Networking.framework/Networking (0x10a511868) and /private/var/containers/Bundle/Application/43E99593-901F-4F18-9371-5E41A513FF75/WooCommerce.app/Frameworks/CocoaLumberjack.framework/CocoaLumberjack (0x1058d2420). One of the two will be used. Which one is undefined.
  1. I tried generating a crash/error on Sentry, but I’m not sure if it is not showing because something is wrong, or because I'm on DEBUG/Local configurations.

Maybe @jkmassel could help with 2. and 3.? 😅

bjhomer commented 3 years ago

@Ecarrion Thanks for checking it out! One clarification: are you adding the library via Cocoapods or via SPM in your test here? I'm anticipating that nothing will have changed for users who are integrating via Cocoapods, so I'm guessing you're using SPM. Is that correct?

I'm guessing that having Tracks pull in CocoaLumberjack via an SPM dependency and having the app pull it in via pods is likely going to result in the duplicate framework issues you are seeing here. @jkmassel: we could resolve this by adding the logging protocol as you suggested. I'm happy to do that; the only caveat is that it will mean that this is not a source-compatible update, and clients of the library would have to make some changes to preserve their logging. Do you feel like that's acceptable?

My guess is that having a single app that uses both Cocoapods and SPM for dependencies is likely to run into problems any time the same dependency is being added by both systems.

Ecarrion commented 3 years ago

@bjhomer

so I'm guessing you're using SPM. Is that correct?

Correct!

jkmassel commented 3 years ago

@bjhomer

the only caveat is that it will mean that this is not a source-compatible update, and clients of the library would have to make some changes to preserve their logging. Do you feel like that's acceptable

I do – we're not at 1.0.0 yet on this project, so there's been several source-breaking changes already – IMHO this one is critical anyway for moving forward so I'm more than happy for us to have to deal with it now 🙂

bjhomer commented 3 years ago

Okay, I think this is ready to review again.

jkmassel commented 3 years ago

@mokagio – tagging you for review in this, because I had to replace the work done in https://github.com/Automattic/Automattic-Tracks-iOS/pull/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.

bjhomer commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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:

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 commented 3 years ago

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.

bjhomer commented 3 years ago

Great, I'm gonna merge it!