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

Use CocoPods instead of git-submodules for linking with AFNetworking in Unit Tests #90

Merged

Conversation

corinnekrych
Copy link
Contributor

@AliSoftware this is a PR for #86

  • I had to change target built to 6.0/10.8 due to AFNetworking 2.50 (the version used in gh submodule) supporting only from this level.
  • I used 2 workspaces:
  • the actual project OHHTTStubs (i put the Podfile in root) with test pod dependency
  • and Demo with OHHTTStubs as pod
    wdyt?

@@ -13,3 +13,8 @@ profile
*.moved-aside
.DS_Store
*.xccheckout
# CocoaPods
Podfile.lock
Copy link
Owner

Choose a reason for hiding this comment

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

You must never gitignore Podfile.lock.

Podfile.lock is the manifest that tell with versions of the pods have been installed the last time you pod install. Keeping it ensure that anyone making a pod install on their own machine will get the same version of pods that the ones you used the last time you checked your code.

This is important because if for some reason a third-party lib update (even a patch) breaks something, the code that works now will suddenly stops working later even if there were no new commit in OHHTTPStubs… just because one dependency did publish a breaking update, and as you didn't commit the Podfile.lock so you didn't lock your dependencies.

That's the way every dependency manager (CocoaPods, Carthage, etc) work and Podfile.lock files are meant to be committed onto GIT, otherwise there would be no guarantee that running pod install one day on one machine will install the same stuff that running pod install days later or on another machine. (If you want to update your dependencies later, that's what pod outdated and pod update XXX and pod update are for anyway)

@AliSoftware
Copy link
Owner

Travis Failed. Seems to be because you introduced the need for running pod install which means that it takes more time (network requests) to build. I would prefer to go with the policy of:

  • Pushing the pods in the repo, so that anyone can build the library and/or run the tests and/or run the demo even if they haven't CocoaPods installed (turnkey solution). This includes people using Carthage.
  • Using Development Pods for the library itself (like :path => ../) so that
    • when we need to work the OHHTTPStubs's code itself to prepare new features and fix issues, we take advantage of this Development Pods mode/feature
    • it avoids having to run pod install during development to test out the changes in the demo
    • it avoids the risk to modify the code in the wrong place (typically modifying the source from the demo project — that would be erased on the next pod install — instead of in the lib project itself)
    • and all those other stuffs that gave the "Development Pods" feature its name. It's made for this kind of use case so we should really do it (that's what I do on all my projects because that's so useful, so I plan to do it for OHHTTPStubs too anyway)

@AliSoftware AliSoftware changed the title 86 cocopods insteadof ghsubmodules Use CocoPods instead of git-submodules for linking with AFNetworking in Unit Tests Mar 4, 2015
@corinnekrych
Copy link
Contributor Author

Hi @AliSoftware

To Pods/Podfile.lock or not to Pod/Podfile.lock, that is a question...
of preferences I think.
Having fix version of Podfile is important though. Having only Podfile with fixed version, required cocoapods and specify a cocoapods version as pre-requisite, but leave you repo clean of generated folder/file. From a Java background(I confess) having to run mvn/pod install does not sound bad.

BUT, I've added them as you wished. Question: should i also add OHHTTPStubs.xcworkspace if you don't want to have a pre-requisite on pod install ?

Yes we need to discuss the minimal deployment target.
I changed it due to AFNetworking supporting only 6.0/10.8. Are you ok to move a level up?
then yes we should change: https://github.com/AliSoftware/OHHTTPStubs/blob/master/OHHTTPStubs.podspec#L39-L40

I'll take a closer look to Travis build shortly.

@corinnekrych
Copy link
Contributor Author

@AliSoftware because i'm not familiar with, just reading http://www.cocoanetics.com/2014/07/development-pods/
you modifly the Demo podfile, what shoul it be?

@AliSoftware
Copy link
Owner

To Pods/Podfile.lock or not to Pod/Podfile.lock, that is a question... of preferences I think.
Having fix version of Podfile is important though. Having only Podfile with fixed version, required cocoapods and specify a cocoapods version as pre-requisite, but leave you repo clean of generated folder/file. From a Java background(I confess) having to run mvn/pod install does not sound bad.

To commit the Pods/ folder or not is entirely debatable and a matter of preferences, I totally agree, as explained above already.

But regarding Podfile.lock, it MUST ALWAYS be pushed on the repo. There is no debate possible here: if you don't commit it, it will break the way pod install work. Not committing Podfile.lock will make pod install always do as it it was the first time you installed the pods, or as if you always did pod update (which bumps the versions of the pods used), which is not what pod install is intended to do.

The Podfile.lock file is at the heart of how CocoaPods works. It is what locks the versions of the pods to a specific version. Whether you choose to commit the Pods/ folder or not depending on your preferences does not change the fact that the Podfile.lock must always be commited no matter what.

Specifying explicit versions in the Podfile (like 3.1.10 here) is NOT sufficient to lock the versions of the pods used, because this does not lock the dependencies the pods rely on, for example, so this does not guarantee that the versions of the dependencies retrieved upon pod install will be the same when you pod install now or 3 month from now. Only Podfile.lock can guarantee that.

Please read the guides again that explain that in details: http://guides.cocoapods.org/using/using-cocoapods.html

@AliSoftware
Copy link
Owner

you modifly the Demo podfile, what shoul it be?

Just modify the Podfile of the demo project to use :path => 'relative/path/to/folder/where/podspec/is' as explained here and run pod install once. And that's it.

You can then commit the Pods/ folder because it only contains symbolic links to the real sources, (that are store locally in the same repo, as the sources of OHHTTPStubs itself are next to its demo project), not copies of those files; so no need to run pod install again each time you change the pod sources (in contrast to the current solution you use that requires us to run pod install on the Demo project each time the library source code changes) because it directly links to the original files.
That is particularly useful for demo projects that live alongside the library's source, like we have here, because you can directly modify the source code of the lib from the demo project when you work on the lib and you don't have to pod install each time to test your lib locally.

And it allows Travis to work without the need to contact other servers, and it allows pod try to work and download and launch the Demo project for your pod that will work out of the box. And so much more.

@AliSoftware
Copy link
Owner

See also my detailed answer and explanation in this GitHub comment regarding the intended usage of pod install vs pod outdated vs pod update and the importance of Podfile.lock.

@corinnekrych
Copy link
Contributor Author

@AliSoftware

  • I did the change for demo podfile
  • Podfile.lock checked in. Let's no start a flame war with bold and uppercase. My view is more: when using fixed version in your Podfile and given you dependencies use fixed versions, you end up with Pods with fixed version too.

but we still have to sort out:

  • a failing travis for iOS7, running out of idea for which simulator to pick... but the other subjobs succeed. (i'll do some more testing on a sep. branch)
  • supported platform, with this commit 892d8e4 I've updated versions supported and move to 4.0.0. Are you ok with that?

If this is not the way you want to go forward, close my PR.

@AliSoftware
Copy link
Owner

My view is more: when using fixed version in your Podfile and given you dependencies use fixed versions, you end up with Pods with fixed version too.

Sorry to insist, but that's still not true.
Believe me, I worked on CocoaPods's resolver (called Molinillo, and which is now also integrated in bundler too) with the team and am part of CocoaPods CoreTeam where we discussed such matters regarding the resolver, and there are a lot of use cases where using a fixed version in the Podfile is not sufficient to ensure pods (especially dependencies) with fixed versions… and that's exactly why Podfile.lock exists. (see also our blog post about the CP resolver)

Anyway, the Podfile.lock file is generated for that exact purpose and CocoaPods expects it to be pushed, that's quite even its raison d'être and that's how CocoaPods work (and a lot of other dependency managers have lock files too and work the same way, including bundler, gem, etc). If this file is not present, the tool cannot guarantee that the version locking feature will work as expected.
That's not some personal choice or whatever, that's just a necessity for CP to work properly in all situation (even when using fixed versions in your Podfile) as explained in the guides; that's not me having personal preferences, that's just how the tool is expected to work at all.


Regarding other aspects, I'm ok with bumping the minimum version.
Anyway, thanks again for your PR; I'll look into the Travis failure for iOS7 as soon has I finish my other WIP in order to review and merge that PR ASAP.

@AliSoftware
Copy link
Owner

Regarding Travis, we will have to test it but I bet that even if xcrun simctl lists simulators configured with 7.0 runtime, I bet that the Xcode installed on travis VM only have the 7.1 runtime (like my local Xcode 6 do, as usually the Downloads tab to get older simulators/runtimes only propose 7.1 and not 7.0)

So I my bet is right, changing the target to 7.1 instead of 7.0 in the .travis.yml should solve the problem.

@corinnekrych
Copy link
Contributor Author

@AliSoftware Still the same with 7.1
Actually i did several trials:

  • remane iPhone 5S by iPhone5s
  • change to iPhone 5
  • 7.0
    without luck so far....

@AliSoftware
Copy link
Owner

Mmmhh. Really strange. Will look into that ASAP. Thanks for all your tests!

@AliSoftware
Copy link
Owner

Bad news. Just ran xcrun simctl list on Travis and it appears that iOS 7 runtime is not installed at all on Travis :-/

xcrun simctl list
== Device Types ==
iPhone 4s (com.apple.CoreSimulator.SimDeviceType.iPhone-4s)
iPhone 5 (com.apple.CoreSimulator.SimDeviceType.iPhone-5)
iPhone 5s (com.apple.CoreSimulator.SimDeviceType.iPhone-5s)
iPhone 6 Plus (com.apple.CoreSimulator.SimDeviceType.iPhone-6-Plus)
iPhone 6 (com.apple.CoreSimulator.SimDeviceType.iPhone-6)
iPad 2 (com.apple.CoreSimulator.SimDeviceType.iPad-2)
iPad Retina (com.apple.CoreSimulator.SimDeviceType.iPad-Retina)
iPad Air (com.apple.CoreSimulator.SimDeviceType.iPad-Air)
Resizable iPhone (com.apple.CoreSimulator.SimDeviceType.Resizable-iPhone)
Resizable iPad (com.apple.CoreSimulator.SimDeviceType.Resizable-iPad)
== Runtimes ==
iOS 7.0 (7.0 - Unknown) (com.apple.CoreSimulator.SimRuntime.iOS-7-0) (unavailable, runtime path not found)
iOS 7.1 (7.1 - Unknown) (com.apple.CoreSimulator.SimRuntime.iOS-7-1) (unavailable, runtime path not found)
iOS 8.1 (8.1 - 12B411) (com.apple.CoreSimulator.SimRuntime.iOS-8-1)
== Devices ==
-- iOS 7.0 --
    iPhone 4s (16E214BA-9A36-489E-81C5-9CCB9604FF8B) (Shutdown) (unavailable)
    iPhone 5 (B3581D0A-B151-4F5E-9884-FC5A78CDD908) (Shutdown) (unavailable)
    iPhone 5s (CC4EE950-AC3A-40B9-A360-AD393B8D3D08) (Shutdown) (unavailable)
    iPad 2 (FFDC0F02-6F2D-4261-A188-36C04B845F57) (Shutdown) (unavailable)
    iPad Retina (62A308F2-3272-4EEA-9CD1-90C2335FD3E8) (Shutdown) (unavailable)
    iPad Air (0A5D20B3-5D34-46F5-99B3-EB4AEA113421) (Shutdown) (unavailable)
-- iOS 7.1 --
    iPhone 4s (DF1BBEDE-90C5-41D9-A11B-8DA659322101) (Shutdown) (unavailable)
    iPhone 5s (9262546F-329F-464B-9706-CD956CE4EC88) (Shutdown) (unavailable)
    iPad 2 (27281F5C-9697-4009-9932-722486AFBBD4) (Shutdown) (unavailable)
    iPad Retina (215C68E1-4BE6-4169-B30D-1847CF744474) (Shutdown) (unavailable)
    iPad Air (88AAFE0A-8784-4CDF-997A-4387E38CF85A) (Shutdown) (unavailable)
-- iOS 8.1 --
    iPhone 4s (37BC3125-3F35-4B1B-9195-BC5105B115AF) (Shutdown)
    iPhone 5 (2DDCAE77-D0F8-4FB1-B9E0-3AFBECA68164) (Shutdown)
    iPhone 5s (604674C2-08C7-4F3E-9910-BB7731045B9A) (Shutdown)
    iPhone 6 Plus (0569A245-BFFC-4F9D-96BD-BF4D5E28F738) (Shutdown)
    iPhone 6 (28727EC6-4665-4FC2-B77C-C2C456206044) (Shutdown)
    iPad 2 (32D0747C-DAF9-4EA6-BC66-28EAE7CA357A) (Shutdown)
    iPad Retina (78CCB429-8012-4AB9-BB00-48C945BA1722) (Shutdown)
    iPad Air (33309D21-9B76-41F2-B16E-8F9B4DB269AE) (Shutdown)
    Resizable iPhone (90F9F633-C4A2-499C-82C0-0E2CD82A1829) (Shutdown)
    Resizable iPad (115F4F5D-C861-4342-A274-D8438126F158) (Shutdown)

@AliSoftware AliSoftware merged commit 3b50a70 into AliSoftware:master Mar 12, 2015
AliSoftware added a commit that referenced this pull request Mar 12, 2015
@AliSoftware
Copy link
Owner

This is now fixed and merged.

Regarding Travis-CI, I confirmed this is because Travis does not actually has the iOS7 Runtime installed. I emailed their support to ask for it, and disabled it in the meantime.

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