Hides ShellOut implementation in iOS target

[ohitsdaniel]

As mentioned in #45, if a target in your workspace directly or indirectly depends on ShellOut and you try to build the SwiftUI preview target, the compilation fails as Process is not available on iOS. Swift Package Manager does currently not supported limiting on which platforms a package can be built. This PR hides the ShellOut implementation using # if os(macOS) || os(Linux) in the iOS target so that its build succeeds.

[ohitsdaniel]

Ping @JohnSundell. As the issue is blocking us from using the SwiftUI canvas, it would be nice to get this change public asap. :)

[JohnSundell]

@ohitsdaniel: if one of your third party dependencies can end up blocking you from doing your work, then I would strongly suggest that you re-evaluate the way you do dependency management. That doesn't sound sustainable. To unblock yourself, you could, for instance, fork ShellOut and apply this fix on your fork, and then point your project to that fork instead (until a proper fix has been developed and merge into this repo).

As for this PR, I'm not going to accept a change that wraps the entire library in an #if directive. I think we'll be able to use SwiftPM's new platform attribute to solve this, but I haven't had time to work on this yet. I'll get to it as quickly as I can, and no amount of "pinging" is going to change that.

[ohitsdaniel]

Hey John, thanks for responding to this PR. First of all, I wanted to apologise if "pinging" you made you feel pressured to act or insulted you in any way. That was definitely not my intention.

Regarding your feedback concerning our dependency management, I guess my wording was a bit off and I would like to clarify some things: ShellOut is a transitive dependency of one of our testing tools (SwiftyMocky). This issue is not blocking us as we're using forks (just as you suggested), however it creates the overhead of maintaining two additional repositories. Resolving this situation would be nice, but I absolutely understand that this is none of your concern and absolutely not your responsibility. Thanks for the constructive feedback and suggesting a solution! 🙂

Regarding the actual content of this PR. When I ran into this issue, I had the same assumption that the newly introduced platforms attribute in the package definition could solve this issue as I assumed that we can list all supported platforms and exclude unsupported ones. This unfortunately is not the case and the platforms array is currently only used to set custom minimum deployment targets for specific platforms. SPM packages assume to support all platforms using a predefined minimum deployment target. Leaving out a certain platform in the platforms array is basically the same as specifying the default minimum deployment target. This is also stated in the evolution proposal and listed as one of the points that the SPM team is aiming to improve in the future. (See Proposed Solution and Future directions (Especially Platform-specific targets or products and Restricting supported platforms): https://github.com/apple/swift-evolution/blob/master/proposals/0236-package-manager-platform-deployment-settings.md#proposed-solution)

I wholeheartedly agree with you that enclosing the entire library in an #if directive is less than ideal and should be avoided if possible. If you think we should wait for SPM to implement restricting supported platforms, I'm absolutely fine with it and we will continue to use our forks in the meanwhile. If you need any support implementing platform restrictions for this package in the future, feel free to reach out to me as I'm happy to help. 🙂

[JohnSundell]

If you didn't intend to pressure me to act, then why did you ping me like that? 😅

No worries though, I understand that this issue is important to you, it's just that I have to balance this against the continued maintenance of ShellOut itself, as well as all of my other work priorities.

I believe that Swift 5.3 includes new SwiftPM capabilities that would solve this issue (specifically, it'll let packages specify dependencies on a per-platform basis), but again, this is something that I'll need a bit more time to research.

[ohitsdaniel]

Once again, sorry for the ping. I expected /some/ sign of life either in the issue or in the corresponding PR. Doesn't mean I have to ping you though. I'm aware that you have to balance your priorities. Thanks for looking into this. 👏

Whenever I find the time, I'll look into this and see if we can find a different solution. I did some more research into the new Swift 5.3 SPM features. (Based on the https://www.hackingwithswift.com/articles/218/whats-new-in-swift-5-3 and the SPM project changelog).

There are 4 new SPM features in Swift 5.3: Package Manager Resources (unrelated), Package Manager Binary Dependencies (unrelated), Package Manager Localized Resources (unrelated), and Conditional target dependencies (somehow related): https://github.com/apple/swift-evolution/blob/master/proposals/0273-swiftpm-conditional-target-dependencies.md

I checked the proposal for conditional target dependencies and it includes this example Package.swift

import PackageDescription
let package = Package(
    name: "BestPackage",
    dependencies: [
        .package(url: "https://github.com/pureswift/bluetooth", .branch("master")),
        .package(url: "https://github.com/pureswift/bluetoothlinux", .branch("master")),
    ],
    targets: [
        .target(
            name: "BestExecutable",
            dependencies: [
                .product(name: "Bluetooth", condition: .when(platforms: [.macOS])),
                .product(name: "BluetoothLinux", condition: .when(platforms: [.linux])),
                .target(name: "DebugHelpers", condition: .when(configuration: .debug)),
            ]
        ),
        .target(name: "DebugHelpers")
     ]
)

The implemented change only affects which targets are built and linked against each other during compilation. In our source code we will still have to add checks for #if os(macOS) / #if canImport(Bluetooth) around any platform-specific code. Currently, there is no way to define that a product (i.e. the produced library) can only be consumed on a set of platforms. People could still define .product(name: "BluetoothLinux", condition: .when(platforms: [.macOS])) as a dependency even though it's not meant to be consumed in a macOS target.

Consumers could use .product(name: "ShellOut", condition: .when(platforms: [.macOS, .linux])), to not build and link ShellOut when building the target for iOS. However, this would mean that consumers would have to enclose the ShellOut imports and usages in their code in #if canImport(ShellOut) or #if os(macOS) || os(Linux). Ideally, they would already do this, but putting the responsibility on the consumers of the library also doesn't feel right to me (just a feeling, very opinionated here).

I won't say that we should encourage misuse of libraries, but as SPM does not enable us to shield us from it, I guess package authors can do two things: make all their packages work on all platforms or make sure that their packages build on all platforms without offering the complete feature set / API.

[ohitsdaniel]

FYI: Xcode 12 fixes the behaviour that led to the underlying problem in previews. Therefore, this PR is no longer necessary. Seems like you knew something I didn't. :)