search

cashapp / AccessibilitySnapshot

Easy regression testing for iOS accessibility
Apache License 2.0
541 stars 68 forks source link

Add Swift Package Manager #47

Closed Sherlouk closed 3 years ago

Sherlouk commented 3 years ago

Supersedes #24

Incomplete:

Tried to keep the changes as small as possible in this first merge request allowing us to iterate more easily!

Project builds correctly when opening the Swift Package (but no tests). Have also run pod install and then run the tests in the Example project and they're working fine for me. (Let's see how CI does!)

@NickEntin - Let me know what you think on these changes 😄

Sherlouk commented 3 years ago

Awesome looks like CI is happy. Care to take a look when you're not busy @NickEntin? 🙏 Thanks!

I can also update some docs once you're happy

NickEntin commented 3 years ago

Oh, we should also add a CI build for this. I pushed a commit to add the CI build, but I'm getting errors when I run it locally.

Sherlouk commented 3 years ago

Done some basic docs - just need to get the CI build working 👍

Sherlouk commented 3 years ago

Pushed up one change which seems to make it a lot happier than it was...

I'm also trying to run the build via the Swift command itself running:

swift build -Xswiftc "-sdk" -Xswiftc "`xcrun --sdk iphonesimulator --show-sdk-path`" -Xswiftc "-target" -Xswiftc "x86_64-apple-ios14.0-simulator"

but it seems to be freaking out on the lack of UIKit. All very peculiar because in a normal project it runs absolutely fine!

One idea might be to setup an actual Xcode project which uses the library via a local dependency? If we can build/test that (add one basic example?) then it would prove what we need it to and circumvent the direct building stupidities?

Sherlouk commented 3 years ago

Spoke to someone at work who knows way more about Swift than me and he pointed me down the right track!

tl;dr swift package generate-xcodeproj should be avoided. If you don't specify a project then xcodebuild will pickup the Package.swift and do what it's needs to do.

All working locally using iOS 14

NickEntin commented 3 years ago

Ahh, interesting! I didn't know that xcodebuild would automatically pick up a Package.swift. It still seems odd to me that generating an Xcode project doesn't work though. It looks like there's two general sets of errors. The first is that Bundle.module can't be found, which is fixed by specifying the resources in the package definition:

         .target(
             name: "AccessibilitySnapshotCore",
             dependencies: ["AccessibilitySnapshotCore-ObjC"],
-            path: "Sources/AccessibilitySnapshot/Core/Swift"
+            path: "Sources/AccessibilitySnapshot/Core/Swift",
+            resources: [.process("Assets")]
         ),

The second set of errors is around undefined symbols in UIKit. That one I'm not sure about. Any idea why it wouldn't linking UIKit correctly when generating the Xcode project? Avoiding generating the Xcode project in CI seems reasonable, but we should make sure this isn't revealing some other underlying issue.

Sherlouk commented 3 years ago

@NickEntin from my understanding this is just a bug with the project that gets generated - a lot of the flags that are needed for an iOS target haven't been configured correctly (bare in mind that iOS support is relatively new to SPM!)

I have tried this exhaustively in projects and haven't seen any problems - I'm pretty confident what we were seeing on CI was just an issue with the approach (and not something you'd do in a standard iOS project)

Sherlouk commented 3 years ago

I had (and can push it to the project if you'd like) made a completely separate CI iOS Xcode project which just showed that including the package normally and using it in a test worked absolutely fine.

This might be another option if you're not confident with the current CI approach!

NickEntin commented 3 years ago

Got it. As long as the test project was working for you, I think this should be fine. 👍

Just a couple small nits, and then I think we're good to merge!

Sherlouk commented 3 years ago

As I say the purpose of CI is to give us confidence -- more than happy to explore other changes and improvements. Or we can just wait for the first issue to be raised and discuss how wrong I am in there 😅

Nits all committed 🙌 Thanks Nick!

NickEntin commented 3 years ago

Haha I think I'm pretty confident in this now. Just trying to double check everything as I go since I don't have a lot of context around SPM. I appreciate all the investigation you've done here! 🙌

As soon as this goes green, I'll merge the PR and enable the SPM Build job as a required check.

Thanks for another great addition, James! 🎉