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

Skip building testing libraries for SPM users. #758

Closed
wants to merge 3 commits into from
Closed

Skip building testing libraries for SPM users. #758

wants to merge 3 commits into from

Conversation

sunshinejr
Copy link

@sunshinejr sunshinejr commented Sep 19, 2019

Hey all! In this PR I'm removing testing libraries for SPM users. This is because currently SPM will both fetch & build your libraries even though they are in a target that you don't use. Because of that (and the current integration of SPM in Xcode), Xcode will build your testing dependencies (either in Xcode Preview or for normal builds) and complain (in Preview you won't ever get your UI to show and in your projects you will get runtime crash for missing XCTest etc.).

With this fix we are still able to test ReactiveSwift (using additional env variable) and skip downloading the testing libraries for normal usage.

Let me know what you think about it.

Checklist

  • Updated CHANGELOG.md.

@sunshinejr
Copy link
Author

Hmm seems like only one test didn't pass on one environment (exceeding 1 sec) - is this something it was there before?

@kaybutter
Copy link

A side effect introduced by this change is that you can no longer open the Package.swift in Xcode 11 and run the tests from there. 🤔

@ikesyo
Copy link
Member

ikesyo commented Sep 20, 2019

This is because currently SPM will both fetch & build your libraries even though they are in a target that you don't use.

I assume (or believe) Quick and Nimble are only built when running swift test since they are dependencies of ReactiveSwiftTests .testTarget and not of ReactiveSwift .target.

@sunshinejr
Copy link
Author

sunshinejr commented Sep 20, 2019

@ikesyo yeah that's what I assumed as well but you can try it yourself by creating a new Xcode 11 project with SwiftUI & add ReactiveSwift as an SPM package and test. First, you will see all the dependencies in the project navigator (e.g. Quick, Nimble and their dependencies - maybe for resolution). But then you will see that Xcode Previews will try to actually build it and fail - so once you import ReactiveSwift into your project then you won't be able to use Xcode Previews.

Additionally I saw reports (and had it happened to me to) that it will actually crash on runtime with a XCTest linking problem in the main target.

Of course you could ignore it and rely on Xcode/SPM fixing it but I think this is worth looking into this PR as a short-term solution until its fixed. The drawback is that you will need to do one additional step in the development process.

@ikesyo
Copy link
Member

ikesyo commented Sep 27, 2019

I just tried with Xcode 11 and only ReactiveSwift was built as a dependency as expected.

image

@sunshinejr
Copy link
Author

@ikesyo I was talking about Xcode Previews, these are available when you open a canvas (option+cmd+enter) in a SwiftUI-compatible view:

struct Test: View {
    var body: some View {
        Text("test")
    }
}

#if DEBUG
struct Test_Previews : PreviewProvider {
    static var previews: some View {
        Group {
            Test()
        }
    }
}
#endif

Zrzut ekranu 2019-09-27 o 10 47 33
(you can also use project from the FB feedback-assistant/reports#42)

@ikesyo
Copy link
Member

ikesyo commented Sep 27, 2019

I don't have Catalina environment yet so I can't examine it locally.

@sunshinejr
Copy link
Author

@ikesyo alright, no worries. We are maintaining our own fork for that matter at Moya for now if anyone is interested as well.

@sunshinejr
Copy link
Author

sunshinejr commented Oct 22, 2019

@ikesyo I just found an alternative approach to that problem - using Rocket for the release process, which can remove testing dependencies just for the release tag. Do you think we should consider this approach instead? RxSwiftCommunity/RxOptional#83

@andersio
Copy link
Member

Closed in favor of #784

@andersio andersio closed this May 22, 2020
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.

None yet

4 participants