Migrate Moya to a swift package manager compatible layout

[AndrewSB]

Picks off where #698 left off. Closes #870.

My concerns:

• Test to make sure the project is still CocoaPods compatible I'm not familiar with Cocoapods, so the new layout is done to Carthage & SPM best practices, but it would be awesome to have a cocoapods person test it and make sure everything still woks :)
• Update README: I'm sure some of the instructions will have changed, locations of Demos & Tests, pulling down dependencies for Demos, etc.
• Update CI for new test locations: The tests are currently failing because the tests CircleCI is expecting are no longer in the same. I also have them setup to pull dependencies down with Carthage instead, so we need to set that up. @BasThomas: I remember you offering to take a look

Future enhancements:

• Use swift test once Swift package manager support #643 is merged
• Implement the Carthage caching strategy @scottrhoyt & I discussed in Reduce CircleCI build time by downloading/caching frameworks. #870

[scottrhoyt]

Cool. I actually just started a branch to work on the tests portion today. It also included migrating CircleCI to test out of Moya.xcodeproj and speeding up builds by caching carthage and fixing the problem from #870. Take a look and maybe we can figure out how to combine them 👍.

[AndrewSB]

Haha, I was actually editing my comment to add that as an enhancement!

I have 3-4 more commits to finish up on this PR, and after which I'll branch off to take a shot at #870. How far did you get?

[scottrhoyt]

Haha. 🥇 Priceless.

Pretty much done on my end. I had to implement the cache because the builds slow down majorly once we need to use carthage for testing. I was just figuring out how to fix the rake test:carthage task so that it also would be speedier. I don't think it is really even doing what is intended right now since it's checking out an old tag instead of building the current commit. Take a look at sh_move_tests branch.

[scottrhoyt]

I've also been testing it with cocoapods to make sure nothing breaks there and I added a new task to build the Demo project just to ensure it builds since nothing else is doing that now.

[AndrewSB]

Cool, that sounds great.

I'm going to get the tests building locally, and then I think I'm done with this PR.
I'm seeing a strange error with Result, have you seen anything like it?

I made sure I was using a current version of Result (I have 3.1.0 checked out), I've never seen Result used without an error type specialization

[scottrhoyt]

No, I haven't run in to that. Just opened up a PR to facilitate comparing and getting this all integrated. Build times are greatly sped up. 😁 #886

[BasThomas]

FWIW, check out antitypical/Result#77 for the Result error. Although you're specifying Moya here, I think it might still be a failed lookup between Result and Alamofire's Result type. 🤔

[AndrewSB]

@scottrhoyt: sounds great! Oh wow, I just saw all the work you put into #886 😫 I'm sorry about superseding it!
@BasThomas: thanks for the link, antitypical/Result#77 (comment) was perfect

[AndrewSB]

@scottrhoyt: I think this is ready to go. We just need to deal with the 3 TODOs above, and then add any learnings you have from #886

For #886: We could either have fewer commits redone here, or you can rebase #886 onto this branch and we can merge it here. What do you think?

[scottrhoyt]

No worries @AndrewSB. I actually have all 3 of those sorted in #886 already in addition to the cache improvements.

[AndrewSB]

Oh awesome! Should I close this then and help review your pull request instead?

[scottrhoyt]

Well, I only handled all the concerns on the testing side, I haven't done anything with the rest of the SPM work. So it might be easiest to rebase the rest of your SPM work off of my branch. But another option would be to try and port the other fixes I made to this branch. You started a lot of the SPM work, so I'm good to go either way. I'm just looking forward to faster builds and being able to run tests easier while developing! 👍

[AndrewSB]

That sounds great to me too. Let me give your PR a look over to see what you've implemented so I can try to figure out what we should include from both

[scottrhoyt]

@AndrewSB I finished combining our work. There were really only two areas where we conflicted:

• I kept the Demo targets where they were instead of moving them into the main Moya library project. If we leave it where it is, I don't think anything in the docs needs to change.
• We don't need to use a carthage copy-frameworks to copy the frameworks for test targets. A simple copy files build phase will do.

Let me know if you have a strong opinion on either of those.

All the other commits are just cleaning up, getting CircleCI working, and getting CocoaPods working. I checked again and can't find anywhere in the docs where paths need to be updated--as long as the Demo projects stay where they were. Let me know what you think and thanks for the teamwork! 🤜💥🤛

[codecov-io]

Current coverage is 73.46% (diff: 100%)

Merging #885 into master will not change coverage

@@             master       #885   diff @@
==========================================
  Files            19         19          
  Lines           701        701          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            515        515          
  Misses          186        186          
  Partials          0          0          

Powered by Codecov. Last update 55c4d5d...fadcdca

[MoyaBot]

	2 Warnings
⚠️	Big PR
⚠️	Consider adding supporting documentation to this change. Documentation can be found in the docs directory.

Generated by 🚫 danger

[scottrhoyt]

Fixed all the CI errors except this one from Danger:

Not quite sure what to do about that one.

[BasThomas]

@ashfurrow mentioned that might be a Danger bug.

[scottrhoyt]

Building the Demo project is also our current method of checking that CocoaPods didn't break. So if we combine the projects, we might need a different approach to that.

[AndrewSB]

Hmm... Having one xcodeproj is a strong opinion that I weakly hold. I'm willing to change, but here's what I think

Carthage is driven off of the main Moya.xcodeproj, so I think it's a good idea to keep that as lean as possible.

It is, but Carthage only looks at the shared targets inside of Moya.xcodeproj. As long as we don't share the Demo targets, it won't have any impact on Carthage at all.

Having an example/demo project in separate sub folder and project is in line with CocoaPods best practices and most libraries do it this way, so I think it is more in line with user expectations.

That makes sense for CocoaPods best practice. I wasn't aware of it.
I'm not sure about this, but I think most Carthage-first projects include the Demo within the same xcodeproj, and I think spm is the same way, especially with their generate-xcodeproj command. I'll do some research and get back to you

Merge conflicts are not fun in *.xcodeproj's, so keeping separate projects will allow people to contribute to Demo code and core library code simultaneously with less risk that the Carthage-driving project will get corrupted.

Merge conflicts are a headache, but from my experience they're pretty rare.
Apart from commits like this where we radically change project layout, most changes to xcodeproj's are just adding or removing a file. Which usually don't conflict, or if they do, are trivial to resolve by setting the merge strategy to union.

[AndrewSB]

Your point on validating cocoapods is real though.
Is there any way to validate cocoapods without having a separate xcodeproj?

How important do you think it is for us to run integration tests to make sure we're not broken through our package managers on every master commit?
I've started to feel like it's (at least to some extent) overkill. And its going to be even worse when we're also validating that SPM works on each commit
I'm going to go back and see why we added the Carthage validation on each commit

Update: we added Carthage sanity checking in #630, and the reason why was because we missed #629.

What do you think about testing our package managers in parallel? Either on Travis, or telling Circle to perform our tests in parallel with our spm & cocoapods bootstraps

[ashfurrow]

We can do a pod lib lint to lint it locally (make sure it compiles).

[AndrewSB]

@ashfurrow awesome! So that fixes the validation problem.

@scottrhoyt: RxSwift is the only large project I could find that just has one xcodeproj and includes their Demos & Examples as part of the one project. Some other projects don't include Demos (ReactiveSwift, Result), while Alamofire has a separate xcodeproj. I think RxSwift does an exceptional job with project structure, and that's where my fondness for the one-project-approach comes from

[scottrhoyt]

I could be misunderstanding how sharing of schemes works, but doesn't unsharing a scheme normally put it in .xcuserdata or somewhere else that we normally gitignore?

I would disagree on merge conflicts. I think the majority of the merge conflicts I have solved have been in *.xcodeproj's. It's one of the reasons why bigger tech companies don't use them at all.

pod lib lint is an alternative to validating that the pod spec isn't broken. However, it doesn't test that it is complete and the advantage to what we are doing now is that it also validates that the Demo project continues to build correctly from CocoaPods. This means that we can have more confidence in the CocoaPods install being complete and pod try Moya is working. For example before #869 MultiTarget.swift was missing from Carthage installs because nothing was testing that it was being included via Carthage and the base project could still build fine without it.

So given that what we have is working correctly and has some advantages, I'm just trying to understand the advantages of changing it to be in one merged project. I definitely understand why we needed to get the tests as part of the main project, but the Demo targets don't seem obvious to me.

[AndrewSB]

I could be misunderstanding how sharing of schemes works, but doesn't unsharing a scheme normally put it in .xcuserdata or somewhere else that we normally gitignore?

Sharing schemes is just checking this box in the Scheme manager (Carthage talks about it here). Unshared schemes are still totally visible to users, and runnable. Not gitignored

I would disagree on merge conflicts. I think the majority of the merge conflicts I have solved have been in *.xcodeproj's. It's one of the reasons why bigger tech companies don't use them at all.

Totally valid. I don't have the experience of working at a large company, so I'm probably not qualified to contribute on the severity of merge conflicts. But, regardless of whether we have one or two xcodeprojs, we're still going to get merge conflicts. Having two xcodeproj's might make the conflicts smaller, or space them out to some extent, but it won't get rid of them #xcodeprojmergeconflictsheretostay 😭

For example before before #869 example MultiTarget.swift was missing from Carthage installs because nothing was testing that it was being included via Carthage and the base project could still build fine without it.

Can we write an integration test that verifies that the built targets have all the files we expect it to have? Something that would have caught MultiTarget.swift being orphaned?
If there is, I think we should write it, but I don't see how having one or two projects would affect ⬆️

So given that what we have is working correctly and has some advantages, I'm just trying to understand the advantages of changing it to be in one merged project.

To me there are two benefits

1. Simplicity.
   Even though sharing iOS code is heavy right now (i.e. you cant just throw up a few .swift files up on GitHub, you need to deal with an xcodeproj, and targets, etc.) I like being able to see the entire project at once. For a while, I didn't even realize Moya had a Demo or Tests, since they were hidden away somewhere I don't usually look.
   That was the same motive you had for moving the tests into the main project, so they're easier to access

2. SPM expects & generates only one xcodeproj per package

[ashfurrow]

I don't have anything more to add to the conversation, but wanted to note a) this is a really great discussion, I'm sure other libraries are having similar ones and we should consider writing a blog post about this PR, and b) I'd really like to thank everyone for providing such thoughtful, respectful comments. This kind of conversation makes Moya a joy to work with, and sets a high bar for the rest of the iOS community. Thank you all.

[scottrhoyt]

Thanks to everyone here! I'm honored to be able to talk with such skilled people at such a high level. Even with some butting of heads, I think we all win in the end. This is a model of how open source iOS can be done! ❤️

I'm pretty sure sharing a scheme is more than that check box. I believe it actually moves the scheme from xcuserdata to xcshareddata (as seen here). Usually xcuserdata is completely gitignored. If it is working differently here, then I must be not understanding something. But either way, I think the shared scheme issue isn't a huge deal because right now I think carthage ignores everything except framework targets.

If we move the Demo code into the main Moya project, the Demo code will by default be working off of a carthage integration and not a cocoapods integration, so building it will not have the effect of validating that the Demo project builds via CocoaPods. I think the corresponding changes to configuration and documentation will have the effect of making Moya more of a carthage-first library instead of cocoapods-first. I'm not opposed to that at all, I actually prefer carthage over cocoapods usually (it's how I integrate Moya), but it's a good sized change nonetheless.

I don't think we will be using SPM to generate or build our xcodeproj's any time soon because we need to specifically tailor the xcodeproj to a carthage integration. I don't think that's what you'd get if you let SPM manage it.

Ultimately, despite my defense of the 2 project solution, I'm not stuck in the mud on this one either. And I do love only having one Xcode window open for a project 😁. So, what I'm going to propose is since this currently doesn't change the Demo location from where it is in code and documentation, we merge this as is (after some commit squashing). Then we can create another PR around moving the Demo targets if that still seems like a good idea. In that PR we can debate the relative merits and change our testing scheme to better accommodate the decision. This PR is overweight as is and this might be a good way to contain it a bit. What do you guys say to that?

[AndrewSB]

Likewise, its a pleasure to work with you guys 😄

I have a feeling you might be right about schemes affecting the xcuserdata to xcshareddata, I'm not totally sure what the implementation details of checking the "Shared" checkbox is either.

That sounds good to me, this PR is starting to get bloated. Lets get this merged and talk about it in another PR. I can create one as soon as we merge this into master

[scottrhoyt]

Great! Give me 20 or 30 minutes to squash commits where appropriate and then we can get this bad boy approved and merged. I think people will enjoy the quality of life improvements in here. Thanks for your help @AndrewSB!

[scottrhoyt]

That's a bit more manageable now. Let me know if you think it needs further compacting.

[AndrewSB]

That looks great 😄
I'll get started with the Demo/ PR

EDIT: created #891