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

0.6.4 watchOS, tvOS, macOS support #72

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

craigrouse
Copy link

@craigrouse craigrouse commented Jun 13, 2019

Added watchOS, tvOS, macOS support.

Merged #66
Merged #53

Added watchOS, tvOS, macOS support.
@craigrouse craigrouse mentioned this pull request Jun 13, 2019
@saoudrizwan
Copy link
Owner

Wow great work! Thanks for your contribution! I will take a look over everything and hopefully merge soon.

@kylebrowning
Copy link

kylebrowning commented Jun 19, 2019

@saoudrizwan Can you also update package.swift.

// swift-tools-version:5.0
// The swift-tools-version declares the minimum version of Swift required to build this package.

import PackageDescription

let package = Package(
    name: "Disk",
    products: [
        // Products define the executables and libraries produced by a package, and make them visible to other packages.
        .library(
            name: "Disk",
            targets: ["Disk"]),
    ],
    dependencies: [
        // Dependencies declare other packages that this package depends on.
        // .package(url: /* package url */, from: "1.0.0"),
    ],
    targets: [
        // Targets are the basic building blocks of a package. A target can define a module or a test suite.
        // Targets can depend on other targets in this package, and on products in packages which this package depends on.
        .target(
            name: "Disk",
            dependencies: []),
        .testTarget(
            name: "DiskTests",
            dependencies: ["Disk"]),
    ]
)

@fl034
Copy link

fl034 commented Oct 10, 2019

When I try to use your PR with CocoaPods in a tvOS project like this:

pod 'Disk', :git => 'https://github.com/craigrouse/Disk.git', :branch => 'master'

I get this error:

Pre-downloading: `Disk` from `https://github.com/craigrouse/Disk.git`, branch `master`
[!] The platform of the target `MyApp` (tvOS 11.0) is not compatible with `Disk (0.6.4)`, which does not support `tvOS`.

Using Cocoapods 1.8.3

@craigrouse
Copy link
Author

@fl034 you need to tell CocoaPods to use the new podspec on my branch

pod 'Disk', :path=>'/path/to/podspec.podspec'

If you just point to the branch, it is still going to attempt to use the default podspec, which does not include iOS support. https://github.com/craigrouse/Disk

I hope this makes sense. Sorry to promote my own post, but you may find some useful hints in here: https://medium.com/@craig.rouse/10-tips-for-distributing-your-framework-via-cocoapods-ed8e63561c66 :) I constantly have to refer back to it myself when building new pods.

@fl034
Copy link

fl034 commented Oct 11, 2019

First of all, thanks for your efforts. It's working great, but I had to embed it without CocoaPods.

I followed your advice and downloaded your project, and embedded it via :path=> but got exactly the same error. When I remove all other platforms from the podspec (except tvOS), then it works.
That means the correct podspec is being used.

I guess this is also the case my approach in my first comment, since I specified your Repo where your changed podspec is present.

When I look at Alamofire's podspec, they're doing this:

  s.ios.deployment_target = '10.0'
  s.osx.deployment_target = '10.12'
  s.tvos.deployment_target = '9.0'
  s.watchos.deployment_target = '3.0'

The docs also say here (https://guides.cocoapods.org/syntax/podspec.html#deployment_target)

As opposed to the platform attribute, the deployment_target attribute allows to specify multiple platforms on which this pod is supported — specifying a different deployment target for each.

I tried this approach and it's working with your code, replacing those lines in your podspec.
https://github.com/craigrouse/Disk/blob/b9729bdfe495f7d34aa48915e7302a353fc2f720/Disk.podspec#L13-L16

@craigrouse
Copy link
Author

Thanks @fl034! If you'd like to submit a PR to my branch, I'll gladly merge it.

@fl034
Copy link

fl034 commented Oct 12, 2019

Thanks for the offer, but feel free to do it yourself. Would be more work to fork and PR than the actual change :D

@omar-belhaouss
Copy link

Any update about this PR ? 🙂

@jarrodparkes
Copy link

I attempted to test your changes with Big Sur and the newest version of Xcode (increasingly likely as WWDC finishes today). I ran into the following issues:

Screen Shot 2020-06-26 at 9 17 02 AM

Screen Shot 2020-06-26 at 9 15 01 AM

Not sure if this PR includes changes to the swift-tools-version, but I'm assuming it might.

@kylebrowning
Copy link

kylebrowning commented Jun 26, 2020

Thats because the owner of this package has decided to require a monthly subscription to get updates, https://gitroyalty.com/saoudrizwan/Disk

@jarrodparkes
Copy link

@kylebrowning ive always been confused by this. it seems like when I've used this project (via Cocoapods) that I've always been able to get around this barrier? is this a SwiftPM-only thing?

@kylebrowning
Copy link

Yeah, you have to pin SwiftPM to the version or commit in which it was supported as open source and free. otherwise you'll get the route to subscriber version.

With cocoa pods, the last free release remains in Cocoapods trunk.

@jarrodparkes
Copy link

Thanks for clarifying. TIL 👍

@jarrodparkes
Copy link

jarrodparkes commented Jun 26, 2020

@kylebrowning I'm NOW supporting the project, would you or anyone here be willing to try and help me test this branch? Each time I try to use craigrouse:master as the branch...

Screen Shot 2020-06-26 at 6 09 10 PM

I get the following error messages...

Screen Shot 2020-06-26 at 6 09 48 PM

@kylebrowning
Copy link

If you’re supporting it ask the maintainers.

@jarrodparkes
Copy link

@saoudrizwan 👀 ?

@bgoncal
Copy link

bgoncal commented Jul 26, 2022

No plans to merge this?

@kylebrowning
Copy link

Ill merge it on my fork if someone wants

@bgoncal
Copy link

bgoncal commented Jul 26, 2022

It would be really nice, are you also supporting SPM?

@kylebrowning
Copy link

yes

https://github.com/kylebrowning/Storage

@bgoncal
Copy link

bgoncal commented Aug 5, 2022

@kylebrowning works like a charm, do you have any plans to maintain it?

@kylebrowning
Copy link

Sure

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

7 participants