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

Swift Package Manager #442

Closed
wants to merge 3 commits into from
Closed

Conversation

paulb777
Copy link
Contributor

@paulb777 paulb777 commented Jul 8, 2020

Successor to #402

  • Updates to swift-tools-version:5.3
  • Removes Source/OCMock/en.lproj/InfoPlist.strings which causes trouble for SPM and doesn't seem to be otherwise used
  • Makes public headers findable from each other by using unqualified file name imports
  • Sets up a Public folder for SPM public headers, since SPM cannot choose headers out of a larger directory.

The Public folder symbolic links to the public headers. Making a copy would be an alternative implementation that would allow other package manager implementations to be unchanged, but this seems to work and is easier to maintain.

Ideally all implementation would migrate to using a separate Public header folder implementation at some point.

Testing:

Fix #375

@mRs-
Copy link

mRs- commented Aug 19, 2020

any news on this? would like to use OCMock with Swift Package Manager as soon as possible 😀

@paulb777
Copy link
Contributor Author

While the PR is pending, Firebase is using it like https://github.com/firebase/firebase-ios-sdk/blob/master/Package.swift#L123

@mRs-
Copy link

mRs- commented Aug 19, 2020

Good point, will give it a try!

@erikdoe
Copy link
Owner

erikdoe commented Aug 19, 2020

To be honest, after the other two PR's about Swift Package Manager got really confusing, and I didn't have time to learn about Swift Package Manager, I mentally parked this one.

Having looked at it now, I must say that I'm not on board with the approach taken. If SPM really needs all public headers on their own in a folder, then this sounds like part of the build process. It feels completely wrong (to me) to create a folder full of symlinks and check that in.

Also, are you sure about the fact that Swift Package Manager needs the headers on their own in a folder. I don't recall the other two PRs mentioning this.

@paulb777
Copy link
Contributor Author

Swift Package Manager can only specify public headers by folder and the previous PRs ended up treating non-public headers as public which caused a few failures in my testing.

From an SPM perspective, it would be more ideal for all build systems to specify the public headers from a single folder, but this change is less intrusive to the existing builds.

@erikdoe
Copy link
Owner

erikdoe commented Aug 19, 2020

Thanks for the quick reply. As mentioned, I would like to explore whether this folder for public headers cannot be generated automatically at build time, using a script or something. It feels wrong to me to create this artefact, which is only needed for SPM, and check it in.

Also, I now see that your are specifying .unsafeFlags(["-fno-objc-arc"]) . If I remember correctly from the previous discussions any package that uses these becomes "un-importable" (see #379 (comment)).

@paulb777
Copy link
Contributor Author

paulb777 commented Aug 19, 2020

I don't think that SPM has any kind of script capability and it is very opinionated about a 1-1 relationship between the git contents and what gets built - so I don't have any ideas about alternatives other than a larger file structure refactor like we did in Firebase to support building with both SPM and CocoaPods.

I didn't touch the .unsafeFlags(["-fno-objc-arc"]) flag from the previous PRs but it seems to work fine for test targets. I'll investigate removing it ...

@paulb777
Copy link
Contributor Author

.unsafeFlags(["-fno-objc-arc"]) is definitely needed. swift build fails with several ARC errors without it.

SPM must have different rules about unsafeFlags for test targets.

@dmaclach
Copy link
Contributor

dmaclach commented Aug 19, 2020 via email

@erikdoe
Copy link
Owner

erikdoe commented Aug 20, 2020

So, I started down the rabbit hole... It really seems like SwiftPM does not allow to run scripts. At the same time I am unwilling to accept a directory full of symlinks just for this.

And maybe it's just me but I can't seem to find any reasonable documentation by either Apple or on swift.org that explains how to create a Swift package for an Objective-C framework. Could anyone please provide a link to such documentation? I'm not going to piece this together from Stack Overflow answers and the source code of SwiftPM.

@paulb777
Copy link
Contributor Author

From https://forums.swift.org/t/picking-and-choosing-public-headers/39558, a custom module map for Swift Package Manager would be an alternative. Would that work?

@erikdoe
Copy link
Owner

erikdoe commented Aug 22, 2020

@paulb777, thank you for looking into this option. Reading through the documentation it seems this could work:

In case of complicated include layouts or headers that are not compatible with modules, a custom module.modulemap can be provided in the include directory.

Now, I know nothing about modulemaps...

By the way, in the meantime I made the non-controversial changes on the "master" branch. The imports now use straight quotes and the localised plist string files are gone. Thanks for pointing these out; good changes anyway. And I would have cherry-picked the commits to give credit that way, but the commits were intertwined with changes to the package manager file.

@karimhm
Copy link

karimhm commented Aug 22, 2020

If any one is interested module maps are extensively documented here.

@karimhm
Copy link

karimhm commented Aug 22, 2020

Did any one investigated the possibility to distribute OCMock as a Swift Package Manager Binary Dependency rather than via source code?.

It is possible to create a Github Actions pipeline that packages OCMock as a Binary Dependencies.

@karimhm
Copy link

karimhm commented Aug 22, 2020

Another interesting thing: Uploading Artifacts on Travis CI.

@paulb777
Copy link
Contributor Author

@erikdoe Thanks for the follow-up and updates. I'll see if I can put together a module map alternative to this PR.

@karimhm Thanks for sharing the links. I'm not sure a binary distribution provides any extra value to justify its extra maintenance and usability complexity.

@karimhm
Copy link

karimhm commented Aug 22, 2020

@karimhm Thanks for sharing the links. I'm not sure a binary distribution provides any extra value to justify its extra maintenance and usability complexity.

With binary distribution it would be possible to use OCMock as a dependency from any target. I believe there is some other libraries that depend on this one, one example would be Cuckoo.

@paulb777
Copy link
Contributor Author

See #460 for a custom module map implementation

@twitterkb
Copy link

just starting to catch up on the changes here w.r.t. SPM.

would like to see some solution that will allow us to start using OCMock as an SPM package …

i don't know which of the alternatives here is preferred at this point.

looking for a tl;dr on where the discussion on this is at and if there's a plan going forward.

@twitterkb
Copy link

i think i prefer the other patch with the Package.swift changes to this one … but would like to see a canonical Package.swift file adopted on master here …

@erikdoe
Copy link
Owner

erikdoe commented May 5, 2021

Closing this as it looks likely that the solution in #460 is the way to go.

@erikdoe erikdoe closed this May 5, 2021
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.

Add Swift Package Manager support
6 participants