search

erikdoe / ocmock

Mock objects for Objective-C
http://ocmock.org
Apache License 2.0
2.16k stars 606 forks source link

Swift pm support #402

Closed mayur1407 closed 4 years ago

mayur1407 commented 4 years ago

Disabled test cases in Package file due to hamrest dependency. Maintain consistency in the imports.

erikdoe commented 4 years ago

What is the relationship between this and #379?

mayur1407 commented 4 years ago

What is the relationship between this and #379?

  1. This pull request has the resolved merge conflict.
  2. Imports consistency is maintained.
  3. hamrest is not supporting SwifTPM, hence, had to disable it from the SwiftPM test target. However, it won't affect the existing XCode project environment.
erikdoe commented 4 years ago

So, if this PR completely supersedes #379 can you close #379? And, echoing my last comment over there, it seems that any of this is blocked until Swift Package Manager supports libraries that don't use ARC, right?

mayur1407 commented 4 years ago

@mayur1407 I hear what you say, but do you agree that this PR is blocked until Swift Package Manager can support libraries that do not use ARC?

@erikdoe: Whenever non-ARC code is used in the ARC project, We have been using the -fno-objc-arc flag. The Compiler needs to know whether memory needs to be handled manually or automatically. Hence, with due respect, I disagree with the waiting time as we don't know the improvement plan of SwiftPM but SwiftPM is definitely a future to maintain an efficient dependency manager in Swift code.

erikdoe commented 4 years ago

It feels like we're talking past each other. I am aware that the compiler needs to know whether to build with or without ARC.

However, my current understanding is that the only way to get a library built without ARC in Swift Package Manager is to pass the flag you refer to as an unsafe flag. And the comments by @karimhm seem to suggest that if you do that, the library can't be used:

By using cSettings: [.unsafeFlags(["-fno-objc-arc"])] this package will be un-importable by other packages.

and

There is no alternative at the moment

So, would you agree that Swift Package Manager does not support libraries that don't use ARC?

If this is true, i.e. Swift Package manager not supporting libraries that don't use ARC, then this implies that you can't use OCMock with Swift Package Manager, right? And if that's the case, why add support now?

paulb777 commented 4 years ago

I made a few additional tweaks for swift-tools-version:5.3 to this PR in https://github.com/paulb777/ocmock/commits/paulb777-spm and so far its working well enough to run the first Firebase test suite I've tried with it in https://github.com/firebase/firebase-ios-sdk/pull/5959.

I didn't run into any problems because of the ARC requirement so far. Why is disabling ARC needed for OCMock?

mayur1407 commented 4 years ago

I made a few additional tweaks for swift-tools-version:5.3 to this PR in https://github.com/paulb777/ocmock/commits/paulb777-spm and so far its working well enough to run the first Firebase test suite I've tried with it in firebase/firebase-ios-sdk#5959.

I didn't run into any problems because of the ARC requirement so far. Why is disabling ARC needed for OCMock?

Memory management is manually handled in the OCMock. Hence, SwiftPM was complaining about it. It is great to hear from you that its working fine after your changes. :) Awaiting for this support to integrate OCMock through this approach. (finger crossed)

erikdoe commented 4 years ago

@paulb777, can you clearify what the tweaks are? As far as I can tell you removed the en.lproj directory. How did that help? I thought the issue was that -fno-objc-arc is required to build OCMock and Swift Package Manager doesn't accept this. Has anything changed?

As it stands this PR adds the build file for the package manager and changes some imports. By the way, I'm still not sure why some imports are in angle brackets and others in straight quotes. I've asked this several times, but as far as I see it I got an explanation what the different styles are for, but I'm after an explanation of why the different styles are used.

erikdoe commented 4 years ago

PS: I didn't state this anywhere, but I'm not even considering to merge a PR with commented out code.

paulb777 commented 4 years ago

@erikdoe SwiftPM generates more limited include search paths in its generated header search paths than CocoaPods which requires changing import syntax. I deleted the en.lproj directory to work around a build failure, because SwiftPM wants every file in its Source tree to be described. There may be a better option with a SwiftPM exclude: directive.

I'm planning to go a bit further in Firebase prototyping and if my OCMock fork continues to work and no one beats me to it, I'll make a clean PR - hopefully sometime next week.

erikdoe commented 4 years ago

I had another look at this PR. When I check it out, open the project in Xcode, and choose Product/Test (Cmd-U), the test target fails to build. Does this work for you? Have you made any other changes?

karimhm commented 4 years ago

I believe that the only way to have this library as a fully functional Swift Package is through the binary distribution newly introduced with Xcode 12

paulb777 commented 4 years ago

To reproduce the current test suite success I have with OCMock and Swift Package Manager:

Related discussion at https://forums.swift.org/t/swiftpm-and-library-unit-testing/26255/14

mayur1407 commented 4 years ago

@erikdoe SwiftPM generates more limited include search paths in its generated header search paths than CocoaPods which requires changing import syntax. I deleted the en.lproj directory to work around a build failure, because SwiftPM wants every file in its Source tree to be described. There may be a better option with a SwiftPM exclude: directive.

I'm planning to go a bit further in Firebase prototyping and if my OCMock fork continues to work and no one beats me to it, I'll make a clean PR - hopefully sometime next week.

Just to inform all, I see that the updated SwiftPM supports en.lproj inside the source folder. SwiftPM is supporting resources inside the package.

erikdoe commented 4 years ago

@paulb777, thank you for confirming that the Firebase build is working with your changes / the changes proposed here. To be honest, though, what I am after is information on whether OCMock itself still builds for you. As I mentioned in my previous comment, when I check out this PR in its current state, the unit tests in OCMock itself do not build anymore; it's not that the tests fail, they don't even compile anymore.

paulb777 commented 4 years ago

Following up in #442

erikdoe commented 4 years ago

Closing this in because we're making progress in #442.