saoudrizwan / Disk

Delightful framework for iOS to easily persist structs, images, and data
MIT License
3.08k stars 170 forks source link

0.6.4 watchOS, tvOS, macOS support #72

Open craigrouse opened 5 years ago

craigrouse commented 5 years ago

Added watchOS, tvOS, macOS support.

Merged https://github.com/saoudrizwan/Disk/pull/66 Merged https://github.com/saoudrizwan/Disk/pull/53

saoudrizwan commented 5 years ago

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

kylebrowning commented 5 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

fl034 commented 4 years ago

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 commented 4 years ago

Any update about this PR ? πŸ™‚

jarrodparkes commented 4 years ago

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 commented 4 years ago

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

jarrodparkes commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

Thanks for clarifying. TIL πŸ‘

jarrodparkes commented 4 years ago

@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 commented 4 years ago

If you’re supporting it ask the maintainers.

jarrodparkes commented 4 years ago

@saoudrizwan πŸ‘€ ?

bgoncal commented 1 year ago

No plans to merge this?

kylebrowning commented 1 year ago

Ill merge it on my fork if someone wants

bgoncal commented 1 year ago

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

kylebrowning commented 1 year ago

yes

https://github.com/kylebrowning/Storage

bgoncal commented 1 year ago

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

kylebrowning commented 1 year ago

Sure