SPM with custom module map #460
Conversation
|
Maybe I misunderstand something, but weren't the modulemaps an approach to avoid the directory full of symlinks? Also, OCMock uses Travis and not Githib Actions as CI system. In any case, given that the test is adding yet more complexity, I am happy not to have a test for the SPM file. There are no tests for CocoaPods and Carthage either. |
|
One reason I added the tests is to make it clear the advantage of the extra directories. The extra directories make it easier for existing OCMock clients to migrate, since they can switch from CocoaPods to SPM without source changes. If there's only a module map, all clients will need to change their code to use the module import So three questions:
If the answer to all three is yes, the PR will be reduced to two files - the |
|
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. |
|
As I have commented previously in various PRs, I'm not happy about adding a directory full of symlinks just for Swift PM. This PR has the additional issue that it introduces a new CI system (Github actions in addition to Travis). We are making some progress and the questions asked by @paulb777 help. I would say that the benefit of easing migration from CocoaPods or Carthage to Swift PM is not worth the directory full of symlinks. Unless I'm missing something, that's a very niche use case. I assume that most clients that still use OCMock are mature Objective-C codebases grown over the years. What would be the reason for them to switch? Also, if a team makes such a major change then changing from I've seen the PR you opened, @twitterkb, and I've seen your comment, @paulb777. If the straight quotes aren't needed why did people seem to tell me that they are? Also, can someone please look at how modulemaps work in detail and come up with a proposal for Swift PM that requires neither straight quotes nor directories with symlinks? In the meantime I'm going to look at the PRs flagged for 3.8 and some of the other bugfixes. It feels like we're going round in circles on this Swift PM topic. And it seems like it's a lot of work for a niche use case for which a workaround seems to exist, right? |
in our codebase, we do indeed have a large and mature Objective-C codebase grown over 10 years … and we have committed (internally) to switching to use SPM for all code by WWDC 21 . we feel that in terms of dependency management that stays close to the Apple process, and w.r.t. inter-operability between Objective-C and Swift, Swift Package Manager based solutions are the way to go.
i don't know why you were told "straight quotes are the way to go". the pull-request i submitted is based on Apple's recommendation in the Issue Navigator when you open an existing project in Xcode12 for the first time … and this will allow us to replace a ruby script that we have used to enforce this behavior in all of our code (with OCMock being in an exclude-list from the ruby script). our solution for exposing the headers via SPM is a module.modulemap . i would have to investigate exactly what that might be for OCMock . (the other pull-request i submitted is really somewhat independent of Swift Package Manager … it is related to being able to import OCMock in any kind of downstream module in a way that fulfills the modular header contract.)
use of Swift PM is definitely not niche in our case. it will be the focus of work by one of our teams for the 9 months or so following Apple's fall events. |
|
I just push an update that removes the extra headers. It might be a good compromise. With this module map, it's still possible for clients to do non-module imports with I still want to do more testing on our source base. |
| @@ -0,0 +1,29 @@ | |||
| module OCMock { | |||
There was a problem hiding this comment.
in my experience, in many cases making this framework module OCMock is the canonical way of describing this to allow framework header or modular import. (though that is not what we did in the example i describe below of a legacy bit of code that we added module.modulemap support to …)
There was a problem hiding this comment.
With framework module OCMock, I see:
$ swift test
/Users/paulbeusterien/gh/ocmock/Source/module.modulemap:2:10: error: header 'OCMock/OCMock.h' not found
header "OCMock/OCMock.h"
^
/Users/paulbeusterien/gh/ocmock/SwiftPMTests/objc/objc-module.m:1:9: fatal error: could not build module 'OCMock'
@import OCMock;
~~~~~~~^~~~~~
2 errors generated.
There was a problem hiding this comment.
in that case, the header should possibly simply be
header "OCMock.h"
… however, it may also be the case that if you follow the guide of the other example i provided, framework may not be necessary.
There was a problem hiding this comment.
Changing to header "OCMock.h" leads to fatal error: 'OCMock.h' file not found
It seems functional to add the wrapper scope like in the example, but I don't see what value it provides. I don't see any different functionality.
There was a problem hiding this comment.
the wrapper scope in the example allows for submodule import, should that be desired.
we do perform submodule import of items in the example. when performing @import OCMock in our current code, we cannot perform submodule import, so have never done so. i don't think it's necessary, since nothing is really relying on it, and since for most use cases, it is test code, and worrying about eyeball complexity based on which submodules you are importing probably isn't as important. i can live without the submodule wrapper separation.
i am surprised that the header "OCMock/OCMock.h" syntax is fully required. that has not been something we have encountered in creating module.modulemap files.
if this works for the OCMock tests and works in modules you ingesting OCMock in, this feels like an appropriate start.
There was a problem hiding this comment.
As I mentioned before I don't know Swift Package Manager. This exchange leaves me puzzled. Should it be framework, module, and/or header? Have you reached consensus?
Also, please bear in mind that I do not want to change the framework header. So, anything that requires double quotes or similar in OCMock's public header files is a non-starter.
Source/module.modulemap
Outdated
| @@ -0,0 +1,29 @@ | |||
| module OCMock { | |||
| umbrella header "OCMock/OCMock.h" | |||
| exclude header "OCMock/NSInvocation+OCMAdditions.h" | |||
There was a problem hiding this comment.
rather than having to include every existing .h as well as having to change this modulemap (and thus, technically, a small aspect of the API) every time the internal structure of OCMock changes in a way that causes the addition or deletion of headers, i would make this module.modulemap simply refer specifically to each header to be included.
that way, only when public headers get added or removed will this file also change … and the compiler will pretty quickly enforce such changes in the first thing that attempts to import the module.
possibly reference:
There was a problem hiding this comment.
Commit coming that replaces the umbrella and excludes with a list of the public headers. Thanks!
| @@ -0,0 +1,29 @@ | |||
| module OCMock { | |||
There was a problem hiding this comment.
the wrapper scope in the example allows for submodule import, should that be desired.
we do perform submodule import of items in the example. when performing @import OCMock in our current code, we cannot perform submodule import, so have never done so. i don't think it's necessary, since nothing is really relying on it, and since for most use cases, it is test code, and worrying about eyeball complexity based on which submodules you are importing probably isn't as important. i can live without the submodule wrapper separation.
i am surprised that the header "OCMock/OCMock.h" syntax is fully required. that has not been something we have encountered in creating module.modulemap files.
if this works for the OCMock tests and works in modules you ingesting OCMock in, this feels like an appropriate start.
|
Successful test on the Firebase repo at firebase/firebase-ios-sdk#6326 |
|
Can we merge this please? |
|
Here's an update on the SwiftPM allows Thus, apps and libraries can still use OCMock for their internal testing, while usage of the libraries without the tests can still be versioned. Example at https://github.com/firebase/firebase-ios-sdk/pull/6860/files |
|
Nice work here! Is there anything else left that is preventing this from merging this? We would like to consume OCMock via SPM as soon as possible. |
|
Firebase is using a fork https://github.com/firebase/firebase-ios-sdk/blob/master/Package.swift#L142. We won't promise to support this in the long term, but no plans to stop in the near term. |
.github/workflows/spm.yml
Outdated
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - name: Xcode 12 | ||
| run: sudo xcode-select -s /Applications/Xcode_12_beta.app/Contents/Developer |
There was a problem hiding this comment.
@paulb777 , i don't know if adding spm.yml testing really is a complete non-starter. the only part that i thought was "unnecessary" was changing xcode-select to use the -beta .
fwiw, we are at the place where we need to move to SPM for OCMock for some changes we are making this week, and may be interested in going with your fork as a starting point, especially if it just works out of the box for us.
There was a problem hiding this comment.
As I mentioned early on in this discussion (and repeated earlier today), OCMock uses Travis CI for continuous integration. Please do not add another CI system.
.github/workflows/spm.yml
Outdated
| name: spm | ||
|
|
||
| on: | ||
| push: | ||
| pull_request: | ||
|
|
||
| jobs: | ||
| swift-build-run: | ||
| runs-on: macOS-latest | ||
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - name: Build | ||
| run: swift build | ||
| - name: Run | ||
| run: swift test |
There was a problem hiding this comment.
As I mentioned before OCMock uses Travis CI. Please do not add another CI system.
There was a problem hiding this comment.
Does this PR become mergeable by moving the CI to Travis or are additional changes needed?
There was a problem hiding this comment.
As I wrote last year, I'm okay with not having any tests for this. So, there isn't even a need to move this to Travis.
Regarding the status of the PR, there's still the open question around what should go into the .modulemap file. The discussion seemed to have petered out, which is why I asked for clarification earlier today.
There was a problem hiding this comment.
If it can be merged, my preference would be to add the tests, to make it easier to maintain going forward. I'm fine to do that migration if you're ok with SPM tests in .travis.yml. The GHA tests run in a minute or two, so it should be minimal impact.
On the .modulemap discussion, since the current state has worked for us and others using the fork, my recommendation would be to keep it as is and address any shortfalls in another PR.
There was a problem hiding this comment.
Okay, that means the .modulemap is good to go. If you want to add a test on CI, please don't add it directly to the Travis file. That only invokes make ci. So, the actual test should go into the makefile.
|
I rebased, did a small update, migrated CI to travis, and tested with Firebase in firebase/firebase-ios-sdk#8042 |
Alternative to #442. Fix #375
Add Swift Package Manager support via a custom module map
Adds tests to verify import syntax
Add a GitHub Action workflow to run the tests on PRs and pushes
Adds headers to the module map to support existing client usage. See SwiftPMTests/objc/objc-header.m.
I couldn't find another way to support client usage other than module syntax without adding the extra headers.
Without them would require source changes for existing clients as well as being problematic for testing libraries (versus frameworks) and Objective C++ that doesn't support module imports.
Any suggestions about a better way?