Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Swift Package Manager #47

Merged
merged 14 commits into from
Nov 24, 2020
Merged

Conversation

Sherlouk
Copy link
Contributor

@Sherlouk Sherlouk commented Nov 13, 2020

Supersedes #24

  • Removed AccessibilitySnapshot directory in favour of a Sources directory (which is closer to the Swift Package Manager expected format)
  • Updated the CocoaPods installation method to point to the new file locations
  • Minor tweaks to source files to make them work with Swift Package Manager (uses compiler flags as to allow CocoaPods support to remain unaffected)
  • Added Package.swift!! 🙌

Incomplete:

  • Update Documentation

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
Copy link
Contributor Author

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

@Sherlouk Sherlouk marked this pull request as ready for review November 18, 2020 22:14
Copy link
Collaborator

@NickEntin NickEntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! Just a few small comments plus adding install instructions to the README, and I think this should be good to go

.gitignore Outdated Show resolved Hide resolved
Package.swift Outdated Show resolved Hide resolved
@NickEntin
Copy link
Collaborator

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
Copy link
Contributor Author

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

@Sherlouk
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Sherlouk Sherlouk changed the title Add Swift Package Manager (Number Two) Add Swift Package Manager Nov 22, 2020
@NickEntin
Copy link
Collaborator

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
Copy link
Contributor Author

Sherlouk commented Nov 24, 2020

@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
Copy link
Contributor Author

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!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
AccessibilitySnapshot.podspec Outdated Show resolved Hide resolved
@NickEntin
Copy link
Collaborator

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
Copy link
Contributor Author

Sherlouk commented Nov 24, 2020

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
Copy link
Collaborator

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! 🎉

Example/Podfile.lock Outdated Show resolved Hide resolved
@NickEntin NickEntin merged commit e0d63f0 into cashapp:master Nov 24, 2020
@Sherlouk Sherlouk deleted the swift-package-manager branch November 24, 2020 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants