Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

Add Swift Package Manager support #187

Closed
wants to merge 6 commits into from

Conversation

zocario
Copy link

@zocario zocario commented Aug 12, 2020

Hello I'm trying to give a little help regarding support of Swift Package Manager as I need it in one of my project :)
This is a very simple implementation I just did and allowed me to use Family with SPM recently. I'm not sure if this is good enough but I think it is a good base to start with if it helps.

Changes

  • Create target for shared code between macOS and mobile platforms (iOS & tvOS) named Family-Shared.
    This target is used by both macOS and mobile target.
  • Family-mobile encapsulate both tvOS and iOS frameworks.

@zenangst
Copy link
Owner

@zocario Good start mate and thanks for starting to add support for this.
Seems like there is still some additional work to be done on the macOS target, mind taking a look at that?

@zocario
Copy link
Author

zocario commented Aug 12, 2020

@zenangst Not today but I can have a look for macOS at the end of week :)

@zenangst
Copy link
Owner

No worries, I just check to see why Travis wasn't playing ball. Cheers!

@zocario
Copy link
Author

zocario commented Aug 12, 2020

I also forgot to add tests targets, I'll polish this and mention you again ;)

@zocario
Copy link
Author

zocario commented Aug 19, 2020

@zenangst I had a second look and fixed macOS compilation when opening swift package.
But I'm facing two failures test on iOS+tvOS:

  • testAddingMultipleChildViewControllers
  • testViewControllerIsVisibleMethods

I'm not really sure to see how this has changed with my modifications to be honest. If you have time to give your opinion on the changes I've started don't hesitate :)

@zocario
Copy link
Author

zocario commented Aug 20, 2020

Also to me we shouldn't need to use the xcodeproj anymore, but I'm not sure we can work without it regarding travis CI. I'll need to update the xcodeproj to work with the changes I made I'll update this soon.

@zocario zocario force-pushed the feature/spm branch 3 times, most recently from 5baec7d to 7837fd1 Compare September 11, 2020 12:53
@codecov-commenter
Copy link

codecov-commenter commented Sep 11, 2020

Codecov Report

Merging #187 into master will decrease coverage by 1.29%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
- Coverage   81.45%   80.16%   -1.30%     
==========================================
  Files          11       11              
  Lines         836      837       +1     
==========================================
- Hits          681      671      -10     
- Misses        155      166      +11     
Flag Coverage Δ
#iOS 74.25% <90.00%> (-9.75%) ⬇️
#macOS 80.16% <90.47%> (+0.02%) ⬆️
#tvOS 74.25% <90.00%> (+31.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
Sources/macOS/Classes/FamilyDocumentView.swift 91.07% <ø> (ø)
Sources/macOS/Classes/FamilyScrollView.swift 81.52% <ø> (ø)
Sources/macOS/Classes/FamilyWrapperView.swift 83.33% <ø> (ø)
...ces/macOS/Extensions/NSScrollView+Extensions.swift 100.00% <ø> (ø)
...macOS/Extensions/NSViewController+Extensions.swift 66.66% <ø> (ø)
Sources/Shared/FamilySpaceManager.swift 56.25% <83.33%> (-27.63%) ⬇️
Sources/Shared/FamilyCache.swift 100.00% <100.00%> (ø)
...ources/Shared/FamilyViewControllerAttributes.swift 84.21% <100.00%> (ø)
Sources/macOS/Classes/FamilyViewController.swift 78.81% <100.00%> (ø)
Sources/Shared/BinarySearch.swift 73.52% <0.00%> (-5.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9105ff...3c10ce8. Read the comment docs.

@zocario
Copy link
Author

zocario commented Sep 11, 2020

@zenangst So I succeed to setup Travis by using the demo project as container of the swift package. It is using the local swift package and exposes its schemes to be run by Travis. I don't know if this setup would be fine for you (I think it should work for Carthage and Cocoapods but needs to confirm) but this is how I'm doing currently at work to get CI running with swift packages. The constraint is just that the xcodeproj can't be at the root level with this configuration that's why I've used the demo project.

@zenangst
Copy link
Owner

Looking good, I was actually thinking about removing the project all together and perhaps use XcodeGen instead so I think that change would be fine.

@zenangst
Copy link
Owner

Don't have time to do a full review now but perhaps early next week if again… life allows 😎

@zenangst
Copy link
Owner

@zocario mind fixing up the merge conflicts here? (totally my bad 😶)

@zocario
Copy link
Author

zocario commented Sep 29, 2020

@zenangst Done :)

@zocario
Copy link
Author

zocario commented Sep 29, 2020

Also I have a test failure on Mac OS but nothing fails locally for me (10.15.7 Travis is 10.15.6), would you mind trying on your side?

XCTAssertEqual(padding.right, insets.right)
XCTAssertEqual(padding.bottom, insets.bottom)

XCTAssertEqual(container.scrollView.bounds, CGRect(origin: .zero, size: CGSize(width: 500, height: 1000)))
Copy link
Owner

Choose a reason for hiding this comment

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

I removed this line on master, if you do the same I think your branch should work.

@codecov-io
Copy link

Codecov Report

Merging #187 into master will decrease coverage by 1.29%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
- Coverage   81.45%   80.16%   -1.30%     
==========================================
  Files          11       11              
  Lines         836      837       +1     
==========================================
- Hits          681      671      -10     
- Misses        155      166      +11     
Flag Coverage Δ
#iOS 74.25% <90.00%> (-9.75%) ⬇️
#macOS 80.16% <90.47%> (+0.02%) ⬆️
#tvOS 74.25% <90.00%> (+31.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
Sources/macOS/Classes/FamilyDocumentView.swift 91.07% <ø> (ø)
Sources/macOS/Classes/FamilyScrollView.swift 81.52% <ø> (ø)
Sources/macOS/Classes/FamilyWrapperView.swift 83.33% <ø> (ø)
...ces/macOS/Extensions/NSScrollView+Extensions.swift 100.00% <ø> (ø)
...macOS/Extensions/NSViewController+Extensions.swift 66.66% <ø> (ø)
Sources/Shared/FamilySpaceManager.swift 56.25% <83.33%> (-27.63%) ⬇️
Sources/Shared/FamilyCache.swift 100.00% <100.00%> (ø)
...ources/Shared/FamilyViewControllerAttributes.swift 84.21% <100.00%> (ø)
Sources/macOS/Classes/FamilyViewController.swift 78.81% <100.00%> (ø)
Sources/Shared/BinarySearch.swift 73.52% <0.00%> (-5.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9105ff...7669cf6. Read the comment docs.

@zenangst
Copy link
Owner

zenangst commented Nov 4, 2020

Hey @zocario, I tried this branch and I was unable to install it using CocoaPods.
I ended up with the following error inside of FamilyScrollView

image

@zenangst
Copy link
Owner

zenangst commented Nov 5, 2020

I'm curious if we could refactor the Package.swift to only produce one framework per platform. I think that would fix the issue with the module issue that arises when trying to use it with CocoaPods.

@zocario
Copy link
Author

zocario commented Dec 7, 2020

Hey @zenangst I'm sorry I completely missed your last message, I'll take an other look this week-end, big weeks at work at the moment 🙈

@zenangst
Copy link
Owner

zenangst commented Dec 7, 2020

@zocario no worries mate :)

Create target for shared code between macOS and mobile platforms (iOS & tvOS) named Family-Shared. This target is used by both macOS and mobile target. Family-mobile encapsulate both tvOS and iOS frameworks.
Use demo project for travis to expose package targets.

Add Family using local package in example project.
@zenangst
Copy link
Owner

zenangst commented Jan 20, 2021

@zocario Hmm, I'm still having issues installing this using CocoaPods

@zenangst
Copy link
Owner

@zocario I opened a new PR here - #207 that takes a different approach. Instead of creating a shared framework, it uses pre-conditions per platform. The major benefit here is that it still works when using it with CocoaPods.

@zocario
Copy link
Author

zocario commented Jan 21, 2021

@zenangst Looks great! Way better approach that my try :) To be honest I wasn't sure about how to do this in an other way, and I had a lots of things to do at work. I should have thought about pre-conditions! I close the PR ;)

@zocario zocario closed this Jan 21, 2021
@zenangst
Copy link
Owner

@zocario no worries mate, thanks for all the hard work that you put into this! ❤️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants